Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying.

[Michal Hocko <mhocko@suse.com>]

On Thu 08-12-16 20:00:39, Tetsuo Handa wrote:
> Cc'ing people involved in commit dc56401fc9f25e8f ("mm: oom_kill: simplify
> OOM killer locking") and Sergey as printk() expert. Topic started from
> http://lkml.kernel.org/r/1481020439-5867-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp .
> 
> Vlastimil Babka wrote:
> > > May I? Something like below? With patch below, the OOM killer can send
> > > SIGKILL smoothly and printk() can report smoothly (the frequency of
> > > "** XXX printk messages dropped **" messages is significantly reduced).
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 2c6d5f6..ee0105b 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -3075,7 +3075,7 @@ void warn_alloc(gfp_t gfp_mask, const char *fmt, ...)
> > >  	 * Acquire the oom lock.  If that fails, somebody else is
> > >  	 * making progress for us.
> > >  	 */
> > 
> > The comment above could use some updating then. Although maybe "somebody 
> > killed us" is also technically "making progress for us" :)
> 
> I think we can update the comment. But since __GFP_KILLABLE does not exist,
> SIGKILL is pending does not imply that current thread will make progress by
> leaving the retry loop immediately. Therefore,

Although this is true I do not think that cluttering the code with this
case is anyhow useful. In the vast majority of cases SIGKILL pending
will be a result of the oom killer.

[...]
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6de9440..6c43d8e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3037,12 +3037,16 @@ void warn_alloc(gfp_t gfp_mask, const char *fmt, ...)
>  	*did_some_progress = 0;
>  
>  	/*
> -	 * Acquire the oom lock.  If that fails, somebody else is
> -	 * making progress for us.
> +	 * Give the OOM killer enough CPU time for sending SIGKILL.
> +	 * Do not return without a short sleep unless TIF_MEMDIE is set, for
> +	 * currently tsk_is_oom_victim(current) == true does not make
> +	 * gfp_pfmemalloc_allowed() == true via TIF_MEMDIE until
> +	 * mark_oom_victim(current) is called.
>  	 */
> -	if (!mutex_trylock(&oom_lock)) {
> +	if (mutex_lock_killable(&oom_lock)) {
>  		*did_some_progress = 1;
> -		schedule_timeout_uninterruptible(1);
> +		if (!test_thread_flag(TIF_MEMDIE))
> +			schedule_timeout_uninterruptible(1);

I am not really sure this is necessary. Just return outside and for
those unlikely cases where the current task was killed before entering
the page allocator simply do not matter imho. I would rather go with
simplicity here.

>  		return NULL;
>  	}
>  
> -- 
> 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>