Re: [PATCH v3 2/2] mm: memcg/slab: Create a new set of kmalloc-cg-<n> caches

[Waiman Long]

On 5/5/21 12:17 PM, Shakeel Butt wrote:
> On Wed, May 5, 2021 at 8:47 AM Waiman Long <longman@xxxxxxxxxx> 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.
> >
> > 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.
> >
> > 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@xxxxxxx>
> > Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
> One nit below and after incorporating Vlastimil's suggestions:
>
> Reviewed-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
>
> > ---
> >   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.
> I think you can say "KMALLOC_CGROUP is for accounted and unreclaimable
> objects only".
Thanks for the suggestion. Will incorporate that.

Cheers,
Longman