Re: [RFC PATCH] mm/slab: save memory by allocating slabobj_ext array from leftover

[Vlastimil Babka <vbabka@suse.cz>]

On 8/5/25 13:57, Harry Yoo wrote:
> On Thu, Jun 19, 2025 at 09:56:59AM +0200, Vlastimil Babka wrote:
>> On 6/13/25 19:47, Harry Yoo wrote:
>> > On Fri, Jun 13, 2025 at 09:04:34AM -0700, Christoph Lameter (Ampere) wrote:
>> >> On Fri, 13 Jun 2025, Harry Yoo wrote:
>> >> 
>> >> > Allocate slabobj_exts array from this unused space instead of using
>> >> > kcalloc(), when it is large enough.
>> >> 
>> >> How does slab debug work in this case? The object layout gets a bit
>> >> complicated with other metadata there as well.
>> > 
>> > Oh, the 'leftover' space I mentioned the cover letter refers to the
>> > wasted space after the last object in a slab, not unused bytes within
>> > objects.
>> > 
>> > There is no per-object metadata stored there and SLUB simply poisons the area.
>> > I taught slab_pad_check() to skip checking the slabobj_exts array.
> 
> Apologies for the late reply. I was sidetracked with multiple things :(
> 
> This is definitely worth optimizing, so let me make some progress
> even if it's a bit slow.
> 
>> I can imagine going further with this in case where leftover space in slab
>> isn't enough.
> 
> Right.
> 
>> - indeed use per-object padding to store only single object's slabobj_ext,
> 
> I think the most conservative approach is to not increase object_size
> but use wasted area when ALIGN(size, align) is bigger than object_size.
> 
> A good candidate for that is xfs inode cache.

Yeah that's what I meant.

>> if it doesn't lead to memory waste
> 
> You mean increasing object size but without decreasing the number of
> objects per slab?

Just the ALIGN thing. It we can increase size without decreasing objects it
means we can also just use leftover space for the array, IIUC?

> ...or (maybe) reducing the number of objects but without increasing
> the size of the remainder (same as calculated in calc_slab_order())?
> 
>> - if not possible, but object size is small enough so there are many per
>> slab, maybe have one less object per slab to store the array?
> 
> If object size is small the array likely does not fit in one object...

Hmm perhaps.

>> - once we have struct slab decoupled from struct page, it could be part of
>> struct slab directly (but it would mean struct slab isn't fixed size)
> 
> It can be tried, but variable struct slab size may or may not work.
> That'll depend on the implementation details of how we allocate struct slab
> in the future.
> 
>> Of course having multiple variants would risk slower code, so fast paths
>> should not be affected 
> 
> I agree that affecting fastpath is not great.

So we'll have to measure it.

>> we could have pointer to the 0th slabobj_ext (we
>> already have) and now also stride (to support the "per-object padding case"
> 
>> - there's still space in struct slab right?)
> 
> which space are you referring to, maybe lower 16 bits of page_type?

Ugh I thought counters used only 32bit of 64... so the space exists only for
64bit kernels? It would be fine to limit the optimization to those only.

>> and then the object alloc/free
>> case could be oblivious to the storage method, with just a bit more
>> arithmetic (stride). Slab folio alloc/free would be more complicated but are
>> not fath path.
> 
> So it would be something like (please correct if I misunderstood):
> 
> index = obj_to_index(s, slab, object)
> (the struct slabobj_ext pointer for the object at given index)
>   == slab->obj_exts + stride * index

Yeah.

> 
> slab->obj_exts, stride are determined depending on the case:
> 
> - In the normal case (the array is allocated from kmalloc caches),
>   stride = sizeof(struct slabobj_ext)
>   slab->obj_exts = (the address of the buffer allocated from kmalloc)
> 
> - In "the obj_exts array is stored in the leftover space" case,
>   stride = sizeof(struct slabobj_ext)
>   slab->obj_exts = (the start address of the leftover space)
> 
> - In "per-object padding" case,
>   stride = s->size
>   slab->obj_exts = slab_address(slab) + s->red_left_pad +
>                        (offset of slabobj_ext);
> 
> Ok, I think it will work. Great idea!

Yeah.

> 
>> Also some variants would be wasteful if they need to be decided upfront (the
>> 2nd and 3rd above) and then the array is unused
> 
> Right.
> 
>> so would be only applicable
>> with SLAB_ACCOUNT caches (if kmemcg is active) or when memalloc profiling is
>> active.
> 
> Right.
> 
>> Shouldn't be a big issue as ad-hoc __GFP_ACCOUNT is handled by
>> different cache selection for kmalloc() and I don't know if anyone is
>> actually doing ad-hoc __GFP_ACCOUNT on named caches.
> 
> A while ago I was thinking of getting rid of ad-hoc __GFP_ACCOUNT usage
> for slab allocations, but at least xarray cache appears to use it in
> an ad-hoc manner. (See xas_nomem()).
> 
> In that case, not all allocations to the same cache has
> __GFP_ACCOUNT flag set.

Hmm bummer.