Message-ID: <3ohcdmztpnxuxhsp23ixpu4kfctbuqju7ju2oykeficsumtyqo@etxvrmrhxvue>
Date: Thu, 10 Jul 2025 11:09:40 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Vitaly Wool <vitaly.wool@...sulko.se>
Cc: linux-mm@...ck.org, akpm@...ux-foundation.org,
        linux-kernel@...r.kernel.org, Uladzislau Rezki <urezki@...il.com>,
        Danilo Krummrich <dakr@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, rust-for-linux@...r.kernel.org,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Kent Overstreet <kent.overstreet@...ux.dev>,
        linux-bcachefs@...r.kernel.org, bpf@...r.kernel.org,
        Herbert Xu <herbert@...dor.apana.org.au>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>
Subject: Re: [PATCH v12 1/4] mm/vmalloc: allow to set node and align in
 vrealloc

* Vitaly Wool <vitaly.wool@...sulko.se> [250710 02:21]:
> 
> 
> > On Jul 9, 2025, at 9:01 PM, Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
> > 
> > * Vitaly Wool <vitaly.wool@...sulko.se <mailto:vitaly.wool@...sulko.se>> [250709 13:24]:
> >> Reimplement vrealloc() to be able to set node and alignment should
> >> a user need to do so. Rename the function to vrealloc_node_align()
> >> to better match what it actually does now and introduce macros for
> >> vrealloc() and friends for backward compatibility.
> >> 
> >> With that change we also provide the ability for the Rust part of
> >> the kernel to set node and alignment in its allocations.
> >> 
> >> Signed-off-by: Vitaly Wool <vitaly.wool@...sulko.se>
> >> Reviewed-by: Uladzislau Rezki (Sony) <urezki@...il.com>
> >> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
> >> ---
> >> include/linux/vmalloc.h | 12 +++++++++---
> >> mm/nommu.c              |  3 ++-
> >> mm/vmalloc.c            | 31 ++++++++++++++++++++++++++-----
> >> 3 files changed, 37 insertions(+), 9 deletions(-)
> >> 
> > ...
> > 
> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >> index 6dbcdceecae1..03dd06097b25 100644
> >> --- a/mm/vmalloc.c
> >> +++ b/mm/vmalloc.c
> >> @@ -4089,19 +4089,31 @@ void *vzalloc_node_noprof(unsigned long size, int node)
> >> EXPORT_SYMBOL(vzalloc_node_noprof);
> >> 
> >> /**
> >> - * vrealloc - reallocate virtually contiguous memory; contents remain unchanged
> >> + * vrealloc_node_align_noprof - reallocate virtually contiguous memory; contents
> >> + * remain unchanged
> >>  * @p: object to reallocate memory for
> >>  * @size: the size to reallocate
> >> + * @align: requested alignment
> >>  * @flags: the flags for the page level allocator
> >> + * @nid: node number of the target node
> >> + *
> >> + * If @p is %NULL, vrealloc_XXX() behaves exactly like vmalloc(). If @size is
> >> + * 0 and @p is not a %NULL pointer, the object pointed to is freed.
> >>  *
> >> - * If @p is %NULL, vrealloc() behaves exactly like vmalloc(). If @size is 0 and
> >> - * @p is not a %NULL pointer, the object pointed to is freed.
> >> + * if @nid is not NUMA_NO_NODE, this function will try to allocate memory on
> >> + * the given node. If reallocation is not necessary (e. g. the new size is less
> >> + * than the current allocated size), the current allocation will be preserved
> >> + * unless __GFP_THISNODE is set. In the latter case a new allocation on the
> >> + * requested node will be attempted.
> > 
> > I am having a very hard time understanding what you mean here.  What is
> > the latter case?
> > 
> > If @nis is !NUMA_NO_NODE, the allocation will be attempted on the given
> > node.  Then things sort of get confusing.  What is the latter case?
> 
> The latter case is __GFP_THISNODE present in flags. That’s the latest if-clause in this paragraph.
> > 
> >>  *
> >>  * If __GFP_ZERO logic is requested, callers must ensure that, starting with the
> >>  * initial memory allocation, every subsequent call to this API for the same
> >>  * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that
> >>  * __GFP_ZERO is not fully honored by this API.
> >>  *
> >> + * If the requested alignment is bigger than the one the *existing* allocation
> >> + * has, this function will fail.
> >> + *
> > 
> > It might be better to say something like:
> > Requesting an alignment that is bigger than the alignment of the
> > *existing* allocation will fail.
> > 
> 
> The whole function description in fact consists of several if-clauses (some of which are nested) so I am just following the pattern here.
> 

I'm not trying to be difficult but I am trying to nicely say that this
is a mess of words.  It's easier to just go find the code and try and
figure out what is going on instead of decoding what is written in the
description.

The pattern garbage.  I was trying to tell you this nicely, but your
change is better left undocumented than to add the above description.

The wording here is difficult to follow for a native English speaker.

You don't have to keep stating "this function", that wasn't part of the
pattern.

There are too many unnecessary negative statements "if @nis is not
NUMA_NO_NODE" or "if reallocation is not necessary".

The commas indicate that you can rewrite the sentences to be more clear
by stating the facts up front.

Stating "the latter case" is a barrier to non-native English speakers
and is unclear when you have multiple statements crammed into the
preceding sentence.

And it's easy to fix, but apparently you don't want to change it and
would rather follow a broken pattern?

Regards,
Liam

Powered by blists - more mailing lists