On Thu, 28 Jun 2012, Oleg Nesterov wrote:
> > @@ -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().
>
oom_kill_process() will now do the put_task_struct() since we need a
reference before killing it, so callers to oom_kill_process() are
responsible for grabbing it before doing rcu_read_unlock().
> 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);
So if the victim has no threads that have an mm, then we have raced with
select_bad_process() and we silently return.
> > return;
> > + } else
> > + victim = p;
>
> And, before return,
>
> > + put_task_struct(victim);
>
> Doesn't look right if victim != p.
>
Ah, good catch, we need to do
if (!p) {
rcu_read_unlock();
put_task_struct(victim);
return;
} else {
put_task_struct(victim);
victim = p;
get_task_struct(victim);
rcu_read_unlock();
}
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>