On 5/4/21 3:23 PM, Waiman Long wrote:
> 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.
>
> One way to solve this problem is to split the kmalloc-<n> caches
> (KMALLOC_NORMAL) into two separate sets - a new set of kmalloc-<n>
> (KMALLOC_NORMAL) caches for non-accounted objects only and a new set of
> kmalloc-cg-<n> (KMALLOC_CGROUP) caches for accounted objects only. All
> the other caches can allow a mix of accounted and non-accounted objects.
>
> With this change, all the objcg pointer array objects will come from
> KMALLOC_NORMAL caches which won't have their objcg pointer arrays. So
> both the recursive kfree() problem and non-freeable slab problem
> are gone.
>
> The new KMALLOC_CGROUP is added between KMALLOC_NORMAL and
> KMALLOC_RECLAIM so that the first for loop in create_kmalloc_caches()
> will include the newly added caches without change.
Great, thanks I hope there would be also benefits to objcg arrays not
created for all the normal caches anymore (possibly poorly used due to
mix of accounted and non-accounted objects in the same cache) and perhaps
it's possible for you to quantify the reduction of those?
> Suggested-by: Vlastimil Babka <vbabka@xxxxxxx>
> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
...
> @@ -321,6 +328,14 @@ kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
>
> static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
> {
> +#ifdef CONFIG_MEMCG_KMEM
> + /*
> + * KMALLOC_CGROUP for non-reclaimable and non-DMA object with
> + * accounting enabled.
> + */
> + if ((flags & (__GFP_DMA | __GFP_RECLAIMABLE | __GFP_ACCOUNT)) == __GFP_ACCOUNT)
> + return KMALLOC_CGROUP;
> +#endif
This function was designed so that KMALLOC_NORMAL would be the first tested and
returned possibility, as it's expected to be the most common. What about the
following on top?
----8<----
diff --git a/include/linux/slab.h b/include/linux/slab.h
index fca03c22ea7c..418c5df0305b 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -328,30 +328,40 @@ kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
{
-#ifdef CONFIG_MEMCG_KMEM
/*
- * KMALLOC_CGROUP for non-reclaimable and non-DMA object with
- * accounting enabled.
+ * The most common case is KMALLOC_NORMAL, so test for it
+ * with a single branch for all flags that might affect it
*/
- if ((flags & (__GFP_DMA | __GFP_RECLAIMABLE | __GFP_ACCOUNT)) == __GFP_ACCOUNT)
- return KMALLOC_CGROUP;
+ if (likely((flags & (__GFP_RECLAIMABLE
+#ifdef CONFIG_MEMCG_KMEM
+ | __GFP_ACCOUNT
#endif
#ifdef CONFIG_ZONE_DMA
- /*
- * The most common case is KMALLOC_NORMAL, so test for it
- * with a single branch for both flags.
- */
- if (likely((flags & (__GFP_DMA | __GFP_RECLAIMABLE)) == 0))
+ | __GFP_DMA
+#endif
+ )) == 0))
return KMALLOC_NORMAL;
+#ifdef CONFIG_MEMCG_KMEM
/*
- * At least one of the flags has to be set. If both are, __GFP_DMA
- * is more important.
+ * KMALLOC_CGROUP for non-reclaimable and non-DMA object with
+ * accounting enabled.
*/
- return flags & __GFP_DMA ? KMALLOC_DMA : KMALLOC_RECLAIM;
-#else
- return flags & __GFP_RECLAIMABLE ? KMALLOC_RECLAIM : KMALLOC_NORMAL;
+ if ((flags & (__GFP_ACCOUNT | __GFP_RECLAIMABLE
+#ifdef CONFIG_ZONE_DMA
+ | __GFP_DMA
+#endif
+ )) == __GFP_ACCOUNT)
+ return KMALLOC_CGROUP;
#endif
+
+#ifdef CONFIG_ZONE_DMA
+ if (flags & __GFP_DMA)
+ return KMALLOC_DMA;
+#endif
+
+ /* if we got here, it has to be __GFP_RECLAIMABLE */
+ return KMALLOC_RECLAIM;
}
/*
