| Message ID | 20190520044951.248096-1-drinkcat@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/failslab: By default, do not fail allocations with direct reclaim only | expand |
2019年5月20日(月) 13:49 Nicolas Boichat <drinkcat@chromium.org>: > > When failslab was originally written, the intention of the > "ignore-gfp-wait" flag default value ("N") was to fail > GFP_ATOMIC allocations. Those were defined as (__GFP_HIGH), > and the code would test for __GFP_WAIT (0x10u). > > However, since then, __GFP_WAIT was replaced by __GFP_RECLAIM > (___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM), and GFP_ATOMIC is > now defined as (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM). > > This means that when the flag is false, almost no allocation > ever fails (as even GFP_ATOMIC allocations contain > __GFP_KSWAPD_RECLAIM). > > Restore the original intent of the code, by ignoring calls > that directly reclaim only (___GFP_DIRECT_RECLAIM), and thus, > failing GFP_ATOMIC calls again by default. > > Fixes: 71baba4b92dc1fa1 ("mm, page_alloc: rename __GFP_WAIT to __GFP_RECLAIM") > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> Good catch. Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > mm/failslab.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/failslab.c b/mm/failslab.c > index ec5aad211c5be97..33efcb60e633c0a 100644 > --- a/mm/failslab.c > +++ b/mm/failslab.c > @@ -23,7 +23,8 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > if (gfpflags & __GFP_NOFAIL) > return false; > > - if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM)) > + if (failslab.ignore_gfp_reclaim && > + (gfpflags & ___GFP_DIRECT_RECLAIM)) > return false; Should we use __GFP_DIRECT_RECLAIM instead of ___GFP_DIRECT_RECLAIM? Because I found the following comment in gfp.h /* Plain integer GFP bitmasks. Do not use this directly. */
On Tue, 21 May 2019, Akinobu Mita wrote: > > When failslab was originally written, the intention of the > > "ignore-gfp-wait" flag default value ("N") was to fail > > GFP_ATOMIC allocations. Those were defined as (__GFP_HIGH), > > and the code would test for __GFP_WAIT (0x10u). > > > > However, since then, __GFP_WAIT was replaced by __GFP_RECLAIM > > (___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM), and GFP_ATOMIC is > > now defined as (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM). > > > > This means that when the flag is false, almost no allocation > > ever fails (as even GFP_ATOMIC allocations contain > > __GFP_KSWAPD_RECLAIM). > > > > Restore the original intent of the code, by ignoring calls > > that directly reclaim only (___GFP_DIRECT_RECLAIM), and thus, > > failing GFP_ATOMIC calls again by default. > > > > Fixes: 71baba4b92dc1fa1 ("mm, page_alloc: rename __GFP_WAIT to __GFP_RECLAIM") > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > Good catch. > > Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com> > > > --- > > mm/failslab.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/mm/failslab.c b/mm/failslab.c > > index ec5aad211c5be97..33efcb60e633c0a 100644 > > --- a/mm/failslab.c > > +++ b/mm/failslab.c > > @@ -23,7 +23,8 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > > if (gfpflags & __GFP_NOFAIL) > > return false; > > > > - if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM)) > > + if (failslab.ignore_gfp_reclaim && > > + (gfpflags & ___GFP_DIRECT_RECLAIM)) > > return false; > > Should we use __GFP_DIRECT_RECLAIM instead of ___GFP_DIRECT_RECLAIM? > Because I found the following comment in gfp.h > > /* Plain integer GFP bitmasks. Do not use this directly. */ > Yes, we should use the two underscore version instead of the three. Nicolas, after that's fixed up, feel free to add Acked-by: David Rientjes <rientjes@google.com>. Thanks!
On Tue, May 21, 2019 at 12:29 AM Akinobu Mita <akinobu.mita@gmail.com> wrote: > > 2019年5月20日(月) 13:49 Nicolas Boichat <drinkcat@chromium.org>: > > > > When failslab was originally written, the intention of the > > "ignore-gfp-wait" flag default value ("N") was to fail > > GFP_ATOMIC allocations. Those were defined as (__GFP_HIGH), > > and the code would test for __GFP_WAIT (0x10u). > > > > However, since then, __GFP_WAIT was replaced by __GFP_RECLAIM > > (___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM), and GFP_ATOMIC is > > now defined as (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM). > > > > This means that when the flag is false, almost no allocation > > ever fails (as even GFP_ATOMIC allocations contain > > __GFP_KSWAPD_RECLAIM). > > > > Restore the original intent of the code, by ignoring calls > > that directly reclaim only (___GFP_DIRECT_RECLAIM), and thus, > > failing GFP_ATOMIC calls again by default. > > > > Fixes: 71baba4b92dc1fa1 ("mm, page_alloc: rename __GFP_WAIT to __GFP_RECLAIM") > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > Good catch. > > Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com> > > > --- > > mm/failslab.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/mm/failslab.c b/mm/failslab.c > > index ec5aad211c5be97..33efcb60e633c0a 100644 > > --- a/mm/failslab.c > > +++ b/mm/failslab.c > > @@ -23,7 +23,8 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > > if (gfpflags & __GFP_NOFAIL) > > return false; > > > > - if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM)) > > + if (failslab.ignore_gfp_reclaim && > > + (gfpflags & ___GFP_DIRECT_RECLAIM)) > > return false; > > Should we use __GFP_DIRECT_RECLAIM instead of ___GFP_DIRECT_RECLAIM? > Because I found the following comment in gfp.h > > /* Plain integer GFP bitmasks. Do not use this directly. */ Oh, nice catch. I must say I had no idea I was using the 3-underscore version, hard to tell them apart depending on the font. I'll send a v2 with both your tags right away. Thanks,
On Tue, May 21, 2019 at 4:01 PM kbuild test robot <lkp@intel.com> wrote: > sparse warnings: (new ones prefixed by >>) > > >> mm/failslab.c:27:26: sparse: sparse: restricted gfp_t degrades to integer > 26 if (failslab.ignore_gfp_reclaim && > > 27 (gfpflags & ___GFP_DIRECT_RECLAIM)) That was for v1, fixed in v2 already.
diff --git a/mm/failslab.c b/mm/failslab.c index ec5aad211c5be97..33efcb60e633c0a 100644 --- a/mm/failslab.c +++ b/mm/failslab.c @@ -23,7 +23,8 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) if (gfpflags & __GFP_NOFAIL) return false; - if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM)) + if (failslab.ignore_gfp_reclaim && + (gfpflags & ___GFP_DIRECT_RECLAIM)) return false; if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
When failslab was originally written, the intention of the "ignore-gfp-wait" flag default value ("N") was to fail GFP_ATOMIC allocations. Those were defined as (__GFP_HIGH), and the code would test for __GFP_WAIT (0x10u). However, since then, __GFP_WAIT was replaced by __GFP_RECLAIM (___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM), and GFP_ATOMIC is now defined as (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM). This means that when the flag is false, almost no allocation ever fails (as even GFP_ATOMIC allocations contain __GFP_KSWAPD_RECLAIM). Restore the original intent of the code, by ignoring calls that directly reclaim only (___GFP_DIRECT_RECLAIM), and thus, failing GFP_ATOMIC calls again by default. Fixes: 71baba4b92dc1fa1 ("mm, page_alloc: rename __GFP_WAIT to __GFP_RECLAIM") Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> --- mm/failslab.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)