On Mon, Mar 07, 2022 at 05:40:42PM +0100, Vlastimil Babka wrote:
> On 3/7/22 08:40, Hyeonggon Yoo wrote:
> > Simplify deactivate_slab() by unlocking n->list_lock and retrying
> > cmpxchg_double() when cmpxchg_double() fails, and perform
> > add_{partial,full} only when it succeed.
> >
> > Releasing and taking n->list_lock again here is not harmful as SLUB
> > avoids deactivating slabs as much as possible.
> >
> > [ vbabka@xxxxxxx: perform add_{partial,full} when cmpxchg_double()
> > succeed.
> >
> > count deactivating full slabs even if debugging flag is not set. ]
> >
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
>
> Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
>
> adding both to slab-next. Fixed up some nits myself, see below:
>
> >
> > @@ -2420,61 +2416,50 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab,
> > new.frozen = 0;
> >
> > if (!new.inuse && n->nr_partial >= s->min_partial)
> > - m = M_FREE;
> > + mode = M_FREE;
> > else if (new.freelist) {
>
> This was against kernel style even before the patch - we use { } in the
> 'else if' branch, thus all branches should use { } even if one-line.
>
Ah, you are right. Agree with this change.
"Remove unnecessary brace" rule does not apply here.
> > - m = M_PARTIAL;
> > - if (!lock) {
> > - lock = 1;
> > - /*
> > - * Taking the spinlock removes the possibility that
> > - * acquire_slab() will see a slab that is frozen
> > - */
> > - spin_lock_irqsave(&n->list_lock, flags);
> > - }
> > - } else {
> > - m = M_FULL;
> > - if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && !lock) {
> > - lock = 1;
> > - /*
> > - * This also ensures that the scanning of full
> > - * slabs from diagnostic functions will not see
> > - * any frozen slabs.
> > - */
> > - spin_lock_irqsave(&n->list_lock, flags);
> > - }
> > - }
> > -
> > - if (l != m) {
> > - if (l == M_PARTIAL)
> > - remove_partial(n, slab);
> > - else if (l == M_FULL)
> > - remove_full(s, n, slab);
> > + mode = M_PARTIAL;
> > + /*
> > + * Taking the spinlock removes the possibility that
> > + * acquire_slab() will see a slab that is frozen
> > + */
> > + spin_lock_irqsave(&n->list_lock, flags);
> > + } else if (kmem_cache_debug_flags(s, SLAB_STORE_USER)) {
> > + mode = M_FULL;
> > + /*
> > + * This also ensures that the scanning of full
> > + * slabs from diagnostic functions will not see
> > + * any frozen slabs.
> > + */
> > + spin_lock_irqsave(&n->list_lock, flags);
> > + } else
> > + mode = M_FULL_NOLIST;
>
> Ditto here (this is new).
Yes.
>
> > - if (m == M_PARTIAL)
> > - add_partial(n, slab, tail);
> > - else if (m == M_FULL)
> > - add_full(s, n, slab);
> > - }
> >
> > - l = m;
> > if (!cmpxchg_double_slab(s, slab,
> > old.freelist, old.counters,
> > new.freelist, new.counters,
> > - "unfreezing slab"))
> > + "unfreezing slab")) {
> > + if (mode == M_PARTIAL || mode == M_FULL)
> > + spin_unlock_irqrestore(&n->list_lock, flags);
> > goto redo;
> > + }
> >
> > - if (lock)
> > - spin_unlock_irqrestore(&n->list_lock, flags);
> >
> > - if (m == M_PARTIAL)
> > + if (mode == M_PARTIAL) {
> > + add_partial(n, slab, tail);
> > + spin_unlock_irqrestore(&n->list_lock, flags);
> > stat(s, tail);
> > - else if (m == M_FULL)
> > - stat(s, DEACTIVATE_FULL);
> > - else if (m == M_FREE) {
> > + } else if (mode == M_FREE) {
> > stat(s, DEACTIVATE_EMPTY);
> > discard_slab(s, slab);
> > stat(s, FREE_SLAB);
> > - }
> > + } else if (mode == M_FULL) {
> > + add_full(s, n, slab);
> > + spin_unlock_irqrestore(&n->list_lock, flags);
> > + stat(s, DEACTIVATE_FULL);
> > + } else if (mode == M_FULL_NOLIST)
> > + stat(s, DEACTIVATE_FULL);
>
> And here.
>
Yes.
> > }
> >
> > #ifdef CONFIG_SLUB_CPU_PARTIAL
>
Thanks!
--
Thank you, You are awesome!
Hyeonggon :-)
