Messages in this thread | | | Subject | Re: [PATCH] exit: Clarify choice of new parent in forget_original_parent() | From | Kirill Tkhai <> | Date | Wed, 17 Jun 2015 20:23:14 +0300 |
| |
В Вт, 16/06/2015 в 21:27 +0200, Oleg Nesterov пишет: > 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?
Good, IMO it improves the readability.
> > > 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.
It looks like gcc inlines both of these function, so it seems there won't be a problem...
> > > +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.
Yes, I saw. Since zap_pid_ns_processes() waits for nr_hashed, and __unhash_process() deletes from pid chain and sibling list at the same time, pid_ns child_reaper can't have a child after nr_hashed == init_pids.
> 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);
Yeah, I'm agree.
> 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; >
Thanks, Kirill
| |