linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [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).