On 8/29/24 02:49, Yosry Ahmed wrote:

On Wed, Aug 28, 2024 at 5:20 PM Shakeel Butt [off-list ref] wrote:

quoted

On Wed, Aug 28, 2024 at 04:25:30PM GMT, Yosry Ahmed wrote:

quoted

On Tue, Aug 27, 2024 at 4:52 PM Shakeel Butt [off-list ref] wrote:

quoted

[...]

quoted

quoted

+
+       /* Ignore KMALLOC_NORMAL cache to avoid circular dependency. */
+       if ((s->flags & KMALLOC_TYPE) == SLAB_KMALLOC)
+               return true;

Taking a step back here, why do we need this? Which circular
dependency are we avoiding here?

commit 494c1dfe855ec1f70f89552fce5eadf4a1717552
Author: Waiman Long [off-list ref]
Date:   Mon Jun 28 19:37:38 2021 -0700

    mm: memcg/slab: create a new set of kmalloc-cg-<n> caches

    There are currently two problems in the way the objcg pointer array
    (memcg_data) in the page structure is being allocated and freed.

    On its allocation, it is possible that the allocated objcg pointer
    array comes from the same slab that requires memory accounting. If this
    happens, the slab will never become empty again as there is at least
    one object left (the obj_cgroup array) in the slab.

    When it is freed, the objcg pointer array object may be the last one
    in its slab and hence causes kfree() to be called again. With the
    right workload, the slab cache may be set up in a way that allows the
    recursive kfree() calling loop to nest deep enough to cause a kernel
    stack overflow and panic the system.
    ...

Thanks for the reference, this makes sense.

Another reason is memory savings, if we have a small subset of objects in
KMALLOC_NORMAL caches accounted, there might be e.g. one vector per a slab
just to account on object while the rest is unaccounted. Separating between
kmalloc and kmalloc-cg caches keeps the former with no vectors and the
latter with fully used vectors.

Wouldn't it be easier to special case the specific slab cache used for
the objcg vector or use a dedicated cache for it instead of using
kmalloc caches to begin with?

The problem is the vector isn't a fixed size, it depends on how many objects
a particular slab (not even a particular cache) has.

Anyway, I am fine with any approach you and/or the slab maintainers
prefer, as long as we make things clear. If you keep the following
approach as-is, please expand the comment or refer to the commit you
just referenced.

Personally, I prefer either explicitly special casing the slab cache
used for the objcgs vector, explicitly tagging KMALLOC_NORMAL
allocations, or having a dedicated documented helper that finds the
slab cache kmalloc type (if any) or checks if it is a KMALLOC_NORMAL
cache.

A helper to check is_kmalloc_normal() would be better than defining
KMALLOC_TYPE and using it directly, yes. We don't need to handle any other
types now until anyone needs those.