Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550162 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups Michal Hocko <mhocko@kernel.org> - 2017-01-03 21:50 +0100
    Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-04 15:30 +0100
      Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups Michal Hocko <mhocko@kernel.org> - 2017-01-04 16:30 +0100
        Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-05 12:00 +0100
          Re: [PATCH 0/3 -v3] GFP_NOFAIL cleanups Michal Hocko <mhocko@kernel.org> - 2017-01-05 13:00 +0100

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

On Tue 03-01-17 23:38:30, 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."

Stop this! Seriously... This is just wasting time...

 * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
 *   return NULL when direct reclaim and memory compaction have failed to allow
 *   the allocation to succeed.  The OOM killer is not called with the current
 *   implementation.

 * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
 *   cannot handle allocation failures. New users should be evaluated carefully
 *   (and the flag should be used only when there is no reasonable failure
 *   policy) but it is definitely preferable to use the flag rather than
 *   opencode endless loop around allocator.

Can you see how the two are asking for opposite behavior?  Asking for
not retrying for ever and not failing and rather retrying for ever
simply doesn't make any sense in any reasonable universe I can think
of. Therefore I think that it is fair to say that behavior is undefined
when both are specified.

Considering there are _no_ users which would do that any further
discussion about this is just pointless and I will not respond to any
further emails in this direction.

This is just ridiculous!

[...]
>  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;
> +	}

No there are simply no users of this and even if had one which would be
legitimate it wouldn't be as simple as this. vmalloc _doesn't_ support
GFP_NOFAIL and it would be really non-trivial to implement it. If for
nothing else there are unconditional GFP_KERNEL allocations in some
vmalloc paths (which is btw. the reason why vmalloc is not GFP_NOFS
unsafe). It would take much more to add the non-failing semantic. And I
see _no_ reason to argue with this possibility when a) there is no such
user currently and b) it is even not clear whether we want to support
such a usecase.

[...]
> > > in http://lkml.kernel.org/r/20161218163727.GC8440@dhcp22.suse.cz .
> > > Indeed that trace is a __GFP_DIRECT_RECLAIM and it might not be blocking
> > > other workqueue items which a regular I/O depend on, I think there are
> > > !__GFP_DIRECT_RECLAIM memory allocation requests for issuing SCSI commands
> > > which could potentially start failing due to helping GFP_NOFS | __GFP_NOFAIL
> > > allocations with memory reserves. If a SCSI disk I/O request fails due to
> > > GFP_ATOMIC memory allocation failures because we allow a FS I/O request to
> > > use memory reserves, it adds a new problem.
> > 
> > Do you have any example of such a request? Anything that requires
> > a forward progress during IO should be using mempools otherwise it
> > is broken pretty much by design already. Also IO depending on NOFS
> > allocations sounds pretty much broken already. So I suspect the above
> > reasoning is just bogus.
> 
> You are missing my point. My question is "who needs memory reserves".
> I'm not saying that disk I/O depends on GFP_NOFS allocation. I'm worrying
> that [PATCH 3/3] consumes memory reserves when disk I/O also depends on
> memory reserves.
> 
> My understanding is that when accessing SCSI disks, SCSI protocol is used.
> SCSI driver allocates memory at runtime for using SCSI protocol using
> GFP_ATOMIC. And GFP_ATOMIC uses some of memory reserves. But [PATCH 3/3]
> also uses memory reserves. If memory reserves are consumed by [PATCH 3/3]
> to the level where GFP_ATOMIC cannot succeed, I think it causes troubles.

Yes and GFP_ATOMIC will have a deeper access to memory reserves than what
we are giving access to in patch 3. There is difference between
ALLOC_HARDER and ALLOC_HIGH. This is described in the changelog. Sure
GFP_NOFAIL will eat into part of the reserves which GFP_ATOMIC (aka
GFP_HIGH) could have used but a) this shouldn't happen unless we are
really getting out of memory and b) it should help other presumably
important allocations (why would they be GFP_NOFAIL otherwise right?).
So it is not just a free ticket to a scarce resource and IMHO it is
justified.

> I'm unable to obtain nice backtraces, but I think we can confirm that
> there are GFP_ATOMIC allocations (e.g. sg_alloc_table_chained() calls
> __sg_alloc_table(GFP_ATOMIC)) when we are using SCSI disks.

How are those blocking further progress? Failing atomic allocations are
nothing to lose sleep over. They cannot be, pretty by definition, relied
on to make a further progress.

[...]

I am _really_ getting tired of this discussion. You are making wrong or
unfounded claims again and again. I have no idea what are you trying to
achieve here but I simply do not see any sense in continuing in this
discussion.
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1550858

Michal Hocko wrote:
> On Tue 03-01-17 23:38:30, 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."
> 
> Stop this! Seriously... This is just wasting time...

You are free to ignore me. But

> 
>  * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
>  *   return NULL when direct reclaim and memory compaction have failed to allow
>  *   the allocation to succeed.  The OOM killer is not called with the current
>  *   implementation.
> 
>  * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
>  *   cannot handle allocation failures. New users should be evaluated carefully
>  *   (and the flag should be used only when there is no reasonable failure
>  *   policy) but it is definitely preferable to use the flag rather than
>  *   opencode endless loop around allocator.
> 
> Can you see how the two are asking for opposite behavior?  Asking for
> not retrying for ever and not failing and rather retrying for ever
> simply doesn't make any sense in any reasonable universe I can think
> of. Therefore I think that it is fair to say that behavior is undefined
> when both are specified.

I consider that I'm using __GFP_NORETRY as a mean to avoid calling the OOM
killer rather than avoid retrying indefinitely. Therefore, I want

  __GFP_NOOOMKILL: The VM implementation must not call the OOM killer when
  direct reclaim and memory compaction have failed to allow the allocation
  to succeed.

and __GFP_NOOOMKILL | __GFP_NOFAIL makes sense.

Technically PATCH 1/3 allows __GFP_NOOOMKILL | __GFP_NOFAIL emulation
via __GFP_NOFAIL | __GFP_NOFAIL. If you don't like such emulation,
I welcome __GFP_NOOOMKILL.

> 
> Considering there are _no_ users which would do that any further
> discussion about this is just pointless and I will not respond to any
> further emails in this direction.
> 
> This is just ridiculous!

Regardless of whether we define __GFP_NOOOMKILL, I wonder we need PATCH 2/3 now
because currently premature OOM killer invocation due to !__GFP_FS && __GFP_NOFAIL
is a prophetical problem. We can consider PATCH 2/3 (or __GFP_NOOOMKILL) when
someone reported OOM killer invocation via !__GFP_FS && __GFP_NOFAIL and
confirmed that the memory counter says premature enough to suppress the OOM
killer invocation.

> 
> [...]
> >  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;
> > +	}
> 
> No there are simply no users of this and even if had one which would be
> legitimate it wouldn't be as simple as this. vmalloc _doesn't_ support
> GFP_NOFAIL and it would be really non-trivial to implement it. If for
> nothing else there are unconditional GFP_KERNEL allocations in some
> vmalloc paths (which is btw. the reason why vmalloc is not GFP_NOFS
> unsafe). It would take much more to add the non-failing semantic. And I
> see _no_ reason to argue with this possibility when a) there is no such
> user currently and b) it is even not clear whether we want to support
> such a usecase.

I didn't know vmalloc() doesn't support GFP_NOFAIL.
Anyway, we don't need to make such change now because of a).

> 
> [...]
> > > > in http://lkml.kernel.org/r/20161218163727.GC8440@dhcp22.suse.cz .
> > > > Indeed that trace is a __GFP_DIRECT_RECLAIM and it might not be blocking
> > > > other workqueue items which a regular I/O depend on, I think there are
> > > > !__GFP_DIRECT_RECLAIM memory allocation requests for issuing SCSI commands
> > > > which could potentially start failing due to helping GFP_NOFS | __GFP_NOFAIL
> > > > allocations with memory reserves. If a SCSI disk I/O request fails due to
> > > > GFP_ATOMIC memory allocation failures because we allow a FS I/O request to
> > > > use memory reserves, it adds a new problem.
> > > 
> > > Do you have any example of such a request? Anything that requires
> > > a forward progress during IO should be using mempools otherwise it
> > > is broken pretty much by design already. Also IO depending on NOFS
> > > allocations sounds pretty much broken already. So I suspect the above
> > > reasoning is just bogus.
> > 
> > You are missing my point. My question is "who needs memory reserves".
> > I'm not saying that disk I/O depends on GFP_NOFS allocation. I'm worrying
> > that [PATCH 3/3] consumes memory reserves when disk I/O also depends on
> > memory reserves.
> > 
> > My understanding is that when accessing SCSI disks, SCSI protocol is used.
> > SCSI driver allocates memory at runtime for using SCSI protocol using
> > GFP_ATOMIC. And GFP_ATOMIC uses some of memory reserves. But [PATCH 3/3]
> > also uses memory reserves. If memory reserves are consumed by [PATCH 3/3]
> > to the level where GFP_ATOMIC cannot succeed, I think it causes troubles.
> 
> Yes and GFP_ATOMIC will have a deeper access to memory reserves than what
> we are giving access to in patch 3. There is difference between
> ALLOC_HARDER and ALLOC_HIGH. This is described in the changelog. Sure
> GFP_NOFAIL will eat into part of the reserves which GFP_ATOMIC (aka
> GFP_HIGH) could have used but a) this shouldn't happen unless we are
> really getting out of memory and b) it should help other presumably
> important allocations (why would they be GFP_NOFAIL otherwise right?).
> So it is not just a free ticket to a scarce resource and IMHO it is
> justified.

If you try to allow !__GFP_FS allocations to fail rather than retry,
someone might start scattering __GFP_NOFAIL regardless of importance.

> 
> > I'm unable to obtain nice backtraces, but I think we can confirm that
> > there are GFP_ATOMIC allocations (e.g. sg_alloc_table_chained() calls
> > __sg_alloc_table(GFP_ATOMIC)) when we are using SCSI disks.
> 
> How are those blocking further progress? Failing atomic allocations are
> nothing to lose sleep over. They cannot be, pretty by definition, relied
> on to make a further progress.

So, regarding simple SCSI disk case, it is guaranteed that disk I/O request
can recover from transient failures (e.g. timeout?) and complete unless
fatal failures (e.g. hardware out of order?) occur, isn't it? Then,
PATCH 3/3 would be helpful for this case.

What about other cases, such as loopback devices ( /dev/loopX ) and/or
networking storage? Are they also guaranteed that I/O requests never be
blocked on memory allocation requests which are not allowed to access
memory reserves? If yes, PATCH 3/3 would be helpful. If no, I think
what we need is a mechanism to propagate allowing access to memory
reserves similar to scope GFP_NOFS API.

> 
> [...]
> 
> I am _really_ getting tired of this discussion. You are making wrong or
> unfounded claims again and again. I have no idea what are you trying to
> achieve here but I simply do not see any sense in continuing in this
> discussion.
> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1550913

On Wed 04-01-17 23:22:24, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 03-01-17 23:38:30, 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."
> > 
> > Stop this! Seriously... This is just wasting time...
> 
> You are free to ignore me. But

my last reply in this subthread

> >  * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
> >  *   return NULL when direct reclaim and memory compaction have failed to allow
> >  *   the allocation to succeed.  The OOM killer is not called with the current
> >  *   implementation.
> > 
> >  * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
> >  *   cannot handle allocation failures. New users should be evaluated carefully
> >  *   (and the flag should be used only when there is no reasonable failure
> >  *   policy) but it is definitely preferable to use the flag rather than
> >  *   opencode endless loop around allocator.
> > 
> > Can you see how the two are asking for opposite behavior?  Asking for
> > not retrying for ever and not failing and rather retrying for ever
> > simply doesn't make any sense in any reasonable universe I can think
> > of. Therefore I think that it is fair to say that behavior is undefined
> > when both are specified.
> 
> I consider that I'm using __GFP_NORETRY as a mean to avoid calling the OOM
> killer rather than avoid retrying indefinitely. Therefore, I want

This would be an abuse. Not invoking the oom killer is an implementation
detail and can change in the future. This is documented.

>   __GFP_NOOOMKILL: The VM implementation must not call the OOM killer when
>   direct reclaim and memory compaction have failed to allow the allocation
>   to succeed.

I am not going to give such a flag to users. The longer I am looking
into how those flags are used the more I am convinced that they are very
often wrong when trying to be too clever. Decision whether to trigger
OOM killer or not is the MM internal thing and _no code_ outside the MM
proper has any word into it.
 
> and __GFP_NOOOMKILL | __GFP_NOFAIL makes sense.

and this example just shows why I think that my cautiousness is
justified...

> Technically PATCH 1/3 allows __GFP_NOOOMKILL | __GFP_NOFAIL emulation
> via __GFP_NOFAIL | __GFP_NOFAIL. If you don't like such emulation,
> I welcome __GFP_NOOOMKILL.
> 
> > 
> > Considering there are _no_ users which would do that any further
> > discussion about this is just pointless and I will not respond to any
> > further emails in this direction.
> > 
> > This is just ridiculous!
> 
> Regardless of whether we define __GFP_NOOOMKILL, I wonder we need PATCH 2/3 now
> because currently premature OOM killer invocation due to !__GFP_FS && __GFP_NOFAIL
> is a prophetical problem. We can consider PATCH 2/3 (or __GFP_NOOOMKILL) when
> someone reported OOM killer invocation via !__GFP_FS && __GFP_NOFAIL and
> confirmed that the memory counter says premature enough to suppress the OOM
> killer invocation.

Again. GFP_NOFS should behave consistently regardless GFP_NOFAIL. The
mere fact that the opencoded endless loop around GFP_NOFS behaves
differently is something to raise a red flag. I want to fix that. So no,
I really do not want to keep the status quo.

[...]
> > > I'm unable to obtain nice backtraces, but I think we can confirm that
> > > there are GFP_ATOMIC allocations (e.g. sg_alloc_table_chained() calls
> > > __sg_alloc_table(GFP_ATOMIC)) when we are using SCSI disks.
> > 
> > How are those blocking further progress? Failing atomic allocations are
> > nothing to lose sleep over. They cannot be, pretty by definition, relied
> > on to make a further progress.
> 
> So, regarding simple SCSI disk case, it is guaranteed that disk I/O request
> can recover from transient failures (e.g. timeout?) and complete unless
> fatal failures (e.g. hardware out of order?) occur, isn't it? Then,
> PATCH 3/3 would be helpful for this case.
> 
> What about other cases, such as loopback devices ( /dev/loopX ) and/or
> networking storage? Are they also guaranteed that I/O requests never be
> blocked on memory allocation requests which are not allowed to access
> memory reserves? If yes, PATCH 3/3 would be helpful. If no, I think
> what we need is a mechanism to propagate allowing access to memory
> reserves similar to scope GFP_NOFS API.

Again, which cannot recover from GFP_ATOMIC requests is broken by
definition.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1551887

Michal Hocko wrote:
> > > Stop this! Seriously... This is just wasting time...
> > 
> > You are free to ignore me. But
> 
> my last reply in this subthread
> 

OK. You can ignore me; I just won't give my Acked-by: or Reviewed-by: to this series.

My understanding is that we changed to tolerate __GFP_NOFAIL usage because
allocation failure leads to unacceptable side effect (e.g. remounting
read-only, kernel panic) rather than allocation helps reclaiming memory.

  commit 647757197cd34fae ("mm: clarify __GFP_NOFAIL deprecation status")
  commit 277fb5fc177dc467 ("btrfs: use __GFP_NOFAIL in alloc_btrfs_bio")

I don't know whether __GFP_NOFAIL users are using __GFP_NOFAIL based on
whether it helps reclaiming memory rather than whether allocation failure
leads to unacceptable side effect, if we allow access to memory reserves
based on __GFP_NOFAIL.

  commit 7444a072c387a93e ("ext4: replace open coded nofail allocation in ext4_free_blocks()")
  commit adb7ef600cc9d9d1 ("ext4: use __GFP_NOFAIL in ext4_free_blocks()")
  commit c9af28fdd44922a6 ("ext4 crypto: don't let data integrity writebacks fail with ENOMEM")
  commit b32e4482aadfd132 ("fscrypto: don't let data integrity writebacks fail with ENOMEM")
  commit 80c545055dc7c1f7 ("f2fs: use __GFP_NOFAIL to avoid infinite loop")

If __GFP_NOFAIL users are using __GFP_NOFAIL based on whether allocation failure
leads to unacceptable side effect, allowing access to memory reserves based on
__GFP_NOFAIL might not help reclaiming memory; something like scope GFP_NOFS API
will be needed.

Anyway, I suggest merging description update shown below into this series and
getting confirmation from all existing __GFP_NOFAIL users. If all existing
__GFP_NOFAIL users are OK with this series (in other words, informed the risk
caused by this series), I'm also OK with this series.

--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -135,16 +135,24 @@
  * __GFP_REPEAT: Try hard to allocate the memory, but the allocation attempt
  *   _might_ fail.  This depends upon the particular VM implementation.
  *
- * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
- *   cannot handle allocation failures. New users should be evaluated carefully
- *   (and the flag should be used only when there is no reasonable failure
- *   policy) but it is definitely preferable to use the flag rather than
- *   opencode endless loop around allocator.
- *
- * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
- *   return NULL when direct reclaim and memory compaction have failed to allow
- *   the allocation to succeed.  The OOM killer is not called with the current
- *   implementation.
+ * __GFP_NOFAIL: The VM implementation must not give up even after direct
+ *   reclaim and memory compaction have failed to allow the allocation to
+ *   succeed. Note that since the OOM killer is not called with the current
+ *   implementation when direct reclaim and memory compaction have failed to
+ *   allow the allocation to succeed unless __GFP_FS is also used (and some
+ *   other conditions are met), e.g. GFP_NOFS | __GFP_NOFAIL allocation has
+ *   possibility of lockup. To reduce the possibility of lockup, __GFP_HIGH is
+ *   implicitly granted by the current implementation if __GFP_NOFAIL is used.
+ *   New users of __GFP_NOFAIL should be evaluated carefully (and __GFP_NOFAIL
+ *   should be used only when there is no reasonable failure policy) but it is
+ *   definitely preferable to use __GFP_NOFAIL rather than opencode endless
+ *   loop around allocator, for a stall detection check inside allocator will
+ *   likely be able to emit possible lockup warnings unless __GFP_NOWARN is
+ *   also used.
+ *
+ * __GFP_NORETRY: The VM implementation must give up as soon as direct reclaim
+ *   and memory compaction have failed to allow the allocation to succeed.
+ *   Therefore, __GFP_NORETRY cannot be used with __GFP_NOFAIL.
  */
 #define __GFP_IO	((__force gfp_t)___GFP_IO)
 #define __GFP_FS	((__force gfp_t)___GFP_FS)

I do not like "mm, oom: get rid of TIF_MEMDIE" series because you have not
gotten confirmation from all users who might be affected (e.g. start failing
inside do_exit() which currently do not fail) by that series. If you clarify
possible side effects and get confirmation from affected users (in case some
users might need to add __GFP_NOFAIL), I will be OK with that series as well.

[toc] | [prev] | [next] | [standalone]

#1551928

On Thu 05-01-17 19:50:23, Tetsuo Handa wrote:
[...]
> Anyway, I suggest merging description update shown below into this series and
> getting confirmation from all existing __GFP_NOFAIL users. If all existing
> __GFP_NOFAIL users are OK with this series (in other words, informed the risk
> caused by this series), I'm also OK with this series.
> 
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -135,16 +135,24 @@
>   * __GFP_REPEAT: Try hard to allocate the memory, but the allocation attempt
>   *   _might_ fail.  This depends upon the particular VM implementation.
>   *
> - * __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
> - *   cannot handle allocation failures. New users should be evaluated carefully
> - *   (and the flag should be used only when there is no reasonable failure
> - *   policy) but it is definitely preferable to use the flag rather than
> - *   opencode endless loop around allocator.
> - *
> - * __GFP_NORETRY: The VM implementation must not retry indefinitely and will
> - *   return NULL when direct reclaim and memory compaction have failed to allow
> - *   the allocation to succeed.  The OOM killer is not called with the current
> - *   implementation.
> + * __GFP_NOFAIL: The VM implementation must not give up even after direct
> + *   reclaim and memory compaction have failed to allow the allocation to
> + *   succeed. Note that since the OOM killer is not called with the current
> + *   implementation when direct reclaim and memory compaction have failed to
> + *   allow the allocation to succeed unless __GFP_FS is also used (and some
> + *   other conditions are met), e.g. GFP_NOFS | __GFP_NOFAIL allocation has
> + *   possibility of lockup. To reduce the possibility of lockup, __GFP_HIGH is
> + *   implicitly granted by the current implementation if __GFP_NOFAIL is used.
> + *   New users of __GFP_NOFAIL should be evaluated carefully (and __GFP_NOFAIL
> + *   should be used only when there is no reasonable failure policy) but it is
> + *   definitely preferable to use __GFP_NOFAIL rather than opencode endless
> + *   loop around allocator, for a stall detection check inside allocator will
> + *   likely be able to emit possible lockup warnings unless __GFP_NOWARN is
> + *   also used.

This is both wrong and unnecessarily describing implementation details.
Non-failing allocation which must not give up can lockup pretty much by
definition. IMHO the current description is sufficient.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web