Re: slabinfo shows incorrect active_objs ???

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Mon, Feb 28, 2022 at 09:17:27AM +0300, Vasily Averin wrote:
> On 25.02.2022 07:37, Vasily Averin wrote:
> > On 25.02.2022 03:08, Roman Gushchin wrote:
> > > 
> > > > On Feb 24, 2022, at 5:17 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
> > > > 
> > > > On 22.02.2022 19:32, Shakeel Butt wrote:
> > > > > If you are just interested in the stats, you can use SLAB for your experiments.
> > > > 
> > > > Unfortunately memcg_slabino.py does not support SLAB right now.
> > > > 
> > > > > On 23.02.2022 20:31, Vlastimil Babka wrote:
> > > > > > On 2/23/22 04:45, Hyeonggon Yoo wrote:
> > > > > > On Wed, Feb 23, 2022 at 01:32:36AM +0100, Vlastimil Babka wrote:
> > > > > > > Hm it would be easier just to disable merging when the precise counters are
> > > > > > > enabled. Assume it would be a config option (possibly boot-time option with
> > > > > > > static keys) anyway so those who don't need them can avoid the overhead.
> > > > > > 
> > > > > > Is it possible to accurately account objects in SLUB? I think it's not
> > > > > > easy because a CPU can free objects to remote cpu's partial slabs using
> > > > > > cmpxchg_double()...
> > > > > AFAIU Roman's idea would be that each alloc/free would simply inc/dec an
> > > > > object counter that's disconnected from physical handling of particular sl*b
> > > > > implementation. It would provide exact count of objects from the perspective
> > > > > of slab users.
> > > > > I assume for reduced overhead the counters would be implemented in a percpu
> > > > > fashion as e.g. vmstats. Slabinfo gathering would thus have to e.g. sum up
> > > > > those percpu counters.
> > > > 
> > > > I like this idea too and I'm going to spend some time for its implementation.
> > > 
> > > Sounds good!
> > > 
> > > Unfortunately it’s quite tricky: the problem is that there is potentially a large and dynamic set of cgroups and also large and dynamic set of slab caches. Given the performance considerations, it’s also unlikely to avoid using percpu variables.
> > > So we come to the (nr_slab_caches * nr_cgroups * nr_cpus) number of “objects”. If we create them proactively, we’re likely wasting lot of memory. Creating them on demand is tricky too (especially without losing some accounting accuracy).
> > 
> > I told about global (i.e. non-memcg) precise slab counters only.
> > I'm expect it can done under new config option and/or static key, and if present use them in /proc/slabinfo output.
> > 
> > At present I'm still going to extract memcg counters via your memcg_slabinfo script.
> 
> I'm not sure I'll be able to debug this patch properly and decided to submit it as is.
> I hope it can be useful.
> 
> In general it works and /proc/slabinfo shows reasonable numbers,
> however in some cases they differs from crash' "kmem -s" output, either +1 or -1.
> Obviously I missed something.
>

Oh, sorry for the noise. You implemented what Roman said.
So s->cpu_slab->inuse is just per-cpu counters for every object of a cache,
not cpu slab. Please ignore my last feedback.

Anyway, I think the +1 or -1 difference is due to race?
What was your preemption model?

> ---[cut here]---
> [PATCH RFC] slub: precise in-use counter for /proc/slabinfo output
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  include/linux/slub_def.h |  3 +++
>  init/Kconfig             |  7 +++++++
>  mm/slub.c                | 20 +++++++++++++++++++-
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 33c5c0e3bd8d..d22e18dfe905 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -56,6 +56,9 @@ struct kmem_cache_cpu {
>  #ifdef CONFIG_SLUB_STATS
>  	unsigned stat[NR_SLUB_STAT_ITEMS];
>  #endif
> +#ifdef CONFIG_SLUB_PRECISE_INUSE
> +	unsigned inuse;		/* Precise in-use counter */
> +#endif
>  };
>  #ifdef CONFIG_SLUB_CPU_PARTIAL
> diff --git a/init/Kconfig b/init/Kconfig
> index e9119bf54b1f..5c57bdbb8938 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1995,6 +1995,13 @@ config SLUB_CPU_PARTIAL
>  	  which requires the taking of locks that may cause latency spikes.
>  	  Typically one would choose no for a realtime system.
> +config SLUB_PRECISE_INUSE
> +	default n
> +	depends on SLUB && SMP
> +	bool "SLUB precise in-use counter"
> +	help
> +	  Per cpu in-use counter shows precise statistic in slabinfo.
> +
>  config MMAP_ALLOW_UNINITIALIZED
>  	bool "Allow mmapped anonymous memory to be uninitialized"
>  	depends on EXPERT && !MMU
> diff --git a/mm/slub.c b/mm/slub.c
> index 261474092e43..90750cae0af9 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3228,6 +3228,9 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>  out:
>  	slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init);
> +#ifdef CONFIG_SLUB_PRECISE_INUSE
> +	raw_cpu_inc(s->cpu_slab->inuse);
> +#endif
>  	return object;
>  }
> @@ -3506,8 +3509,12 @@ static __always_inline void slab_free(struct kmem_cache *s, struct slab *slab,
>  	 * With KASAN enabled slab_free_freelist_hook modifies the freelist
>  	 * to remove objects, whose reuse must be delayed.
>  	 */
> -	if (slab_free_freelist_hook(s, &head, &tail, &cnt))
> +	if (slab_free_freelist_hook(s, &head, &tail, &cnt)) {
>  		do_slab_free(s, slab, head, tail, cnt, addr);
> +#ifdef CONFIG_SLUB_PRECISE_INUSE
> +		raw_cpu_sub(s->cpu_slab->inuse, cnt);
> +#endif
> +	}
>  }
>  #ifdef CONFIG_KASAN_GENERIC
> @@ -6253,6 +6260,17 @@ void get_slabinfo(struct kmem_cache *s, struct slabinfo *sinfo)
>  		nr_free += count_partial(n, count_free);
>  	}
> +#ifdef CONFIG_SLUB_PRECISE_INUSE
> +	{
> +		unsigned int cpu, nr_inuse = 0;
> +
> +		for_each_possible_cpu(cpu)
> +			nr_inuse += per_cpu_ptr((s)->cpu_slab, cpu)->inuse;
> +
> +		if (nr_inuse <= nr_objs)
> +			nr_free = nr_objs - nr_inuse;
> +	}
> +#endif
>  	sinfo->active_objs = nr_objs - nr_free;
>  	sinfo->num_objs = nr_objs;
>  	sinfo->active_slabs = nr_slabs;
> -- 
> 2.25.1

-- 
Thank you, You are awesome!
Hyeonggon :-)