[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <572688a7-8719-4f94-a5cd-e726486c757d@suse.cz>
Date: Fri, 6 Sep 2024 10:52:04 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Shakeel Butt <shakeel.butt@...ux.dev>, Yosry Ahmed <yosryahmed@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Johannes Weiner <hannes@...xchg.org>, Michal Hocko <mhocko@...nel.org>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Muchun Song <muchun.song@...ux.dev>, David Rientjes <rientjes@...gle.com>,
Hyeonggon Yoo <42.hyeyoo@...il.com>, Eric Dumazet <edumazet@...gle.com>,
"David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Meta kernel team <kernel-team@...a.com>,
cgroups@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v4] memcg: add charging of already allocated slab objects
On 9/5/24 20:48, Shakeel Butt wrote:
>> > ---
>> > v3: https://lore.kernel.org/all/20240829175339.2424521-1-shakeel.butt@linux.dev/
>> > Changes since v3:
>> > - Add kernel doc for kmem_cache_charge.
>> >
>> > v2: https://lore.kernel.org/all/20240827235228.1591842-1-shakeel.butt@linux.dev/
>> > Change since v2:
>> > - Add handling of already charged large kmalloc objects.
>> > - Move the normal kmalloc cache check into a function.
>> >
>> > v1: https://lore.kernel.org/all/20240826232908.4076417-1-shakeel.butt@linux.dev/
>> > Changes since v1:
>> > - Correctly handle large allocations which bypass slab
>> > - Rearrange code to avoid compilation errors for !CONFIG_MEMCG builds
>> >
>> > RFC: https://lore.kernel.org/all/20240824010139.1293051-1-shakeel.butt@linux.dev/
>> > Changes since the RFC:
>> > - Added check for already charged slab objects.
>> > - Added performance results from neper's tcp_crr
>> >
>> >
>> > include/linux/slab.h | 20 ++++++++++++++
>> > mm/slab.h | 7 +++++
>> > mm/slub.c | 49 +++++++++++++++++++++++++++++++++
>> > net/ipv4/inet_connection_sock.c | 5 ++--
>> > 4 files changed, 79 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/linux/slab.h b/include/linux/slab.h
>> > index eb2bf4629157..68789c79a530 100644
>> > --- a/include/linux/slab.h
>> > +++ b/include/linux/slab.h
>> > @@ -547,6 +547,26 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
>> > gfp_t gfpflags) __assume_slab_alignment __malloc;
>> > #define kmem_cache_alloc_lru(...) alloc_hooks(kmem_cache_alloc_lru_noprof(__VA_ARGS__))
>> >
>> > +/**
>> > + * kmem_cache_charge - memcg charge an already allocated slab memory
>> > + * @objp: address of the slab object to memcg charge.
>> > + * @gfpflags: describe the allocation context
>> > + *
>> > + * kmem_cache_charge is the normal method to charge a slab object to the current
what is "normal method"?
>> > + * memcg. The objp should be pointer returned by the slab allocator functions
>> > + * like kmalloc or kmem_cache_alloc. The memcg charge behavior can be controller
>>
>> s/controller/controlled
>
> Thanks. Vlastimil please fix this when you pick this up.
I felt it could be improved more, so ended up with this. Thoughts?
/**
* kmem_cache_charge - memcg charge an already allocated slab memory
* @objp: address of the slab object to memcg charge
* @gfpflags: describe the allocation context
*
* kmem_cache_charge allows charging a slab object to the current memcg,
* primarily in cases where charging at allocation time might not be possible
* because the target memcg is not known (i.e. softirq context)
*
* The objp should be pointer returned by the slab allocator functions like
* kmalloc (with __GFP_ACCOUNT in flags) or kmem_cache_alloc. The memcg charge
* behavior can be controlled through gfpflags parameter, which affects how the
* necessary internal metadata can be allocated. Including __GFP_NOFAIL denotes
* that overcharging is requested instead of failure, but is not applied for the
* internal metadata allocation.
*
* There are several cases where it will return true even if the charging was
* not done:
* More specifically:
*
* 1. For !CONFIG_MEMCG or cgroup_disable=memory systems.
* 2. Already charged slab objects.
* 3. For slab objects from KMALLOC_NORMAL caches - allocated by kmalloc()
* without __GFP_ACCOUNT
* 4. Allocating internal metadata has failed
*
* Return: true if charge was successful otherwise false.
*/
>> > +
>> > + /* Ignore KMALLOC_NORMAL cache to avoid circular dependency. */
>>
>> Is it possible to point to the commit that has the explanation here?
>> The one you pointed me to before? Otherwise it's not really obvious
>> where the circular dependency comes from (at least to me).
>>
>
> Not sure about the commit reference. We can add more text here.
> Vlastimil, how much detail do you prefer?
What about:
/*
* Ignore KMALLOC_NORMAL cache to avoid possible circular dependency
* of slab_obj_exts being allocated from the same slab and thus the slab
* becoming effectively unfreeable.
*/
> thanks,
> Shakeel
Powered by blists - more mailing lists