Re: [patch] mm, oom: avoid checking set of allowed nodes twice when selecting a victim

[Michal Hocko <mhocko@suse.cz>]

On Tue 24-04-12 16:09:14, David Rientjes wrote:
> On Thu, 12 Apr 2012, Michal Hocko wrote:
> 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 46bf2ed5..a9df008 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -171,23 +171,10 @@ static bool oom_unkillable_task(struct task_struct *p,
> >  	return false;
> >  }
> >  
> > -/**
> > - * oom_badness - heuristic function to determine which candidate task to kill
> > - * @p: task struct of which task we should calculate
> > - * @totalpages: total present RAM allowed for page allocation
> > - *
> > - * The heuristic for determining which task to kill is made to be as simple and
> > - * predictable as possible.  The goal is to return the highest value for the
> > - * task consuming the most memory to avoid subsequent oom failures.
> > - */
> > -unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
> > +/* can be used only for tasks which are killable as per oom_unkillable_task */
> > +static unsigned int __oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
> >  		      const nodemask_t *nodemask, unsigned long totalpages)
> >  {
> > -	long points;
> > -
> > -	if (oom_unkillable_task(p, memcg, nodemask))
> > -		return 0;
> > -
> >  	p = find_lock_task_mm(p);
> >  	if (!p)
> >  		return 0;
> > @@ -239,6 +226,26 @@ unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
> >  	return (points < 1000) ? points : 1000;
> >  }
> >  
> > +/**
> > + * oom_badness - heuristic function to determine which candidate task to kill
> > + * @p: task struct of which task we should calculate
> > + * @totalpages: total present RAM allowed for page allocation
> > + *
> > + * The heuristic for determining which task to kill is made to be as simple and
> > + * predictable as possible.  The goal is to return the highest value for the
> > + * task consuming the most memory to avoid subsequent oom failures.
> > + */
> > +unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
> > +		      const nodemask_t *nodemask, unsigned long totalpages)
> > +{
> > +	long points;
> > +
> > +	if (oom_unkillable_task(p, memcg, nodemask))
> > +		return 0;
> > +
> > +	return __oom_badness(p, memcg, nodemask, totalpages);
> > +}
> > +
> >  /*
> >   * Determine the type of allocation constraint.
> >   */
> > @@ -366,7 +373,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
> >  			}
> >  		}
> >  
> > -		points = oom_badness(p, memcg, nodemask, totalpages);
> > +		points = __oom_badness(p, memcg, nodemask, totalpages);
> >  		if (points > *ppoints) {
> >  			chosen = p;
> >  			*ppoints = points;
> 
> No, the way I had it written is correct: the above unnecessarily checks 
> for membership in a memcg or intersection with a set of allowable nodes 
> for child threads in oom_kill_process().  

your patch does 
	if (oom_unkillable_task(child, memcg, nodemask))
		continue;
	oom_badness((child, memcg, nodemask,
				   totalpages);

in oom_kill_process so the check is very same. Or am I missing
something?


> With a lot of children and with 
> a CONFIG_NODES_SHIFT significantly large (the prerequisite for this patch 
> to make any difference), that's too costly to do.
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>