Messages in this thread | | | From | Waiman Long <> | Subject | Re: [PATCH v3 2/2] mm: memcg/slab: Create a new set of kmalloc-cg-<n> caches | Date | Wed, 5 May 2021 14:11:52 -0400 |
| |
On 5/5/21 1:30 PM, Roman Gushchin wrote: > On Wed, May 05, 2021 at 11:46:13AM -0400, 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 still allow a mix of accounted and non-accounted >> objects. > I agree that it's likely the best approach here. Thanks for discovering > and fixing the problem! > >> 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. Since both the KMALLOC_NORMAL and KMALLOC_CGROUP caches no longer >> have mixed accounted and unaccounted objects, this will slightly reduce >> the number of objcg pointer arrays that need to be allocated and save >> a bit of memory. > Unfortunately the positive effect of this change will be likely > reversed by a lower utilization due to a larger number of caches.
That is also true, will mention that.
> > Btw, I wonder if we also need a change in the slab caches merging procedure? > KMALLOC_NORMAL caches should not be merged with caches which can potentially > include accounted objects.
Thank for catching this omission.
I will take a look and modify the merging procedure in a new patch. Accounting is usually specified at kmem_cache_create() time. Though, I did find one instance of setting ACCOUNT flag in kmem_cache_alloc(), I will ignore this case and merge accounted, but unreclaimable caches to KMALLOC_CGROUP.
> >> 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. >> >> Suggested-by: Vlastimil Babka <vbabka@suse.cz> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> include/linux/slab.h | 42 ++++++++++++++++++++++++++++++++++-------- >> mm/slab_common.c | 23 +++++++++++++++-------- >> 2 files changed, 49 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/slab.h b/include/linux/slab.h >> index 0c97d788762c..f2d9ebc34f5c 100644 >> --- a/include/linux/slab.h >> +++ b/include/linux/slab.h >> @@ -305,9 +305,16 @@ static inline void __check_heap_object(const void *ptr, unsigned long n, >> /* >> * Whenever changing this, take care of that kmalloc_type() and >> * create_kmalloc_caches() still work as intended. >> + * >> + * KMALLOC_NORMAL is for non-accounted objects only whereas KMALLOC_CGROUP >> + * is for accounted objects only. All the other kmem caches can have both >> + * accounted and non-accounted objects. >> */ >> enum kmalloc_cache_type { >> KMALLOC_NORMAL = 0, >> +#ifdef CONFIG_MEMCG_KMEM >> + KMALLOC_CGROUP, >> +#endif >> KMALLOC_RECLAIM, >> #ifdef CONFIG_ZONE_DMA >> KMALLOC_DMA, >> @@ -315,28 +322,47 @@ enum kmalloc_cache_type { >> NR_KMALLOC_TYPES >> }; >> >> +#ifndef CONFIG_MEMCG_KMEM >> +#define KMALLOC_CGROUP KMALLOC_NORMAL >> +#endif >> +#ifndef CONFIG_ZONE_DMA >> +#define KMALLOC_DMA KMALLOC_NORMAL >> +#endif >> + >> #ifndef CONFIG_SLOB >> extern struct kmem_cache * >> kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1]; >> >> +/* >> + * Define gfp bits that should not be set for KMALLOC_NORMAL. >> + */ >> +#define KMALLOC_NOT_NORMAL_BITS \ >> + (__GFP_RECLAIMABLE | \ >> + (IS_ENABLED(CONFIG_ZONE_DMA) ? __GFP_DMA : 0) | \ >> + (IS_ENABLED(CONFIG_MEMCG_KMEM) ? __GFP_ACCOUNT : 0)) >> + >> static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) >> { >> -#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)) >> + if (likely((flags & KMALLOC_NOT_NORMAL_BITS) == 0)) >> return KMALLOC_NORMAL; > Likely KMALLOC_CGROUP is also very popular, so maybe we want to change the > optimization here a bit.
I doubt this optimization is really noticeable and whether KMALLOC_CGROUP is really popular will depend on the workloads. I am not planning to spend additional time to micro-optimize this part of the code.
Cheers, Longman
| |