Re: [PATCH RFC 00/15] mm/slab: introduce alloc_flags and slab_alloc_context

[Harry Yoo]

On 6/10/26 11:04 PM, Vlastimil Babka (SUSE) wrote:
> On 6/10/26 15:46, Harry Yoo wrote:
>>
>>
>> On 6/9/26 6:17 PM, Vlastimil Babka (SUSE) wrote:
>>> This series is based on slab/for-next. If all goes well, it would
>>> hopefully go to slab/for-next soon after the 7.2 merge window, so any
>>> other work can be based on it to avoid conflicts, as it touches a lot
>>> parts of slab.
>>>
>>> Git: https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=b4/slab_alloc_flags
>>>
>>> The slab implementation currently relies on gfp flags to convey
>>> some context information internally:
>>>
>>> - The absence of both __GFP_RECLAIM flags is interpreted as "cannot spin
>>>   on locks", and intended to be used by kmalloc_nolock(). But false
>>>   positives are possible e.g. during early boot where gfp_allowed_mask
>>>   clears __GFP_RECLAIM from all allocations. This leads to unnecessary
>>>   allocation failures and workarounds such as fd3634312a04 ("debugobject:
>>>   Make it work with deferred page initialization - again").
>>>
>>> - __GFP_NO_OBJ_EXT exists and takes up valuable bit in the gfp flags
>>>   space, only to prevent recursive kmalloc() allocations for obj_ext
>>>   arrays and sheaves.
>>
>> [ Cc'ing Vishal and Matthew as it's somewhat relevant to memdescs... ]
>>
>> When the page allocator starts allocateing slab objects,
>> we still need a way to avoid recursion for obj_ext arrays and sheaves
>> (by passing SLAB_ALLOC_NO_RECURSE).
>>
>> Looking at kmalloc_flags(), probably we'll end up introducing a separate
>> gfp type for slab-specific flags?
> 
> What do you mean by separate gfp type?

I meant the patchset is introducing a new type to specify the context
(specific to slab) other than gfp_t... which is `unsigned int
alloc_flags` now.

>> Hmm but SLAB_ALLOC_* flags are defined in mm/slab.h and kmalloc_flags()
>> is defined in include/linux/slab.h. Do yo intend to restrict the slab
>> alloc flags to MM only?
> 
> Yeah I don't expect users outside MM. If a valid one appears, we can move
> it. I should try moving kmalloc_flags() to mm/slab.h as well, unless there's
> some header dependency issue that will prevent it.

Ack.

>>> The page allocator uses its internal alloc_flags to convey various
>>> context information, including ALLOC_TRYLOCK (meaning "cannot spin").
>>> This series copies that concept for the slab allocator, with its own
>>> slab-specific internal flags:
>>>
>>> - SLAB_ALLOC_DEFAULT - no extra flags (the value is 0), but explicit
>>> - SLAB_ALLOC_TRYLOCK - do not spin on locks (used by kmalloc_nolock())
>>> - SLAB_ALLOC_NEW_SLAB - replacing existing 'bool new_slab' parameter
>>> 			for allocating obj_ext arrays
>>> - SLAB_ALLOC_NO_RECURSE - replacing usage of __GFP_NO_OBJ_EXT
>>>
>>> To reduce the amount of parameters in various internal functions, we
>>> additionally introduce slab_alloc_context (also inspired by page
>>> allocator's alloc_context) for passing a number of existing arguments
>>> and the new alloc_flags:
>>>
>>> /* Structure holding extra parameters for slab allocations */
>>> struct slab_alloc_context {
>>> 	unsigned long caller_addr;
>>> 	unsigned long orig_size;
>>> 	unsigned int alloc_flags;
>>> 	struct list_lru *lru;
>>> };
>>
>> Perhaps beyond the scope of the patchset, but I wonder if we could have
>> something like struct slab_alloc_context but for kmalloc callers to
>> simplify {PASS,DECL}_KMALLOC_PARAMS().
>>
>> Something like:
>>
>> struct kmalloc_params {
>> #ifdef CONFIG_SLAB_BUCKETS
>> 	kmem_buckets *b;
>> #endif
>> #ifdef CONFIG_KMALLOC_PARTITION_CACHES
>> 	kmalloc_token_t token;
>> #endif
>> };
>>
>> The idea is to move optional kmalloc parameters (depending on config)
>> into a single struct, instead of using the macros.
>>
>> void *__kmalloc_node(size_t size, gfp_t flags, int node,
>> 		     unsigned long caller,
>> 		     struct kmalloc_params params);
>>
>> void *kmalloc_node() {
>>     /* ... snip ...*/
>>     struct kmalloc_params params = KMALLOC_PARAMS(params.b, params.token);
>>     return __kmalloc_node(size, flags, node, _RET_IP_, params);
>> }
>>
>> The compiler should optimize away unused fields based on the config.
>>
>> Per System V AMD64 ABI, the compiler will use registers to pass the
>> struct, as long as the struct size does not exceed 16 bytes.
>> (Otherwise it will be passed on stack).
> 
> Hm but does this work on all architectures,

apparently not on s390, unfortunately.
on s390 it works only when the struct size does not exceed 8 bytes.

> and are we doing this somewhere
> (for structures larger than a native word) already?

hmm perhaps struct timespec64?

> Also Marco noted earlier that gcc won't optimize away the struct if it
> becomes zero-sized:
> 
> https://lore.kernel.org/all/CANpmjNO1aNm3mKphDGWasK_NUfVY8q4K9GCjyREZFqrOu9WLcw@xxxxxxxxxxxxxx/

Ouch, right. That means we still need at least one macro to define those
parameters :( Sounds less promising now...

-- 
Cheers,
Harry / Hyeonggon

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature