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

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

[Alexander Aring]

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.

Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
changes since v2:
 - don't make a separate sentence for except

 mm/slab.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 10e96137b44f..a86879f42f2e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3482,7 +3482,8 @@ void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
  * @flags: See kmalloc().
  *
  * Allocate an object from this cache.  The flags are only relevant
- * if the cache has no available objects.
+ * if the cache has no available objects, except flag __GFP_ZERO which
+ * will zero the returned object.
  *
  * Return: pointer to the new object or %NULL in case of error
  */
-- 
2.31.1

[Cluster-devel] [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Alexander Aring]

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.

Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
changes since v2:
 - don't make a separate sentence for except

 mm/slab.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 10e96137b44f..a86879f42f2e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3482,7 +3482,8 @@ void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
  * @flags: See kmalloc().
  *
  * Allocate an object from this cache.  The flags are only relevant
- * if the cache has no available objects.
+ * if the cache has no available objects, except flag __GFP_ZERO which
+ * will zero the returned object.
  *
  * Return: pointer to the new object or %NULL in case of error
  */
-- 
2.31.1

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

[Vlastimil Babka]

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.
- 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"

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?

> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
> changes since v2:
>  - don't make a separate sentence for except
> 
>  mm/slab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 10e96137b44f..a86879f42f2e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3482,7 +3482,8 @@ void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
>   * @flags: See kmalloc().
>   *
>   * Allocate an object from this cache.  The flags are only relevant
> - * if the cache has no available objects.
> + * if the cache has no available objects, except flag __GFP_ZERO which
> + * will zero the returned object.
>   *
>   * Return: pointer to the new object or %NULL in case of error
>   */

[Cluster-devel] [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Vlastimil Babka]

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.
- 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"

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?

> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
> changes since v2:
>  - don't make a separate sentence for except
> 
>  mm/slab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 10e96137b44f..a86879f42f2e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3482,7 +3482,8 @@ void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
>   * @flags: See kmalloc().
>   *
>   * Allocate an object from this cache.  The flags are only relevant
> - * if the cache has no available objects.
> + * if the cache has no available objects, except flag __GFP_ZERO which
> + * will zero the returned object.
>   *
>   * Return: pointer to the new object or %NULL in case of error
>   */

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

[Alexander Aring]

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.

- Alex

[Cluster-devel] [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Alexander Aring]

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.

- Alex

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

[Vlastimil Babka]

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

[Cluster-devel] [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Vlastimil Babka]

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

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

[Alexander Aring]

Hi,

On Thu, Nov 10, 2022 at 3:37 AM Vlastimil Babka <vbabka@suse.cz> wrote:
...
>
> 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
>

perfect, thanks!

- Alex

[Cluster-devel] [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

[Alexander Aring]

Hi,

On Thu, Nov 10, 2022 at 3:37 AM Vlastimil Babka <vbabka@suse.cz> wrote:
...
>
> 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
>

perfect, thanks!

- Alex