Re: [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Vlastimil Babka <vbabka@suse.cz>]

On 10/14/22 13:59, Alexander Aring wrote:
> Hi,
> 
> On Fri, Oct 14, 2022 at 3:35 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 10/11/22 16:54, Alexander Aring wrote:
>> > This patch will add a comment for the __GFP_ZERO flag case for
>> > kmem_cache_alloc(). As the current comment mentioned that the flags only
>> > matters if the cache has no available objects it's different for the
>> > __GFP_ZERO flag which will ensure that the returned object is always
>> > zeroed in any case.
>> >
>> > I have the feeling I run into this question already two times if the
>> > user need to zero the object or not, but the user does not need to zero
>> > the object afterwards. However another use of __GFP_ZERO and only zero
>> > the object if the cache has no available objects would also make no
>> > sense.
>>
>> Hmm, but even with the update, the comment is still rather misleading, no?
>> - can the caller know if the cache has available objects and thus the flags
>> are irrelevant, in order to pass flags that are potentially wrong (if there
>> were no objects)? Not really.
> 
> No, the caller cannot know it and that's why __GFP_ZERO makes no sense
> if they matter only if the cache has no available objects.
> 
>> - even if cache has available objects, we'll always end up in
>> slab_pre_alloc_hook doing might_alloc(flags) which will trigger warnings
>> (given CONFIG_DEBUG_ATOMIC_SLEEP etc.) if the flags are inappropriate for
>> given context. So they are still "relevant"
>>
> 
> yes, so they are _always_ relevant in the current implementation. Also
> as you said the user doesn't know when they become relevant or not..
> 
>> So maybe just delete the whole comment? slub.c doesn't have it, and if any
>> such comment should exist for kmem_cache_alloc() and contain anything useful
>> and not misleading, it should be probably in include/linux/slab.h anyway?
>>
> 
> ctags brought me there, but this isn't a real argument why it should
> not be in the header file...
> 
> I am not sure about deleting the whole comment as people have an vague
> idea about how kmem_cache works and still need to know for __GFP_ZERO
> that it will always zero the memory, but thinking again somebody will
> make the conclusion it does not make sense as the user doesn't know
> when objects are reused or allocated. Having that in mind and reading
> the current comment was making me do more investigations into the
> internal behaviour to figure out how it works regarding __GFP_ZERO.

So, I did the following, which IMHO resolves the misleading parts and also
mentions __GFP_ZERO. Sounds OK?

https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.2/cleanups&id=d6a3a7c3f65dfebcbc4872d5912d3465c8e8b051