Process groups are now reference-counted. Processes in a group as well

as the death entries of a deceased processes that were in this group
and have not yet been reaped hold references to the group, so that it
won't be deleted until the group is empty and all gone group members
have been reaped. This fixes #1799 at last.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@24335 a95241bf-73f2-0310-859d-f6bbb57e9c96

headers/private/kernel/team.h

@@ -17,7 +17,7 @@ status_t team_init(struct kernel_args *args);
 team_id team_create_team(const char *path, const char *name, char **args,
 			int argc, char **envp, int envc, int priority);
 status_t wait_for_team(team_id id, status_t *returnCode);
-void team_remove_team(struct team *team, struct process_group **_freeGroup);
+void team_remove_team(struct team *team);
 void team_delete_team(struct team *team);
 struct process_group *team_get_process_group_locked(
 			struct process_session *session, pid_t id);

headers/private/kernel/thread_types.h

@@ -80,6 +80,7 @@ struct process_group {
 	struct process_group *next;		// next in hash
 	struct process_session *session;
 	pid_t				id;
+	int32				refs;
 	struct team			*teams;
 	bool				orphaned;
 };
@@ -115,6 +116,7 @@ typedef struct job_control_entry job_control_entry;
 struct job_control_entry : DoublyLinkedListLinkImpl<job_control_entry> {
 	job_control_state	state;		// current team job control state
 	thread_id			thread;		// main thread ID == team ID
+	bool				has_group_ref;
 
 	// valid while state != JOB_CONTROL_STATE_DEAD
 	struct team*		team;
@@ -124,6 +126,13 @@ struct job_control_entry : DoublyLinkedListLinkImpl<job_control_entry> {
 	status_t			status;
 	uint16				reason;
 	uint16				signal;
+
+	job_control_entry();
+	~job_control_entry();
+
+	void InitDeadState();
+
+	job_control_entry& operator=(const job_control_entry& other);
 };
 
 typedef DoublyLinkedList<job_control_entry> JobControlEntryList;

src/system/kernel/team.cpp

@@ -19,6 +19,7 @@
 
 #include <elf.h>
 #include <file_cache.h>
+#include <heap.h>
 #include <int.h>
 #include <kernel.h>
 #include <kimage.h>
@@ -620,29 +621,21 @@ is_process_group_leader(struct team *team)
 }
 
 
-/*!	You must hold the team lock when calling this function. */
 static void
-insert_group_into_session(struct process_session *session, struct process_group *group)
+deferred_delete_process_group(struct process_group *group)
 {
 	if (group == NULL)
 		return;
 
-	group->session = session;
-	hash_insert(sGroupHash, group);
-	session->group_count++;
-}
+	// remove_group_from_session() keeps this pointer around
+	// only if the session can be freed as well
+	if (group->session) {
+		TRACE(("deferred_delete_process_group(): frees session %ld\n",
+			group->session->id));
+		deferred_free(group->session);
+	}
 
-
-/*!	You must hold the team lock when calling this function. */
-static void
-insert_team_into_group(struct process_group *group, struct team *team)
-{
-	team->group = group;
-	team->group_id = group->id;
-	team->session_id = group->session->id;
-
-	team->group_next = group->teams;
-	group->teams = team;
+	deferred_free(group);
 }
 
 
@@ -668,23 +661,84 @@ remove_group_from_session(struct process_group *group)
 }
 
 
-/*!	Removes the team from the group. If that group becomes therefore
-	unused, it will set \a _freeGroup to point to the group - otherwise
-	it will be \c NULL.
-	It cannot be freed here because this function has to be called
-	with having the team lock held.
-
-	\param team the team that'll be removed from it's group
-	\param _freeGroup points to the group to be freed or NULL
+/*!	Team lock must be held.
 */
 static void
-remove_team_from_group(struct team *team, struct process_group **_freeGroup)
+acquire_process_group_ref(pid_t groupID)
+{
+	process_group* group = team_get_process_group_locked(NULL, groupID);
+	if (group == NULL) {
+		panic("acquire_process_group_ref(): unknown group ID: %ld", groupID);
+		return;
+	}
+
+	group->refs++;
+}
+
+
+/*!	Team lock must be held.
+*/
+static void
+release_process_group_ref(pid_t groupID)
+{
+	process_group* group = team_get_process_group_locked(NULL, groupID);
+	if (group == NULL) {
+		panic("release_process_group_ref(): unknown group ID: %ld", groupID);
+		return;
+	}
+
+	if (group->refs <= 0) {
+		panic("release_process_group_ref(%ld): ref count already 0", groupID);
+		return;
+	}
+
+	if (--group->refs > 0)
+		return;
+
+	// group is no longer used
+
+	remove_group_from_session(group);
+	deferred_delete_process_group(group);
+}
+
+
+/*!	You must hold the team lock when calling this function. */
+static void
+insert_group_into_session(struct process_session *session, struct process_group *group)
+{
+	if (group == NULL)
+		return;
+
+	group->session = session;
+	hash_insert(sGroupHash, group);
+	session->group_count++;
+}
+
+
+/*!	You must hold the team lock when calling this function. */
+static void
+insert_team_into_group(struct process_group *group, struct team *team)
+{
+	team->group = group;
+	team->group_id = group->id;
+	team->session_id = group->session->id;
+
+	team->group_next = group->teams;
+	group->teams = team;
+	acquire_process_group_ref(group->id);
+}
+
+
+/*!	Removes the team from the group.
+
+	\param team the team that'll be removed from it's group
+*/
+static void
+remove_team_from_group(struct team *team)
 {
 	struct process_group *group = team->group;
 	struct team *current, *last = NULL;
 
-	*_freeGroup = NULL;
-
 	// the team must be in any team to let this function have any effect
 	if  (group == NULL)
 		return;
@@ -705,13 +759,7 @@ remove_team_from_group(struct team *team, struct process_group **_freeGroup)
 	team->group = NULL;
 	team->group_next = NULL;
 
-	if (group->teams != NULL)
-		return;
-
-	// we can remove this group as it is no longer used
-
-	remove_group_from_session(group);
-	*_freeGroup = group;
+	release_process_group_ref(group->id);
 }
 
 
@@ -723,6 +771,7 @@ create_process_group(pid_t id)
 		return NULL;
 
 	group->id = id;
+	group->refs = 0;
 	group->session = NULL;
 	group->teams = NULL;
 	group->orphaned = true;
@@ -1095,7 +1144,6 @@ load_image_etc(int32 argCount, char * const *args, int32 envCount,
 	char * const *env, int32 priority, uint32 flags,
 	port_id errorPort, uint32 errorToken, bool kernel)
 {
-	struct process_group *group;
 	struct team *team, *parent;
 	const char *threadName;
 	thread_id thread;
@@ -1221,14 +1269,13 @@ err1:
 	state = disable_interrupts();
 	GRAB_TEAM_LOCK();
 
-	remove_team_from_group(team, &group);
+	remove_team_from_group(team);
 	remove_team_from_parent(parent, team);
 	hash_remove(sTeamHash, team);
 
 	RELEASE_TEAM_LOCK();
 	restore_interrupts(state);
 
-	team_delete_process_group(group);
 	delete_team_struct(team);
 
 	return status;
@@ -1381,7 +1428,6 @@ fork_team(void)
 {
 	struct thread *parentThread = thread_get_current_thread();
 	struct team *parentTeam = parentThread->team, *team;
-	struct process_group *group = NULL;
 	struct fork_arg *forkArgs;
 	struct area_info info;
 	thread_id threadID;
@@ -1490,14 +1536,13 @@ err1:
 	state = disable_interrupts();
 	GRAB_TEAM_LOCK();
 
-	remove_team_from_group(team, &group);
+	remove_team_from_group(team);
 	remove_team_from_parent(parentTeam, team);
 	hash_remove(sTeamHash, team);
 
 	RELEASE_TEAM_LOCK();
 	restore_interrupts(state);
 
-	team_delete_process_group(group);
 	delete_team_struct(team);
 
 	return status;
@@ -1565,6 +1610,57 @@ get_job_control_entry(struct team* team, pid_t id, uint32 flags)
 }
 
 
+job_control_entry::job_control_entry()
+	:
+	has_group_ref(false)
+{
+}
+
+
+job_control_entry::~job_control_entry()
+{
+	if (has_group_ref) {
+		InterruptsSpinLocker locker(team_spinlock);
+		release_process_group_ref(group_id);
+	}
+}
+
+
+/*!	Team and thread lock must be held.
+*/
+void
+job_control_entry::InitDeadState()
+{
+	if (team != NULL) {
+		struct thread* thread = team->main_thread;
+		group_id = team->group_id;
+		this->thread = thread->id;
+		status = thread->exit.status;
+		reason = thread->exit.reason;
+		signal = thread->exit.signal;
+		team = NULL;
+		acquire_process_group_ref(group_id);
+		has_group_ref = true;
+	}
+}
+
+
+job_control_entry&
+job_control_entry::operator=(const job_control_entry& other)
+{
+	state = other.state;
+	thread = other.thread;
+	has_group_ref = false;
+	team = other.team;
+	group_id = other.group_id;
+	status = other.status;
+	reason = other.reason;
+	signal = other.signal;
+
+	return *this;
+}
+
+
 /*! This is the kernel backend for waitpid(). It is a bit more powerful when it
 	comes to the reason why a thread has died than waitpid() can be.
 */
@@ -2041,11 +2137,9 @@ team_set_foreground_process_group(int32 ttyIndex, pid_t processGroupID)
 /*!	Removes the specified team from the global team hash, and from its parent.
 	It also moves all of its children up to the parent.
 	You must hold the team lock when you call this function.
-	If \a _freeGroup is set to a value other than \c NULL, it must be freed
-	from the calling function.
 */
 void
-team_remove_team(struct team *team, struct process_group **_freeGroup)
+team_remove_team(struct team *team)
 {
 	struct team *parent = team->parent;
 
@@ -2109,7 +2203,7 @@ team_remove_team(struct team *team, struct process_group **_freeGroup)
 	reparent_children(team);
 
 	// remove us from our process group
-	remove_team_from_group(team, _freeGroup);
+	remove_team_from_group(team);
 
 	// remove us from our parent
 	remove_team_from_parent(parent, team);
@@ -2870,7 +2964,6 @@ _user_setpgid(pid_t processID, pid_t groupID)
 
 	status_t status = B_OK;
 	struct process_group *freeGroup = NULL;
-	struct process_group *freeGroup2 = NULL;
 
 	InterruptsSpinLocker locker(team_spinlock);
 
@@ -2893,7 +2986,7 @@ _user_setpgid(pid_t processID, pid_t groupID)
 			if (targetGroup != NULL) {
 				// In case of processID == groupID we have to free the
 				// allocated group.
-				freeGroup2 = group;
+				freeGroup = group;
 			} else if (processID == groupID) {
 				// We created a new process group, let us insert it into the
 				// team's session.
@@ -2905,7 +2998,7 @@ _user_setpgid(pid_t processID, pid_t groupID)
 				// we got a group, let's move the team there
 				process_group* oldGroup = team->group;
 
-				remove_team_from_group(team, &freeGroup);
+				remove_team_from_group(team);
 				insert_team_into_group(targetGroup, team);
 
 				// Update the "orphaned" flag of all potentially affected
@@ -2953,7 +3046,6 @@ _user_setpgid(pid_t processID, pid_t groupID)
 	}
 
 	team_delete_process_group(freeGroup);
-	team_delete_process_group(freeGroup2);
 
 	return status == B_OK ? groupID : status;
 }
@@ -2964,7 +3056,7 @@ _user_setsid(void)
 {
 	struct team *team = thread_get_current_thread()->team;
 	struct process_session *session;
-	struct process_group *group, *freeGroup = NULL;
+	struct process_group *group;
 	cpu_status state;
 	bool failed = false;
 
@@ -2987,7 +3079,7 @@ _user_setsid(void)
 
 	// this may have changed since the check above
 	if (!is_process_group_leader(team)) {
-		remove_team_from_group(team, &freeGroup);
+		remove_team_from_group(team);
 
 		insert_group_into_session(session, group);
 		insert_team_into_group(group, team);
@@ -3001,8 +3093,7 @@ _user_setsid(void)
 		team_delete_process_group(group);
 		free(session);
 		return B_NOT_ALLOWED;
-	} else
-		team_delete_process_group(freeGroup);
+	}
 
 	return team->group_id;
 }

src/system/kernel/thread.cpp

@@ -1192,7 +1192,6 @@ thread_exit(void)
 {
 	cpu_status state;
 	struct thread *thread = thread_get_current_thread();
-	struct process_group *freeGroup = NULL;
 	struct team *team = thread->team;
 	thread_id parentID = -1;
 	bool deleteTeam = false;
@@ -1261,12 +1260,7 @@ thread_exit(void)
 			team->job_control_entry = NULL;
 
 			if (death != NULL) {
-				death->team = NULL;
-				death->group_id = team->group_id;
-				death->thread = thread->id;
-				death->status = thread->exit.status;
-				death->reason = thread->exit.reason;
-				death->signal = thread->exit.signal;
+				death->InitDeadState();
 
 				// team_set_job_control_state() already moved our entry
 				// into the parent's list. We just check the soft limit of
@@ -1281,7 +1275,7 @@ thread_exit(void)
 			} else
 				RELEASE_THREAD_LOCK();
 
-			team_remove_team(team, &freeGroup);
+			team_remove_team(team);
 
 			send_signal_etc(parentID, SIGCHLD,
 				SIGNAL_FLAG_TEAMS_LOCKED | B_DO_NOT_RESCHEDULE);
@@ -1324,15 +1318,6 @@ thread_exit(void)
 
 	// delete the team if we're its main thread
 	if (deleteTeam) {
-		// TODO: Deleting the process group is actually a problem. According to
-		// the POSIX standard the process should become a zombie and live on
-		// until it is reaped. Hence the process group would continue to exist
-		// for that time as well. That is moving processes to it (setpgid())
-		// should work. This can actually happen e.g. when executing something
-		// like "echo foobar | wc" in the shell. The built-in "echo" could
-		// exit() even before setpgid() has been invoked for the "wc" child.
-		// Cf. bug #1799.
-		team_delete_process_group(freeGroup);
 		team_delete_team(team);
 
 		// we need to delete any death entry that made it to here