在 2023/3/14 22:41, Vlastimil Babka 写道:
>
> On 3/14/23 13:34, Chen Jun wrote:
>> When kmalloc_node() is called without __GFP_THISNODE and the target node
>> lacks sufficient memory, SLUB allocates a folio from a different node
>> other than the requested node, instead of taking a partial slab from it.
>>
>> However, since the allocated folio does not belong to the requested
>> node, it is deactivated and added to the partial slab list of the node
>> it belongs to.
>>
>> This behavior can result in excessive memory usage when the requested
>> node has insufficient memory, as SLUB will repeatedly allocate folios
>> from other nodes without reusing the previously allocated ones.
>>
>> To prevent memory wastage,
>> when (node != NUMA_NO_NODE) && (gfpflags & __GFP_THISNODE) is:
>> 1) try to get a partial slab from target node with __GFP_THISNODE.
>> 2) if 1) failed, try to allocate a new slab from target node with
>> __GFP_THISNODE.
>> 3) if 2) failed, retry 1) and 2) without __GFP_THISNODE constraint.
>>
>> when node != NUMA_NO_NODE || (gfpflags & __GFP_THISNODE), the behavior
>> remains unchanged.
>>
>> On qemu with 4 numa nodes and each numa has 1G memory. Write a test ko
>> to call kmalloc_node(196, GFP_KERNEL, 3) for (4 * 1024 + 4) * 1024 times.
>>
>> cat /proc/slabinfo shows:
>> kmalloc-256 4200530 13519712 256 32 2 : tunables..
>>
>> after this patch,
>> cat /proc/slabinfo shows:
>> kmalloc-256 4200558 4200768 256 32 2 : tunables..
>>
>> Signed-off-by: Chen Jun <chenjun102@xxxxxxxxxx>
>> ---
>> mm/slub.c | 22 +++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 39327e98fce3..32e436957e03 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2384,7 +2384,7 @@ static void *get_partial(struct kmem_cache *s, int node, struct partial_context
>> searchnode = numa_mem_id();
>>
>> object = get_partial_node(s, get_node(s, searchnode), pc);
>> - if (object || node != NUMA_NO_NODE)
>> + if (object || (node != NUMA_NO_NODE && (pc->flags & __GFP_THISNODE)))
>> return object;
>>
>> return get_any_partial(s, pc);
>> @@ -3069,6 +3069,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> struct slab *slab;
>> unsigned long flags;
>> struct partial_context pc;
>> + bool try_thisnode = true;
>>
>> stat(s, ALLOC_SLOWPATH);
>>
>> @@ -3181,8 +3182,18 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> }
>>
>> new_objects:
>> -
>> pc.flags = gfpflags;
>> +
>> + /*
>> + * when (node != NUMA_NO_NODE) && (gfpflags & __GFP_THISNODE)
>> + * 1) try to get a partial slab from target node with __GFP_THISNODE.
>> + * 2) if 1) failed, try to allocate a new slab from target node with
>> + * __GFP_THISNODE.
>> + * 3) if 2) failed, retry 1) and 2) without __GFP_THISNODE constraint.
>> + */
>> + if (node != NUMA_NO_NODE && !(gfpflags & __GFP_THISNODE) && try_thisnode)
>> + pc.flags |= __GFP_THISNODE;
>
> Hmm I'm thinking we should also perhaps remove direct reclaim possibilities
> from the attempt 2). In your qemu test it should make no difference, as it
> fills everything with kernel memory that is not reclaimable. But in practice
> the target node might be filled with user memory, and I think it's better to
> quickly allocate on a different node than spend time in direct reclaim. So
> the following should work I think?
>
> pc.flags = GFP_NOWAIT | __GFP_NOWARN |__GFP_THISNODE
>
Hmm, Should it be that:
pc.flags |= GFP_NOWAIT | __GFP_NOWARN |__GFP_THISNODE
^
>> +
>> pc.slab = &slab;
>> pc.orig_size = orig_size;
>> freelist = get_partial(s, node, &pc);
>> @@ -3190,10 +3201,15 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> goto check_new_slab;
>>
>> slub_put_cpu_ptr(s->cpu_slab);
>> - slab = new_slab(s, gfpflags, node);
>> + slab = new_slab(s, pc.flags, node);
>> c = slub_get_cpu_ptr(s->cpu_slab);
>>
>> if (unlikely(!slab)) {
>> + if (node != NUMA_NO_NODE && !(gfpflags & __GFP_THISNODE) && try_thisnode) {
>> + try_thisnode = false;
>> + goto new_objects;
>> + }
>> +
>> slab_out_of_memory(s, gfpflags, node);
>> return NULL;
>> }
>
>
