Re: [PATCH] oom: consider multi-threaded tasks in task_will_free_mem

[Michal Hocko]

On Tue 26-04-16 15:57:52, Michal Hocko wrote:
> On Tue 12-04-16 11:19:16, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@xxxxxxxx>
> > 
> > task_will_free_mem is a misnomer for a more complex PF_EXITING test
> > for early break out from the oom killer because it is believed that
> > such a task would release its memory shortly and so we do not have
> > to select an oom victim and perform a disruptive action.
> > 
> > Currently we make sure that the given task is not participating in the
> > core dumping because it might get blocked for a long time - see
> > d003f371b270 ("oom: don't assume that a coredumping thread will exit
> > soon").
> > 
> > The check can still do better though. We shouldn't consider the task
> > unless the whole thread group is going down. This is rather unlikely
> > but not impossible. A single exiting thread would surely leave all the
> > address space behind. If we are really unlucky it might get stuck on the
> > exit path and keep its TIF_MEMDIE and so block the oom killer.
> > 
> > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> > ---
> > 
> > Hi,
> > I hope I got it right but I would really appreciate if Oleg found some
> > time and double checked after me. The fix is more cosmetic than anything
> > else but I guess it is worth it.
> 
> ping...

Andrew, this is not in the mmotm tree now because I didn't feel really
confortable with the patch without Oleg seeing it. But it seems Oleg is
ok [1] with it so could you push it to Linus along with the rest of oom
pile please?

[1] http://lkml.kernel.org/r/20160517184225.GB32068@xxxxxxxxxx

> > 
> > Thanks!
> > 
> >  include/linux/oom.h | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/oom.h b/include/linux/oom.h
> > index 628a43242a34..b09c7dc523ff 100644
> > --- a/include/linux/oom.h
> > +++ b/include/linux/oom.h
> > @@ -102,13 +102,24 @@ extern struct task_struct *find_lock_task_mm(struct task_struct *p);
> >  
> >  static inline bool task_will_free_mem(struct task_struct *task)
> >  {
> > +	struct signal_struct *sig = task->signal;
> > +
> >  	/*
> >  	 * A coredumping process may sleep for an extended period in exit_mm(),
> >  	 * so the oom killer cannot assume that the process will promptly exit
> >  	 * and release memory.
> >  	 */
> > -	return (task->flags & PF_EXITING) &&
> > -		!(task->signal->flags & SIGNAL_GROUP_COREDUMP);
> > +	if (sig->flags & SIGNAL_GROUP_COREDUMP)
> > +		return false;
> > +
> > +	if (!(task->flags & PF_EXITING))
> > +		return false;
> > +
> > +	/* Make sure that the whole thread group is going down */
> > +	if (!thread_group_empty(task) && !(sig->flags & SIGNAL_GROUP_EXIT))
> > +		return false;
> > +
> > +	return true;
> >  }
> >  
> >  /* sysctls */
> > -- 
> > 2.8.0.rc3
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs