Re: [PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
On Tue, May 21, 2019 at 4:01 PM kbuild test robot 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.
Re: [PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
Hi Nicolas, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.2-rc1 next-20190520] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nicolas-Boichat/mm-failslab-By-default-do-not-fail-allocations-with-direct-reclaim-only/20190521-045221 reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot sparse warnings: (new ones prefixed by >>) >> mm/failslab.c:27:26: sparse: sparse: restricted gfp_t degrades to integer vim +27 mm/failslab.c 16 17 bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) 18 { 19 /* No fault-injection for bootstrap cache */ 20 if (unlikely(s == kmem_cache)) 21 return false; 22 23 if (gfpflags & __GFP_NOFAIL) 24 return false; 25 26 if (failslab.ignore_gfp_reclaim && > 27 (gfpflags & ___GFP_DIRECT_RECLAIM)) 28 return false; 29 30 if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) 31 return false; 32 33 return should_fail(, s->object_size); 34 } 35 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
On Tue, May 21, 2019 at 12:29 AM Akinobu Mita wrote: > > 2019年5月20日(月) 13:49 Nicolas Boichat : > > > > 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 > > Good catch. > > Reviewed-by: Akinobu Mita > > > --- > > 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,
Re: [PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
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 > > Good catch. > > Reviewed-by: Akinobu Mita > > > --- > > 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 . Thanks!
Re: [PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
2019年5月20日(月) 13:49 Nicolas Boichat : > > 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 Good catch. Reviewed-by: Akinobu Mita > --- > 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. */
[PATCH] mm/failslab: By default, do not fail allocations with direct reclaim only
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 --- 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; if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) -- 2.21.0.1020.gf2820cf01a-goog