On 06/27, David Rientjes wrote: > > On Tue, 26 Jun 2012, David Rientjes wrote: > > > It turns out that task->children is not an rcu-protected list so this > > doesn't work. Yes. And just in case, we can't rcuify ->children because of re-parenting. > It's a tough patch to review, but the basics are that > > - oom_kill_process() is made to no longer need tasklist_lock; it's only > taken for the iteration over children and everything else, including > dump_header() is protected by rcu_read_lock() for kernels enabling > /proc/sys/vm/oom_dump_tasks, > > - oom_kill_process() assumes that we have a reference to p, the victim, > when it's called. It can release this reference and grab a child's > reference if necessary and drops it before returning, and > > - select_bad_process() does not require tasklist_lock, it gets > protected by rcu_read_lock() as well. Looks correct at first glance... (ignoring the fact we need the fixes in while_each_thread/rcu interaction but this is off-topic and should be fixed anyway). > @@ -348,6 +348,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints, > struct task_struct *chosen = NULL; > unsigned long chosen_points = 0; > > + rcu_read_lock(); > do_each_thread(g, p) { > unsigned int points; > > @@ -370,6 +371,9 @@ static struct task_struct *select_bad_process(unsigned int *ppoints, > chosen_points = points; > } > } while_each_thread(g, p); > + if (chosen) > + get_task_struct(chosen); OK, so the caller should do put_task_struct(). But, unless I misread the patch, > @@ -454,6 +458,7 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, > ... > + rcu_read_lock(); > + p = find_lock_task_mm(victim); > + if (!p) { > + rcu_read_unlock(); > + put_task_struct(victim); > return; > + } else > + victim = p; And, before return, > + put_task_struct(victim); Doesn't look right if victim != p. Oleg. -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html