On Fri, Sep 06, 2024 at 10:52:04AM GMT, Vlastimil Babka wrote:

On 9/5/24 20:48, Shakeel Butt wrote:

quoted

quoted

quoted

---
v3: https://lore.kernel.org/all/20240829175339.2424521-1-shakeel.butt@linux.dev/ (local)
Changes since v3:
- Add kernel doc for kmem_cache_charge.

v2: https://lore.kernel.org/all/20240827235228.1591842-1-shakeel.butt@linux.dev/ (local)
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/ (local)
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/ (local)
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"? 

This is just a copy-paste from kmalloc() documentation.

quoted

quoted

quoted

+ * 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.
 */
 

Yes, this is much better.

quoted

quoted

quoted

+
+       /* 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.
         */

Looks great to me.

thanks,
Shakeel