Date:   Mon, 23 Oct 2023 20:44:41 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     "Christoph Lameter (Ampere)" <cl@...two.org>
Cc:     chengming.zhou@...ux.dev, penberg@...nel.org, rientjes@...gle.com,
        iamjoonsoo.kim@....com, akpm@...ux-foundation.org,
        roman.gushchin@...ux.dev, 42.hyeyoo@...il.com, willy@...radead.org,
        pcc@...gle.com, tytso@....edu, maz@...nel.org,
        ruansy.fnst@...itsu.com, vishal.moola@...il.com,
        lrh2000@....edu.cn, hughd@...gle.com, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Chengming Zhou <zhouchengming@...edance.com>
Subject: Re: [RFC PATCH v2 0/6] slub: Delay freezing of CPU partial slabs

On 10/23/23 19:00, Christoph Lameter (Ampere) wrote:
> On Mon, 23 Oct 2023, Vlastimil Babka wrote:
> 
>>>
>>> The slab will be delay frozen when it's picked to actively use by the
>>> CPU, it becomes full at the same time, in which case we still need to
>>> rely on "frozen" bit to avoid manipulating its list. So the slab will
>>> be frozen only when activate use and be unfrozen only when deactivate.
>>
>> Interesting solution! I wonder if we could go a bit further and remove
>> acquire_slab() completely. Because AFAICS even after your changes,
>> acquire_slab() is still attempted including freezing the slab, which means
>> still doing an cmpxchg_double under the list_lock, and now also handling the
>> special case when it failed, but we at least filled percpu partial lists.
>> What if we only filled the partial list without freezing, and then froze the
>> first slab outside of the list_lock?
>>
>> Or more precisely, instead of returning the acquired "object" we would
>> return the first slab removed from partial list. I think it would simplify
>> the code a bit, and further reduce list_lock holding times.
>>
>> I'll also point out a few more details, but it's not a full detailed review
>> as the suggestion above, and another for 4/5, could mean a rather
>> significant change for v3.
> 
> This is not that easy. The frozen bit indicates that list management does 
> not have to be done for a slab if its processed in free. If you take a 
> slab off the list without setting that bit then something else needs to 
> provide the information that "frozen" provided.

Yes, that's the new slab_node_partial flag in patch 1, protected by list_lock.

> If the frozen bit changes can be handled in a different way than 
> with cmpxchg then that is a good optimization.

Frozen bit stays the same, but some scenarios can now avoid it.

> For much of the frozen handling we must be holding the node list lock 
> anyways in order to add/remove from the list. So we already have a lock 
> that could be used to protect flag operations.

I can see the following differences between the traditional frozen bit and
the new flag:

frozen bit advantage:
- __slab_free() on an already-frozen slab can ignore list operations and
list_lock completely

frozen bit disadvantage:
- acquire_slab() trying to do cmpxchg_double() under list_lock (see commit
9b1ea29bc0d7)

slab_node_partial flag advantage:
- we can take slabs off from node partial list without cmpxchg_double()
- probably less cmpxchg_double() operations overall

slab_node_partial flag disadvantage:
- a __slab_free() that encouters a slab that's not frozen (but
slab_node_partial flag is not set) might have to do more work, including
taking the list_lock only to find out that slab_node_partial flag is false
(but AFAICS that happens only when the slab becomes fully free by the free
operation, thus relatively rarely).

Put together, I think we might indeed get the best of both if the frozen
flag is kept to use for cpu slabs, and we rely on slab_node_partial flag for
cpu partial slabs, as the series does.

Powered by blists - more mailing lists