Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups

[Vlastimil Babka]

On 01/03/2017 03:38 PM, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 03-01-17 10:36:31, Tetsuo Handa wrote:
> > [...]
> > > I'm OK with "[PATCH 1/3] mm: consolidate GFP_NOFAIL checks in the allocator
> > > slowpath" given that we describe that we make __GFP_NOFAIL stronger than
> > > __GFP_NORETRY with this patch in the changelog.
> >
> > Again. __GFP_NORETRY | __GFP_NOFAIL is nonsense! I do not really see any
> > reason to describe all the nonsense combinations of gfp flags.
>
> Before [PATCH 1/3]:
>
>   __GFP_NORETRY is used as "Do not invoke the OOM killer. Fail allocation
>   request even if __GFP_NOFAIL is specified if direct reclaim/compaction
>   did not help."
>
>   __GFP_NOFAIL is used as "Never fail allocation request unless __GFP_NORETRY
>   is specified even if direct reclaim/compaction did not help."
>
> After [PATCH 1/3]:
>
>   __GFP_NORETRY is used as "Do not invoke the OOM killer. Fail allocation
>   request unless __GFP_NOFAIL is specified."
>
>   __GFP_NOFAIL is used as "Never fail allocation request even if direct
>   reclaim/compaction did not help. Invoke the OOM killer unless __GFP_NORETRY is
>   specified."
>
> Thus, __GFP_NORETRY | __GFP_NOFAIL perfectly makes sense as
> "Never fail allocation request if direct reclaim/compaction did not help.
> But do not invoke the OOM killer even if direct reclaim/compaction did not help."

It may technically do that, but how exactly is that useful, i.e. "make sense"? 
Patch 2/3 here makes sure that OOM killer is not invoked when the allocation 
context is "limited" and thus OOM might be premature (despite __GFP_NOFAIL).
What's the use case for __GFP_NORETRY | __GFP_NOFAIL ?

> > > But I don't think "[PATCH 2/3] mm, oom: do not enfore OOM killer for __GFP_NOFAIL
> > > automatically" is correct. Firstly, we need to confirm
> > >
> > >   "The pre-mature OOM killer is a real issue as reported by Nils Holland"
> > >
> > > in the changelog is still true because we haven't tested with "[PATCH] mm, memcg:
> > > fix the active list aging for lowmem requests when memcg is enabled" applied and
> > > without "[PATCH 2/3] mm, oom: do not enfore OOM killer for __GFP_NOFAIL
> > > automatically" and "[PATCH 3/3] mm: help __GFP_NOFAIL allocations which do not
> > > trigger OOM killer" applied.
> >
> > Yes I have dropped the reference to this report already in my local
> > patch because in this particular case the issue was somewhere else
> > indeed!
>
> OK.
>
> > > Secondly, as you are using __GFP_NORETRY in "[PATCH] mm: introduce kv[mz]alloc
> > > helpers" as a mean to enforce not to invoke the OOM killer
> > >
> > > 	/*
> > > 	 * Make sure that larger requests are not too disruptive - no OOM
> > > 	 * killer and no allocation failure warnings as we have a fallback
> > > 	 */
> > > 	if (size > PAGE_SIZE)
> > > 		kmalloc_flags |= __GFP_NORETRY | __GFP_NOWARN;
> > >
> > > , we can use __GFP_NORETRY as a mean to enforce not to invoke the OOM killer
> > > rather than applying "[PATCH 2/3] mm, oom: do not enfore OOM killer for
> > > __GFP_NOFAIL automatically".
> > >
>
> As I wrote above, __GFP_NORETRY | __GFP_NOFAIL perfectly makes sense.
>
> > > Additionally, although currently there seems to be no
> > > kv[mz]alloc(GFP_KERNEL | __GFP_NOFAIL) users, kvmalloc_node() in
> > > "[PATCH] mm: introduce kv[mz]alloc helpers" will be confused when a
> > > kv[mz]alloc(GFP_KERNEL | __GFP_NOFAIL) user comes in in the future because
> > > "[PATCH 1/3] mm: consolidate GFP_NOFAIL checks in the allocator slowpath" makes
> > > __GFP_NOFAIL stronger than __GFP_NORETRY.
> >
> > Using NOFAIL in kv[mz]alloc simply makes no sense at all. The vmalloc
> > fallback would be simply unreachable!
>
> My intention is shown below.
>
>  void *kvmalloc_node(size_t size, gfp_t flags, int node)
>  {
>  	gfp_t kmalloc_flags = flags;
>  	void *ret;
>
>  	/*
>  	 * vmalloc uses GFP_KERNEL for some internal allocations (e.g page tables)
>  	 * so the given set of flags has to be compatible.
>  	 */
>  	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
>
>  	/*
>  	 * Make sure that larger requests are not too disruptive - no OOM
>  	 * killer and no allocation failure warnings as we have a fallback
>  	 */
> -	if (size > PAGE_SIZE)
> +	if (size > PAGE_SIZE) {
>  		kmalloc_flags |= __GFP_NORETRY | __GFP_NOWARN;
> +		kmalloc_flags &= ~__GFP_NOFAIL;

This does make kvmalloc_node more robust against callers that would try to use 
it with __GFP_NOFAIL, but is it a good idea to allow that right now? If there 
are none yet (AFAIK?), we should rather let the existing WARN_ON kick in (which 
won't happen if we strip __GFP_NOFAIL) and discuss a better solution for such 
new future caller.

Also this means the kmalloc() cannot do "__GFP_NORETRY | __GFP_NOFAIL" so I'm 
not sure how it's related with your points above - it's not an example of the 
combination that would show that "it makes perfect sense".

Thanks,
Vlastimil