Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <ba3d6ac7-6900-3e8d-46b5-8302ca61f8ef@suse.cz>
Date:   Mon, 23 Oct 2023 20:44:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [RFC PATCH v2 0/6] slub: Delay freezing of CPU partial slabs
Content-Language: en-US
To:     "Christoph Lameter (Ampere)" <cl@gentwo.org>
Cc:     chengming.zhou@linux.dev, penberg@kernel.org, rientjes@google.com,
        iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
        roman.gushchin@linux.dev, 42.hyeyoo@gmail.com, willy@infradead.org,
        pcc@google.com, tytso@mit.edu, maz@kernel.org,
        ruansy.fnst@fujitsu.com, vishal.moola@gmail.com,
        lrh2000@pku.edu.cn, hughd@google.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, Chengming Zhou <zhouchengming@bytedance.com>
References: <20231021144317.3400916-1-chengming.zhou@linux.dev>
 <4134b039-fa99-70cd-3486-3d0c7632e4a3@suse.cz>
 <fcb9b7f0-fb21-cce8-d452-766a5cc73d4a@gentwo.org>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <fcb9b7f0-fb21-cce8-d452-766a5cc73d4a@gentwo.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.