Re: slabinfo shows incorrect active_objs ???

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

On Mon, Feb 28, 2022 at 10:22:42AM +0000, Hyeonggon Yoo wrote:
> 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.
> > 
> > ---[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;
> 
> Hi Vasily, thank you for this patch.
> This looks nice, but I see things we can improve:
> 
> 1) using raw_cpu_{inc,sub}(), s->cpu_slab->inuse will be racy if kernel
> can be preempted. slub does not disable preemption/interrupts at all in fastpath.
> 
> And yeah, we can accept being racy to some degree. but it will be incorrect
> more and more if system is up for long time. So I think atomic integer
> is right choice if correctness is important?
> 
> 2) This code is not aware of cpu partials. there is list of slab for
> each kmem_cache_cpu. you can iterate them by: 
> 

And replying this, I realized again ... we need to consider disabling
preemption when freeing to remote cpu's partials if CONFIG_SLUB_PRECISE_INUSE=y.

Hmm, do we need another approach?

> 	kmem_cache_cpu->partial->next->next->next->... and so on until it enters NULL.
> 
> So we need to count cpu partials' inuse too.
> Then we need per-slab counters... I think we can use struct slab's
> __unused field for this?
> 
> Thanks :)
> 
> -- 
> Thank you, You are awesome!
> Hyeonggon :-)

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