Re: [PATCH v2] mm/slub: allocate sheaves on local memory nodes

[Harry Yoo <harry@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2932 bytes --]



On 6/9/26 7:14 PM, Vlastimil Babka (SUSE) wrote:
> On 6/9/26 05:41, Harry Yoo wrote:
>> On 6/3/26 1:26 PM, Hao Li wrote:
>>> On Mon, Jun 01, 2026 at 08:28:16PM +0900, Harry Yoo wrote:
>>>> On 6/1/26 6:56 PM, Hao Li wrote:
>>>>> Sheaf structs are exchanged through node-local barns. Since barn structs
>>>>> are already allocated from their local NUMA node, this patch aims to
>>>>> allocate sheaf structs from their local memory nodes as well.
>>>>>
>>>>> To achieve this, the obvious choice would be using cpu_to_mem().
>>>>> However, init_percpu_sheaves() and bootstrap_cache_sheaves() iterate
>>>>> through possible CPUs, whereas cpu_to_mem() is only initialized for
>>>>> online CPUs. Therefore, we cannot use cpu_to_mem() and instead need to
>>>>> use local_memory_node(cpu_to_node(cpu)), similar to what
>>>>> __build_all_zonelists() does.
>>>>>
>>>>> The primary goal of this patch is to improve NUMA node locality.
>>>>> Although the actual performance impact is minor, it still yields a ~1%
>>>>> improvement on a 192-core, 8-NUMA-node system when testing with the
>>>>> will-it-scale mmap test case.
>>>>
>>>> Oh, nice :)
>>>>
>>>> I have a question though...
>>>>
>>>> I wonder if would be better to handle this by e.g.) not returning empty
>>>> sheaves back to barn and freeing them if the node id doesn't match and
>>>> it's not a memoryless node.
>>>>
>>>> init_percpu_sheaves() and bootstrap_cache_sheaves() are not the only
>>>> places that can allocate sheaves from remote nodes; sheaves allocation
>>>> could fall back to other nodes and then SLUB could keep reusing those
>>>> sheaves from remote nodes even after memory is reclaimed.
>>>
>>> This is a good catch. In addition to the fallback mechanism, task migration
>>> between CPUs in __pcs_replace_empty_main() and __pcs_replace_full_main() can
>>> also mix up sheaf structs across different barns. So yeah, changing allocation
>>> locality is not a silver bullet.
>>>
>>>> If this works well, we probably don't need to handle it in
>>>> init_percpu_sheaves() and bootstrap_cache_sheaves() at all as they will
>>>> eventually be freed, while covering the other case too?
>>>
>>> freeing the empty sheaf if the NUMA node mismatches instead of putting it back
>>> into the barn is indeed a good idea. I like it. But unfortunately, my testing
>>> didn't show a clear performance improvement, though there was no noticeable
>>> degradation either. :-(
>>
>> Hmm... the idea still makes sense to me, but yeah, it's not late to fix
>> it when we have data to back up.
> 
> I think if that approach doesn't complicate things unreasonably, it makes
> sense to do it even if there are no clear wins (as long as there are no
> regressions).

I'm fine with that unless it introduces regressions or the
implementation complicates things too much :)

What do you say, Hao?

-- 
Cheers,
Harry / Hyeonggon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]