On 8/29/24 02:49, Yosry Ahmed wrote: > On Wed, Aug 28, 2024 at 5:20 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: >> >> On Wed, Aug 28, 2024 at 04:25:30PM GMT, Yosry Ahmed wrote: >> > On Tue, Aug 27, 2024 at 4:52 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: >> > > >> [...] >> > > + >> > > + /* 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 <longman@xxxxxxxxxx> >> 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.
