Re: [RFC PATCH v2 0/6] slub: Delay freezing of CPU partial slabs

[Chengming Zhou]

On 2023/10/23 23:46, Vlastimil Babka wrote:
> On 10/21/23 16:43, chengming.zhou@xxxxxxxxx wrote:
>> From: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
> 
> Hi!
> 
>> Changes in RFC v2:
>>  - Reuse PG_workingset bit to keep track of whether slub is on the
>>    per-node partial list, as suggested by Matthew Wilcox.
>>  - Fix OOM problem on kernel without CONFIG_SLUB_CPU_PARTIAL, which
>>    is caused by leak of partial slabs when get_partial_node().
>>  - Add a patch to simplify acquire_slab().
>>  - Reorder patches a little.
>>  - v1: https://lore.kernel.org/all/20231017154439.3036608-1-chengming.zhou@xxxxxxxxx/
>>
>> 1. Problem
>> ==========
>> Now we have to freeze the slab when get from the node partial list, and
>> unfreeze the slab when put to the node partial list. Because we need to
>> rely on the node list_lock to synchronize the "frozen" bit changes.
>>
>> This implementation has some drawbacks:
>>
>>  - Alloc path: twice cmpxchg_double.
>>    It has to get some partial slabs from node when the allocator has used
>>    up the CPU partial slabs. So it freeze the slab (one cmpxchg_double)
>>    with node list_lock held, put those frozen slabs on its CPU partial
>>    list. Later ___slab_alloc() will cmpxchg_double try-loop again if that
>>    slab is picked to use.
>>
>>  - Alloc path: amplified contention on node list_lock.
>>    Since we have to synchronize the "frozen" bit changes under the node
>>    list_lock, the contention of slab (struct page) can be transferred
>>    to the node list_lock. On machine with many CPUs in one node, the
>>    contention of list_lock will be amplified by all CPUs' alloc path.
>>
>>    The current code has to workaround this problem by avoiding using
>>    cmpxchg_double try-loop, which will just break and return when
>>    contention of page encountered and the first cmpxchg_double failed.
>>    But this workaround has its own problem.
> 
> I'd note here: For more context, see 9b1ea29bc0d7 ("Revert "mm, slub:
> consider rest of partial list if acquire_slab() fails"")

Good, will add it.

> 
>>  - Free path: redundant unfreeze.
>>    __slab_free() will freeze and cache some slabs on its partial list,
>>    and flush them to the node partial list when exceed, which has to
>>    unfreeze those slabs again under the node list_lock. Actually we
>>    don't need to freeze slab on CPU partial list, in which case we
>>    can save the unfreeze cmpxchg_double operations in flush path.
>>
>> 2. Solution
>> ===========
>> We solve these problems by leaving slabs unfrozen when moving out of
>> the node partial list and on CPU partial list, so "frozen" bit is 0.
>>
>> These partial slabs won't be manipulate concurrently by alloc path,
>> the only racer is free path, which may manipulate its list when !inuse.
>> So we need to introduce another synchronization way to avoid it, we
>> reuse PG_workingset to keep track of whether the slab is on node partial
>> list or not, only in that case we can manipulate the slab list.
>>
>> 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?

Good idea, we can return one slab and put other slabs to the CPU partial list.
So we can remove the acquire_slab() completely and don't need to handle the
fail case. The code will be cleaner, too.

> 
> 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.

Ok, I will do this in the next version. But I find we have to return the object
in the "IS_ENABLED(CONFIG_SLUB_TINY) || kmem_cache_debug(s)" case, in which
we need to allocate a single object under the node list_lock.

Maybe we can use "struct partial_context" to return the object in this case?

 struct partial_context {
-       struct slab **slab;
        gfp_t flags;
        unsigned int orig_size;
+       void *object;
 };

Then we can change all get_partial interfaces to return a slab. Do you agree
with this way?

> 
> 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.

Thank you!

> 
> Thanks!
> 
>> 3. Testing
>> ==========
>> We just did some simple testing on a server with 128 CPUs (2 nodes) to
>> compare performance for now.
>>
>>  - perf bench sched messaging -g 5 -t -l 100000
>>    baseline	RFC
>>    7.042s	6.966s
>>    7.022s	7.045s
>>    7.054s	6.985s
>>
>>  - stress-ng --rawpkt 128 --rawpkt-ops 100000000
>>    baseline	RFC
>>    2.42s	2.15s
>>    2.45s	2.16s
>>    2.44s	2.17s
>>
>> It shows above there is about 10% improvement on stress-ng rawpkt
>> testcase, although no much improvement on perf sched bench testcase.
>>
>> Thanks for any comment and code review!
>>
>> Chengming Zhou (6):
>>   slub: Keep track of whether slub is on the per-node partial list
>>   slub: Prepare __slab_free() for unfrozen partial slab out of node
>>     partial list
>>   slub: Don't freeze slabs for cpu partial
>>   slub: Simplify acquire_slab()
>>   slub: Introduce get_cpu_partial()
>>   slub: Optimize deactivate_slab()
>>
>>  include/linux/page-flags.h |   2 +
>>  mm/slab.h                  |  19 +++
>>  mm/slub.c                  | 245 +++++++++++++++++++------------------
>>  3 files changed, 150 insertions(+), 116 deletions(-)
>>
>