* [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB @ 2021-10-05 15:31 Jens Axboe 2021-10-05 21:18 ` Andrew Morton 2021-10-07 15:32 ` Vlastimil Babka 0 siblings, 2 replies; 6+ messages in thread From: Jens Axboe @ 2021-10-05 15:31 UTC (permalink / raw) To: LKML, Linux Memory Management List, Andrew Morton Allocations can be a very hot path, and this out-of-line function call is noticeable. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h index e525f6957c49..3128d2c8b3b4 100644 --- a/include/linux/fault-inject.h +++ b/include/linux/fault-inject.h @@ -64,8 +64,8 @@ static inline struct dentry *fault_create_debugfs_attr(const char *name, struct kmem_cache; -int should_failslab(struct kmem_cache *s, gfp_t gfpflags); #ifdef CONFIG_FAILSLAB +int should_failslab(struct kmem_cache *s, gfp_t gfpflags); extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); #else static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) diff --git a/mm/slab.h b/mm/slab.h index 58c01a34e5b8..92fd6fe01877 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -491,8 +491,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, might_alloc(flags); +#ifdef CONFIG_FAILSLAB if (should_failslab(s, flags)) return NULL; +#endif if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) return NULL; diff --git a/mm/slab_common.c b/mm/slab_common.c index ec2bb0beed75..c21bd447f237 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1323,6 +1323,7 @@ EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc_node); EXPORT_TRACEPOINT_SYMBOL(kfree); EXPORT_TRACEPOINT_SYMBOL(kmem_cache_free); +#ifdef CONFIG_FAILSLAB int should_failslab(struct kmem_cache *s, gfp_t gfpflags) { if (__should_failslab(s, gfpflags)) @@ -1330,3 +1331,4 @@ int should_failslab(struct kmem_cache *s, gfp_t gfpflags) return 0; } ALLOW_ERROR_INJECTION(should_failslab, ERRNO); +#endif -- Jens Axboe ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB 2021-10-05 15:31 [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB Jens Axboe @ 2021-10-05 21:18 ` Andrew Morton 2021-10-05 21:20 ` Jens Axboe 2021-10-07 15:32 ` Vlastimil Babka 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2021-10-05 21:18 UTC (permalink / raw) To: Jens Axboe; +Cc: LKML, Linux Memory Management List On Tue, 5 Oct 2021 09:31:43 -0600 Jens Axboe <axboe@kernel.dk> wrote: > Allocations can be a very hot path, and this out-of-line function > call is noticeable. > > --- a/include/linux/fault-inject.h > +++ b/include/linux/fault-inject.h > @@ -64,8 +64,8 @@ static inline struct dentry *fault_create_debugfs_attr(const char *name, > > struct kmem_cache; > > -int should_failslab(struct kmem_cache *s, gfp_t gfpflags); > #ifdef CONFIG_FAILSLAB > +int should_failslab(struct kmem_cache *s, gfp_t gfpflags); > extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); > #else > static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > diff --git a/mm/slab.h b/mm/slab.h > index 58c01a34e5b8..92fd6fe01877 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -491,8 +491,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > might_alloc(flags); > > +#ifdef CONFIG_FAILSLAB > if (should_failslab(s, flags)) > return NULL; > +#endif Can we avoid the ifdefs here? > > if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) > return NULL; > diff --git a/mm/slab_common.c b/mm/slab_common.c > index ec2bb0beed75..c21bd447f237 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1323,6 +1323,7 @@ EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc_node); > EXPORT_TRACEPOINT_SYMBOL(kfree); > EXPORT_TRACEPOINT_SYMBOL(kmem_cache_free); > > +#ifdef CONFIG_FAILSLAB > int should_failslab(struct kmem_cache *s, gfp_t gfpflags) > { > if (__should_failslab(s, gfpflags)) > @@ -1330,3 +1331,4 @@ int should_failslab(struct kmem_cache *s, gfp_t gfpflags) > return 0; > } > ALLOW_ERROR_INJECTION(should_failslab, ERRNO); > +#endif Like, --- a/include/linux/fault-inject.h~mm-dont-call-should_failslab-for-config_failslab-fix +++ a/include/linux/fault-inject.h @@ -68,6 +68,10 @@ struct kmem_cache; int should_failslab(struct kmem_cache *s, gfp_t gfpflags); extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); #else +static inline int should_failslab(struct kmem_cache *s, gfp_t gfpflags) +{ + return 0; +} static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) { return false; --- a/mm/slab.h~mm-dont-call-should_failslab-for-config_failslab-fix +++ a/mm/slab.h @@ -491,10 +491,8 @@ static inline struct kmem_cache *slab_pr might_alloc(flags); -#ifdef CONFIG_FAILSLAB if (should_failslab(s, flags)) return NULL; -#endif if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) return NULL; _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB 2021-10-05 21:18 ` Andrew Morton @ 2021-10-05 21:20 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2021-10-05 21:20 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, Linux Memory Management List On 10/5/21 3:18 PM, Andrew Morton wrote: > On Tue, 5 Oct 2021 09:31:43 -0600 Jens Axboe <axboe@kernel.dk> wrote: > >> Allocations can be a very hot path, and this out-of-line function >> call is noticeable. >> >> --- a/include/linux/fault-inject.h >> +++ b/include/linux/fault-inject.h >> @@ -64,8 +64,8 @@ static inline struct dentry *fault_create_debugfs_attr(const char *name, >> >> struct kmem_cache; >> >> -int should_failslab(struct kmem_cache *s, gfp_t gfpflags); >> #ifdef CONFIG_FAILSLAB >> +int should_failslab(struct kmem_cache *s, gfp_t gfpflags); >> extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); >> #else >> static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> diff --git a/mm/slab.h b/mm/slab.h >> index 58c01a34e5b8..92fd6fe01877 100644 >> --- a/mm/slab.h >> +++ b/mm/slab.h >> @@ -491,8 +491,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, >> >> might_alloc(flags); >> >> +#ifdef CONFIG_FAILSLAB >> if (should_failslab(s, flags)) >> return NULL; >> +#endif > > Can we avoid the ifdefs here? > >> >> if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) >> return NULL; >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index ec2bb0beed75..c21bd447f237 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -1323,6 +1323,7 @@ EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc_node); >> EXPORT_TRACEPOINT_SYMBOL(kfree); >> EXPORT_TRACEPOINT_SYMBOL(kmem_cache_free); >> >> +#ifdef CONFIG_FAILSLAB >> int should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> { >> if (__should_failslab(s, gfpflags)) >> @@ -1330,3 +1331,4 @@ int should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> return 0; >> } >> ALLOW_ERROR_INJECTION(should_failslab, ERRNO); >> +#endif > > Like, > > --- a/include/linux/fault-inject.h~mm-dont-call-should_failslab-for-config_failslab-fix > +++ a/include/linux/fault-inject.h > @@ -68,6 +68,10 @@ struct kmem_cache; > int should_failslab(struct kmem_cache *s, gfp_t gfpflags); > extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); > #else > +static inline int should_failslab(struct kmem_cache *s, gfp_t gfpflags) > +{ > + return 0; > +} > static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > { > return false; > --- a/mm/slab.h~mm-dont-call-should_failslab-for-config_failslab-fix > +++ a/mm/slab.h > @@ -491,10 +491,8 @@ static inline struct kmem_cache *slab_pr > > might_alloc(flags); > > -#ifdef CONFIG_FAILSLAB > if (should_failslab(s, flags)) > return NULL; > -#endif > > if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) > return NULL; > _ Yep, that'll work! -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB 2021-10-05 15:31 [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB Jens Axboe 2021-10-05 21:18 ` Andrew Morton @ 2021-10-07 15:32 ` Vlastimil Babka 2021-10-07 15:50 ` Jens Axboe 2021-10-07 16:36 ` Konstantin Ryabitsev 1 sibling, 2 replies; 6+ messages in thread From: Vlastimil Babka @ 2021-10-07 15:32 UTC (permalink / raw) To: Jens Axboe, LKML, Linux Memory Management List, Andrew Morton, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Jesper Dangaard Brouer, Christoph Lameter, David Rientjes, Pekka Enberg, Joonsoo Kim, Yonghong Song, John Fastabend, KP Singh, Howard McLauchlan Cc: bpf On 10/5/21 17:31, Jens Axboe wrote: > Allocations can be a very hot path, and this out-of-line function > call is noticeable. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> It used to be inline b4 (hi, Konstantin!) and then was converted to be like this intentionally :/ See 4f6923fbb352 ("mm: make should_failslab always available for fault injection") And now also kernel/bpf/verifier.c contains: BTF_ID(func, should_failslab) I think either your or Andrew's version will break this BTF_ID thing, at the very least. But I do strongly agree that putting unconditionally a non-inline call into slab allocator fastpath sucks. Can we make it so that bpf can only do these overrides when CONFIG_FAILSLAB is enabled? I don't know, perhaps putting this BTF_ID() in #ifdef as well, or providing a dummy that is always available (so that nothing breaks), but doesn't actually affect slab_pre_alloc_hook() unless CONFIG_FAILSLAB has been enabled? > --- > > diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h > index e525f6957c49..3128d2c8b3b4 100644 > --- a/include/linux/fault-inject.h > +++ b/include/linux/fault-inject.h > @@ -64,8 +64,8 @@ static inline struct dentry *fault_create_debugfs_attr(const char *name, > > struct kmem_cache; > > -int should_failslab(struct kmem_cache *s, gfp_t gfpflags); > #ifdef CONFIG_FAILSLAB > +int should_failslab(struct kmem_cache *s, gfp_t gfpflags); > extern bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags); > #else > static inline bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > diff --git a/mm/slab.h b/mm/slab.h > index 58c01a34e5b8..92fd6fe01877 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -491,8 +491,10 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > might_alloc(flags); > > +#ifdef CONFIG_FAILSLAB > if (should_failslab(s, flags)) > return NULL; > +#endif > > if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags)) > return NULL; > diff --git a/mm/slab_common.c b/mm/slab_common.c > index ec2bb0beed75..c21bd447f237 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1323,6 +1323,7 @@ EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc_node); > EXPORT_TRACEPOINT_SYMBOL(kfree); > EXPORT_TRACEPOINT_SYMBOL(kmem_cache_free); > > +#ifdef CONFIG_FAILSLAB > int should_failslab(struct kmem_cache *s, gfp_t gfpflags) > { > if (__should_failslab(s, gfpflags)) > @@ -1330,3 +1331,4 @@ int should_failslab(struct kmem_cache *s, gfp_t gfpflags) > return 0; > } > ALLOW_ERROR_INJECTION(should_failslab, ERRNO); > +#endif > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB 2021-10-07 15:32 ` Vlastimil Babka @ 2021-10-07 15:50 ` Jens Axboe 2021-10-07 16:36 ` Konstantin Ryabitsev 1 sibling, 0 replies; 6+ messages in thread From: Jens Axboe @ 2021-10-07 15:50 UTC (permalink / raw) To: Vlastimil Babka, LKML, Linux Memory Management List, Andrew Morton, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Jesper Dangaard Brouer, Christoph Lameter, David Rientjes, Pekka Enberg, Joonsoo Kim, Yonghong Song, John Fastabend, KP Singh, Howard McLauchlan Cc: bpf On 10/7/21 9:32 AM, Vlastimil Babka wrote: > On 10/5/21 17:31, Jens Axboe wrote: >> Allocations can be a very hot path, and this out-of-line function >> call is noticeable. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > It used to be inline b4 (hi, Konstantin!) and then was converted to be like > this intentionally :/ > > See 4f6923fbb352 ("mm: make should_failslab always available for fault > injection") > > And now also kernel/bpf/verifier.c contains: > BTF_ID(func, should_failslab) > > I think either your or Andrew's version will break this BTF_ID thing, at the > very least. > > But I do strongly agree that putting unconditionally a non-inline call into > slab allocator fastpath sucks. Can we make it so that bpf can only do these > overrides when CONFIG_FAILSLAB is enabled? > I don't know, perhaps putting this BTF_ID() in #ifdef as well, or providing > a dummy that is always available (so that nothing breaks), but doesn't > actually affect slab_pre_alloc_hook() unless CONFIG_FAILSLAB has been enabled? That seems to be the right approach, limiting it on it actually being enabled and a function call. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB 2021-10-07 15:32 ` Vlastimil Babka 2021-10-07 15:50 ` Jens Axboe @ 2021-10-07 16:36 ` Konstantin Ryabitsev 1 sibling, 0 replies; 6+ messages in thread From: Konstantin Ryabitsev @ 2021-10-07 16:36 UTC (permalink / raw) To: Vlastimil Babka Cc: Jens Axboe, LKML, Linux Memory Management List, Andrew Morton, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Jesper Dangaard Brouer, Christoph Lameter, David Rientjes, Pekka Enberg, Joonsoo Kim, Yonghong Song, John Fastabend, KP Singh, Howard McLauchlan, bpf On Thu, Oct 07, 2021 at 05:32:52PM +0200, Vlastimil Babka wrote: > On 10/5/21 17:31, Jens Axboe wrote: > > Allocations can be a very hot path, and this out-of-line function > > call is noticeable. > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > It used to be inline b4 (hi, Konstantin!) Congratulations, you made me look. :) -K ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-07 16:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-05 15:31 [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB Jens Axboe 2021-10-05 21:18 ` Andrew Morton 2021-10-05 21:20 ` Jens Axboe 2021-10-07 15:32 ` Vlastimil Babka 2021-10-07 15:50 ` Jens Axboe 2021-10-07 16:36 ` Konstantin Ryabitsev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).