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

[David Rientjes <rientjes@google.com>]

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().  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>