Received-SPF: pass (google.com: domain of linux-kernel+bounces-70549-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 18 Feb 2024 11:25:29 -0800 (PST)
From: David Rientjes <rientjes@google.com>
To: Jianfeng Wang <jianfeng.w.wang@oracle.com>
cc: cl@linux.com, penberg@kernel.org, iamjoonsoo.kim@lge.com, 
    akpm@linux-foundation.org, vbabka@suse.cz, roman.gushchin@linux.dev, 
    42.hyeyoo@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] slub: avoid scanning all partial slabs in
 get_slabinfo()
In-Reply-To: <20240215211457.32172-1-jianfeng.w.wang@oracle.com>
Message-ID: <6b58d81f-8e8f-3732-a5d4-40eece75013b@google.com>
References: <20240215211457.32172-1-jianfeng.w.wang@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Thu, 15 Feb 2024, Jianfeng Wang wrote:

> When reading "/proc/slabinfo", the kernel needs to report the number of
> free objects for each kmem_cache. The current implementation relies on
> count_partial() that counts the number of free objects by scanning each
> kmem_cache_node's partial slab list and summing free objects from all
> partial slabs in the list. This process must hold per kmem_cache_node
> spinlock and disable IRQ. Consequently, it can block slab allocation
> requests on other CPU cores and cause timeouts for network devices etc.,
> if the partial slab list is long. In production, even NMI watchdog can
> be triggered because some slab caches have a long partial list: e.g.,
> for "buffer_head", the number of partial slabs was observed to be ~1M
> in one kmem_cache_node. This problem was also observed by several
> others [1-2] in the past.
> 
> The fix is to maintain a counter of free objects for each kmem_cache.
> Then, in get_slabinfo(), use the counter rather than count_partial()
> when reporting the number of free objects for a slab cache. per-cpu
> counter is used to minimize atomic or lock operation.
> 
> Benchmark: run hackbench on a dual-socket 72-CPU bare metal machine
> with 256 GB memory and Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.3 GHz.
> The command is "hackbench 18 thread 20000". Each group gets 10 runs.
> 

This seems particularly intrusive for the common path to optimize for 
reading of /proc/slabinfo, and that's shown in the benchmark result.

Could you discuss the /proc/slabinfo usage model a bit?  It's not clear if 
this is being continuously read, or whether even a single read in 
isolation is problematic.

That said, optimizing for reading /proc/slabinfo at the cost of runtime 
performance degradation doesn't sound like the right trade-off.

> Results:
> - Mainline:
> 21.0381 +- 0.0325 seconds time elapsed  ( +-  0.15% )
> - Mainline w/ this patch:
> 21.1878 +- 0.0239 seconds time elapsed  ( +-  0.11% )
> 
> [1] https://lore.kernel.org/linux-mm/
> alpine.DEB.2.21.2003031602460.1537@www.lameter.com/T/
> [2] https://lore.kernel.org/lkml/
> alpine.DEB.2.22.394.2008071258020.55871@www.lameter.com/T/
> 
> Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com>
> ---
>  mm/slab.h |  4 ++++
>  mm/slub.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 54deeb0428c6..a0e7672ba648 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -11,6 +11,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/kfence.h>
>  #include <linux/kasan.h>
> +#include <linux/percpu_counter.h>
>  
>  /*
>   * Internal slab definitions
> @@ -277,6 +278,9 @@ struct kmem_cache {
>  	unsigned int red_left_pad;	/* Left redzone padding size */
>  	const char *name;		/* Name (only for display!) */
>  	struct list_head list;		/* List of slab caches */
> +#ifdef CONFIG_SLUB_DEBUG
> +	struct percpu_counter free_objects;
> +#endif
>  #ifdef CONFIG_SYSFS
>  	struct kobject kobj;		/* For sysfs */
>  #endif
> diff --git a/mm/slub.c b/mm/slub.c
> index 2ef88bbf56a3..44f8ded96574 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -736,6 +736,12 @@ static inline bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
>  static unsigned long object_map[BITS_TO_LONGS(MAX_OBJS_PER_PAGE)];
>  static DEFINE_SPINLOCK(object_map_lock);
>  
> +static inline void
> +__update_kmem_cache_free_objs(struct kmem_cache *s, s64 delta)
> +{
> +	percpu_counter_add_batch(&s->free_objects, delta, INT_MAX);
> +}
> +
>  static void __fill_map(unsigned long *obj_map, struct kmem_cache *s,
>  		       struct slab *slab)
>  {
> @@ -1829,6 +1835,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  	return flags | slub_debug_local;
>  }
>  #else /* !CONFIG_SLUB_DEBUG */
> +static inline void
> +__update_kmem_cache_free_objs(struct kmem_cache *s, s64 delta) {}
> +
>  static inline void setup_object_debug(struct kmem_cache *s, void *object) {}
>  static inline
>  void setup_slab_debug(struct kmem_cache *s, struct slab *slab, void *addr) {}
> @@ -2369,6 +2378,7 @@ static struct slab *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  	slab->inuse = 0;
>  	slab->frozen = 0;
>  
> +	__update_kmem_cache_free_objs(s, slab->objects);
>  	account_slab(slab, oo_order(oo), s, flags);
>  
>  	slab->slab_cache = s;
> @@ -2445,6 +2455,7 @@ static void free_slab(struct kmem_cache *s, struct slab *slab)
>  		call_rcu(&slab->rcu_head, rcu_free_slab);
>  	else
>  		__free_slab(s, slab);
> +	__update_kmem_cache_free_objs(s, -slab->objects);
>  }
>  
>  static void discard_slab(struct kmem_cache *s, struct slab *slab)
> @@ -3859,6 +3870,8 @@ static __fastpath_inline void *slab_alloc_node(struct kmem_cache *s, struct list
>  	 */
>  	slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size);
>  
> +	if (object)
> +		__update_kmem_cache_free_objs(s, -1);
>  	return object;
>  }
>  
> @@ -4235,6 +4248,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
>  	unsigned long tid;
>  	void **freelist;
>  
> +	__update_kmem_cache_free_objs(s, cnt);
>  redo:
>  	/*
>  	 * Determine the currently cpus per cpu slab.
> @@ -4286,6 +4300,7 @@ static void do_slab_free(struct kmem_cache *s,
>  				struct slab *slab, void *head, void *tail,
>  				int cnt, unsigned long addr)
>  {
> +	__update_kmem_cache_free_objs(s, cnt);
>  	__slab_free(s, slab, head, tail, cnt, addr);
>  }
>  #endif /* CONFIG_SLUB_TINY */
> @@ -4658,6 +4673,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  		memcg_slab_alloc_error_hook(s, size, objcg);
>  	}
>  
> +	__update_kmem_cache_free_objs(s, -i);
>  	return i;
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc_bulk);
> @@ -4899,6 +4915,9 @@ void __kmem_cache_release(struct kmem_cache *s)
>  	cache_random_seq_destroy(s);
>  #ifndef CONFIG_SLUB_TINY
>  	free_percpu(s->cpu_slab);
> +#endif
> +#ifdef CONFIG_SLUB_DEBUG
> +	percpu_counter_destroy(&s->free_objects);
>  #endif
>  	free_kmem_cache_nodes(s);
>  }
> @@ -5109,6 +5128,14 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
>  	s->random = get_random_long();
>  #endif
>  
> +#ifdef CONFIG_SLUB_DEBUG
> +	int ret;
> +
> +	ret = percpu_counter_init(&s->free_objects, 0, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +#endif
> +
>  	if (!calculate_sizes(s))
>  		goto error;
>  	if (disable_higher_order_debug) {
> @@ -7100,15 +7127,15 @@ void get_slabinfo(struct kmem_cache *s, struct slabinfo *sinfo)
>  {
>  	unsigned long nr_slabs = 0;
>  	unsigned long nr_objs = 0;
> -	unsigned long nr_free = 0;
> +	unsigned long nr_free;
>  	int node;
>  	struct kmem_cache_node *n;
>  
>  	for_each_kmem_cache_node(s, node, n) {
>  		nr_slabs += node_nr_slabs(n);
>  		nr_objs += node_nr_objs(n);
> -		nr_free += count_partial(n, count_free);
>  	}
> +	nr_free = percpu_counter_sum_positive(&s->free_objects);
>  
>  	sinfo->active_objs = nr_objs - nr_free;
>  	sinfo->num_objs = nr_objs;
> -- 
> 2.42.1
> 
>