Message ID | e01e5e40-692a-519c-4cba-e3331f173c82@kernel.dk (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | mm: don't call should_failslab() for !CONFIG_FAILSLAB | expand |
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;
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!
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 >
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.
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
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
Allocations can be a very hot path, and this out-of-line function call is noticeable. Signed-off-by: Jens Axboe <axboe@kernel.dk> ---