[ +Cc Jesper - he might have an opinion on this. ]
On Wed, Jul 26, 2023 at 7:34 PM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> Nit: I would change the subject from "Revert: " as it's not a revert
> exactly. If we can come up with a good subject that's not very long :)
Will do :)
> On 7/23/23 21:09, Hyeonggon Yoo wrote:
> > This is partial revert of commit b47291ef02b0 ("mm, slub: change percpu
> > partial accounting from objects to pages"). and full revert of commit
> > 662188c3a20e ("mm/slub: Simplify struct slab slabs field definition").
> >
> > While b47291ef02b0 prevents percpu partial slab list becoming too long,
> > it assumes that the order of slabs are always oo_order(s->oo).
> I think I've considered this possibility, but decided it's not important
> because if the system becomes memory pressured in a way that it can't
> allocate the oo_order() and has to fallback, we no longer care about
> accurate percpu caching, as we're unlikely having optimum performance anyway.
But it does not perform any direct reclamation/compaction to allocate
high order slabs,
so isn't it an easier condition to happen than that?
> > The current approach can surprisingly lower the number of objects cached
> > per cpu when it fails to allocate high order slabs. Instead of accounting
> > the number of slabs, change it back to accounting objects, but keep
> > the assumption that the slab is always half-full.
>
> That's a nice solution as that avoids converting the sysfs variable, so I
> wouldn't mind going that way even if I doubt the performance benefits in a
> memory pressured system.
> But maybe there's a concern that if the system is
> really memory pressured and has to fallback to smaller orders, before this
> patch it would keep fewer percpu partial slabs than after this patch, which
> would increase the pressure further and thus be counter-productive?
You mean SLUB needs to stop per-cpu caching when direct/or indirect
reclamation is desired?
> > With this change, the number of cached objects per cpu is not surprisingly
> > decreased even when it fails to allocate high order slabs. It still
> > prevents large inaccuracy because it does not account based on the
> > number of free objects when taking slabs.
> > ---
> > include/linux/slub_def.h | 2 --
> > mm/slab.h | 6 ++++++
> > mm/slub.c | 31 ++++++++++++-------------------
> > 3 files changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> > index deb90cf4bffb..589ff6a2a23f 100644
> > --- a/include/linux/slub_def.h
> > +++ b/include/linux/slub_def.h
> > @@ -109,8 +109,6 @@ struct kmem_cache {
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
> > /* Number of per cpu partial objects to keep around */
> > unsigned int cpu_partial;
> > - /* Number of per cpu partial slabs to keep around */
> > - unsigned int cpu_partial_slabs;
> > #endif
> > struct kmem_cache_order_objects oo;
> >
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 799a315695c6..be38a264df16 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -65,7 +65,13 @@ struct slab {
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
> > struct {
> > struct slab *next;
> > +#ifdef CONFIG_64BIT
> > int slabs; /* Nr of slabs left */
> > + int pobjects; /* Approximate count */
> > +#else
> > + short int slabs;
> > + short int pobjects;
> > +#endif
> > };
> > #endif
> > };
> > diff --git a/mm/slub.c b/mm/slub.c
> > index f7940048138c..199d3d03d5b9 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -486,18 +486,7 @@ static inline unsigned int oo_objects(struct kmem_cache_order_objects x)
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
> > static void slub_set_cpu_partial(struct kmem_cache *s, unsigned int nr_objects)
> > {
> > - unsigned int nr_slabs;
> > -
> > s->cpu_partial = nr_objects;
> > -
> > - /*
> > - * We take the number of objects but actually limit the number of
> > - * slabs on the per cpu partial list, in order to limit excessive
> > - * growth of the list. For simplicity we assume that the slabs will
> > - * be half-full.
> > - */
> > - nr_slabs = DIV_ROUND_UP(nr_objects * 2, oo_objects(s->oo));
> > - s->cpu_partial_slabs = nr_slabs;
> > }
> > #else
> > static inline void
> > @@ -2275,7 +2264,7 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> > struct slab *slab, *slab2;
> > void *object = NULL;
> > unsigned long flags;
> > - unsigned int partial_slabs = 0;
> > + int objects_taken = 0;
> >
> > /*
> > * Racy check. If we mistakenly see no partial slabs then we
> > @@ -2312,11 +2301,11 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
> > } else {
> > put_cpu_partial(s, slab, 0);
> > stat(s, CPU_PARTIAL_NODE);
> > - partial_slabs++;
> > + objects_taken += slab->objects / 2;
> > }
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
> > if (!kmem_cache_has_cpu_partial(s)
> > - || partial_slabs > s->cpu_partial_slabs / 2)
> > + || objects_taken > s->cpu_partial / 2)
> > break;
> > #else
> > break;
> > @@ -2699,13 +2688,14 @@ static void put_cpu_partial(struct kmem_cache *s, struct slab *slab, int drain)
> > struct slab *slab_to_unfreeze = NULL;
> > unsigned long flags;
> > int slabs = 0;
> > + int pobjects = 0;
> >
> > local_lock_irqsave(&s->cpu_slab->lock, flags);
> >
> > oldslab = this_cpu_read(s->cpu_slab->partial);
> >
> > if (oldslab) {
> > - if (drain && oldslab->slabs >= s->cpu_partial_slabs) {
> > + if (drain && oldslab->pobjects >= s->cpu_partial) {
> > /*
> > * Partial array is full. Move the existing set to the
> > * per node partial list. Postpone the actual unfreezing
> > @@ -2714,14 +2704,17 @@ static void put_cpu_partial(struct kmem_cache *s, struct slab *slab, int drain)
> > slab_to_unfreeze = oldslab;
> > oldslab = NULL;
> > } else {
> > + pobjects = oldslab->pobjects;
> > slabs = oldslab->slabs;
> > }
> > }
> >
> > slabs++;
> > + pobjects += slab->objects / 2;
> >
> > slab->slabs = slabs;
> > slab->next = oldslab;
> > + slab->pobjects = pobjects;
> >
> > this_cpu_write(s->cpu_slab->partial, slab);
> >
> > @@ -5653,13 +5646,13 @@ static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
> >
> > slab = slub_percpu_partial(per_cpu_ptr(s->cpu_slab, cpu));
> >
> > - if (slab)
> > + if (slab) {
> > slabs += slab->slabs;
> > + objects += slab->objects;
> > + }
> > }
> > #endif
> >
> > - /* Approximate half-full slabs, see slub_set_cpu_partial() */
> > - objects = (slabs * oo_objects(s->oo)) / 2;
> > len += sysfs_emit_at(buf, len, "%d(%d)", objects, slabs);
> >
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
> > @@ -5669,7 +5662,7 @@ static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
> > slab = slub_percpu_partial(per_cpu_ptr(s->cpu_slab, cpu));
> > if (slab) {
> > slabs = READ_ONCE(slab->slabs);
> > - objects = (slabs * oo_objects(s->oo)) / 2;
> > + objects = READ_ONCE(slab->pobjects);
> > len += sysfs_emit_at(buf, len, " C%d=%d(%d)",
> > cpu, objects, slabs);
> > }
>
