Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB

From: Vlastimil Babka <vbabka@suse.cz>
To: Jens Axboe <axboe@kernel.dk>, LKML <linux-kernel@vger.kernel.org>,
	Linux Memory Management List <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	Christoph Lameter <cl@linux.com>,
	David Rientjes <rientjes@google.com>,
	Pekka Enberg <penberg@kernel.org>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Howard McLauchlan <hmclauchlan@fb.com>
Cc: bpf@vger.kernel.org
Subject: Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB
Date: Thu, 7 Oct 2021 17:32:52 +0200	[thread overview]
Message-ID: <2dfc6273-6cdd-f4f5-bed9-400873ac9152@suse.cz> (raw)
In-Reply-To: <e01e5e40-692a-519c-4cba-e3331f173c82@kernel.dk>

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
> 