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@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </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@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>