On 5/5/21 2:02 PM, Vlastimil Babka wrote:

On 5/5/21 7:30 PM, Roman Gushchin wrote:

quoted

On Wed, May 05, 2021 at 11:46:13AM -0400, Waiman Long wrote:

quoted

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.

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.

Good point. But looks like kmalloc* caches are extempt from all merging in
create_boot_cache() via

	s->refcount = -1;       /* Exempt from merging for now */

It wouldn't hurt though to create the kmalloc-cg-* caches with SLAB_ACCOUNT flag
to prevent accidental merging in case the above is ever removed. It would also
better reflect reality, and ensure that the array is allocated immediately with
the page, AFAICS.

I am not sure if this is really true.

struct kmem_cache *__init create_kmalloc_cache(const char *name,
                 unsigned int size, slab_flags_t flags,
                 unsigned int useroffset, unsigned int usersize)
{
         struct kmem_cache *s = kmem_cache_zalloc(kmem_cache, GFP_NOWAIT);

         if (!s)
                 panic("Out of memory when creating slab %s\n", name);

         create_boot_cache(s, name, size, flags, useroffset, usersize);
         kasan_cache_create_kmalloc(s);
         list_add(&s->list, &slab_caches);
         s->refcount = 1;
         return s;
}

Even though refcount is set to -1 initially, it is set back to 1 
afterward. So merging can still happen AFAICS.

Cheers,
Longman