Re: [PATCH 2/2] mm,oom: Do oom_task_origin() test in oom_badness().

[Michal Hocko <mhocko@kernel.org>]

On Sat 21-05-16 11:01:30, Tetsuo Handa wrote:
> Currently, oom_scan_process_thread() returns OOM_SCAN_SELECT if
> oom_task_origin() returned true. But this might cause OOM livelock.
> 
> If the OOM killer finds a task with oom_task_origin(task) == true,
> it means that that task is either inside try_to_unuse() from swapoff
> path or unmerge_and_remove_all_rmap_items() from ksm's run_store path.
> 
> Let's take a look at try_to_unuse() as an example. Although there is
> signal_pending() test inside the iteration loop, there are operations
> (e.g. mmput(), wait_on_page_*()) which might block in unkillable state
> waiting for other threads which might allocate memory.
> 
> Therefore, sending SIGKILL to a task with oom_task_origin(task) == true
> can not guarantee that that task shall not stuck at unkillable waits.
> Once the OOM reaper reaped that task's memory (or gave up reaping it),
> the OOM killer must not select that task again when oom_task_origin(task)
> returned true. We need to select different victims until that task can
> hit signal_pending() test or finish the iteration loop.
> 
> Since oom_badness() is a function which returns score of the given thread
> group with eligibility/livelock test, it is more natural and safer to let
> oom_badness() return highest score when oom_task_origin(task) == true.
> 
> This patch moves oom_task_origin() test from oom_scan_process_thread() to
> after MMF_OOM_REAPED test inside oom_badness(), changes the callers to
> receive the score using "unsigned long" variable, and eliminates
> OOM_SCAN_SELECT path in the callers.

I do not think this is the right approach. If the problem is real then
the patch just papers over deficiency of the oom_task_origin which
should be addressed instead.

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Nacked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/oom.h |  1 -
>  mm/memcontrol.c     |  9 +--------
>  mm/oom_kill.c       | 26 ++++++++++++++------------
>  3 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/oom.h b/include/linux/oom.h
> index c63de01..f6b37a4 100644
> --- a/include/linux/oom.h
> +++ b/include/linux/oom.h
> @@ -47,7 +47,6 @@ enum oom_scan_t {
>  	OOM_SCAN_OK,		/* scan thread and find its badness */
>  	OOM_SCAN_CONTINUE,	/* do not consider thread for oom kill */
>  	OOM_SCAN_ABORT,		/* abort the iteration and return */
> -	OOM_SCAN_SELECT,	/* always select this thread first */
>  };
>  
>  extern struct mutex oom_lock;
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 49cee6f..73c8c44 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1263,7 +1263,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	struct mem_cgroup *iter;
>  	unsigned long chosen_points = 0;
>  	unsigned long totalpages;
> -	unsigned int points = 0;
> +	unsigned long points = 0;
>  	struct task_struct *chosen = NULL;
>  
>  	mutex_lock(&oom_lock);
> @@ -1288,13 +1288,6 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  		css_task_iter_start(&iter->css, &it);
>  		while ((task = css_task_iter_next(&it))) {
>  			switch (oom_scan_process_thread(&oc, task)) {
> -			case OOM_SCAN_SELECT:
> -				if (chosen)
> -					put_task_struct(chosen);
> -				chosen = task;
> -				chosen_points = ULONG_MAX;
> -				get_task_struct(chosen);
> -				/* fall through */
>  			case OOM_SCAN_CONTINUE:
>  				continue;
>  			case OOM_SCAN_ABORT:
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 743afdd..c2ed496 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -186,6 +186,19 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
>  	}
>  
>  	/*
> +	 * If task is allocating a lot of memory and has been marked to be
> +	 * killed first if it triggers an oom, then select it.
> +	 *
> +	 * Score ULONG_MAX / 1000 rather than ULONG_MAX is used in order to
> +	 * avoid overflow when the caller multiplies this score later using
> +	 * "1000 / totalpages".
> +	 */
> +	if (oom_task_origin(p)) {
> +		task_unlock(p);
> +		return ULONG_MAX / 1000;
> +	}
> +
> +	/*
>  	 * The baseline for the badness score is the proportion of RAM that each
>  	 * task's rss, pagetable and swap space use.
>  	 */
> @@ -286,13 +299,6 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
>  	if (!is_sysrq_oom(oc) && atomic_read(&task->signal->oom_victims))
>  		return OOM_SCAN_ABORT;
>  
> -	/*
> -	 * If task is allocating a lot of memory and has been marked to be
> -	 * killed first if it triggers an oom, then select it.
> -	 */
> -	if (oom_task_origin(task))
> -		return OOM_SCAN_SELECT;
> -
>  	return OOM_SCAN_OK;
>  }
>  
> @@ -309,13 +315,9 @@ static struct task_struct *select_bad_process(struct oom_control *oc,
>  
>  	rcu_read_lock();
>  	for_each_process(p) {
> -		unsigned int points;
> +		unsigned long points;
>  
>  		switch (oom_scan_process_thread(oc, p)) {
> -		case OOM_SCAN_SELECT:
> -			chosen = p;
> -			chosen_points = ULONG_MAX;
> -			/* fall through */
>  		case OOM_SCAN_CONTINUE:
>  			continue;
>  		case OOM_SCAN_ABORT:
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>