Messages in this thread Patch in this message | | | Date | Tue, 16 Jun 2015 21:27:59 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] exit: Clarify choice of new parent in forget_original_parent() |
| |
On 06/16, Kirill Tkhai wrote: > > 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?
> Rename find_child_reaper() for better conformity of its name and its > function.
I never argueus with renames ;) Probably the new name looks better.
> 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.
> +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; > > + 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.
--- x/kernel/exit.c +++ x/kernel/exit.c @@ -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 */ - reaper = find_child_reaper(father); + child_reaper = find_child_reaper(father); if (list_empty(&father->children)) return; - reaper = find_new_reaper(father, reaper); + reaper = find_new_reaper(father, child_reaper); list_for_each_entry(p, &father->children, sibling) { for_each_thread(p, t) { t->real_parent = reaper;
| |