exit: Clarify choice of new parent in forget_original_parent()

Second parameter of find_new_reaper() and the similarity of its name and find_child_reaper()'s name confuse a reader.

Rename find_child_reaper() for better conformity of its name and its function.

Also delete the second parameter of find_new_reaper().

(quoted diff) +2 -1

+static struct task_struct *find_new_reaper(struct task_struct *father)
 {
-	struct task_struct *thread, *reaper;
+	struct task_struct *thread, *reaper, *child_reaper;
 

Review

Quoted 5 lines · Kirill Tkhai

(quoted diff) +4 -0

+	 * child_reaper doesn't have children after zap_pid_ns_processes(),
+	 * therefore it can't enter this function.
+	 */
+	BUG_ON(child_reaper == father);

Yes, we can add this BUG_ON(). But please see the comments in zap_pid_ns_processes(). We can change zap_pid_ns_processes() so that it returns with non-empty ->children list due to EXIT_DEAD children.

Unlikely we will actually do this, at least soon, so I won't argue with this BUG_ON().

But. In this case it would be better to add it into forget_original_parent(),

	reaper = find_new_reaper(...);
	BUG_ON(reaper == father);

forgot to mention,

Quoted 12 lines · Oleg Nesterov

Review

Quoted 5 lines · Kirill Tkhai

(quoted diff) +4 -0

+	 * child_reaper doesn't have children after zap_pid_ns_processes(),
+	 * therefore it can't enter this function.
+	 */
+	BUG_ON(child_reaper == father);

Yes, we can add this BUG_ON(). But please see the comments in zap_pid_ns_processes(). We can change zap_pid_ns_processes() so that it returns with non-empty ->children list due to EXIT_DEAD children.

Quoted 7 lines

Unlikely we will actually do this, at least soon, so I won't argue with this BUG_ON().

But. In this case it would be better to add it into forget_original_parent(),

	reaper = find_new_reaper(...);
	BUG_ON(reaper == father);

because this way:

1. This BUG_ON() will still be valid even if we actually change zap_pid_ns_processes() to return with EXIT_DEAD children

2. If we really want this sanity check, we should not tie it to ->child_reaper case.

OTOH. If for some reason you want to check ->child_reaper only, then you should probably do this right after list_empty(&father->children) check, or at least before find_alive_thread(). Because otherwise it looks confusing, it looks as if "child_reaper == father" is only wrong if find_alive_thread(father) fails.

Quoted 3 lines

Second parameter of find_new_reaper() and the similarity of its name and find_child_reaper()'s name confuse a reader.

OK, I agree that

	reaper = find_child_reaper(father);
	...
	reaper = find_new_reaper(father, reaper);

can look confusing and probably deserves a cleanup. How about the patch below then?

Quoted 3 lines

Rename find_child_reaper() for better conformity of its name and its function.

I never argueus with renames ;) Probably the new name looks better.

Quoted 1 line

Also delete the second parameter of find_new_reaper().

Yes, we can do this. But this 2nd argument avoids another another task_active_pid_ns(father)->child_reaper, so this is optimization.

I agree, this optimization is minor, but still I think this change needs some justification.

Quoted 10 lines

(quoted diff) +2 -1

+static struct task_struct *find_new_reaper(struct task_struct *father)
 {
-	struct task_struct *thread, *reaper;
+	struct task_struct *thread, *reaper, *child_reaper;
 
 	thread = find_alive_thread(father);
 	if (thread)
 		return thread;

Review

Quoted 6 lines

(quoted diff) +6 -0

+	child_reaper = task_active_pid_ns(father)->child_reaper;
+	/*
+	 * child_reaper doesn't have children after zap_pid_ns_processes(),
+	 * therefore it can't enter this function.
+	 */
+	BUG_ON(child_reaper == father);

Yes, we can add this BUG_ON(). But please see the comments in zap_pid_ns_processes(). We can change zap_pid_ns_processes() so that it returns with non-empty ->children list due to EXIT_DEAD children.

Unlikely we will actually do this, at least soon, so I won't argue with this BUG_ON().

But. In this case it would be better to add it into forget_original_parent(),

	reaper = find_new_reaper(...);
	BUG_ON(reaper == father);

Oh. Off-topic, but this reminds me that I forgot about another bug with ->has_child_subreaper... this needs another discussion.

Oleg.

kernel/exit.c +1 -1

@@ -551,17 +551,17 @@ static void reparent_leader(struct task_
 static void forget_original_parent(struct task_struct *father,
 					struct list_head *dead)
 {
-	struct task_struct *p, *t, *reaper;
+	struct task_struct *p, *t, *child_reaper, *reaper;
 
 	if (unlikely(!list_empty(&father->ptraced)))
 		exit_ptrace(father, dead);
 
 	/* Can drop and reacquire tasklist_lock */