linux-kernel - Re: [PATCH v12 1/4] mm/vmalloc: allow to set node and align in vrealloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DB8ABBBNMMVB.1QWW942P0MRJP@kernel.org>
Date: Thu, 10 Jul 2025 11:57:50 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alexei Starovoitov" <alexei.starovoitov@...il.com>
Cc: "Vitaly Wool" <vitaly.wool@...sulko.se>, "linux-mm"
 <linux-mm@...ck.org>, "Andrew Morton" <akpm@...ux-foundation.org>, "LKML"
 <linux-kernel@...r.kernel.org>, "Uladzislau Rezki" <urezki@...il.com>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Vlastimil Babka" <vbabka@...e.cz>,
 "rust-for-linux" <rust-for-linux@...r.kernel.org>, "Lorenzo Stoakes"
 <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 "Kent Overstreet" <kent.overstreet@...ux.dev>,
 <linux-bcachefs@...r.kernel.org>, "bpf" <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

On Thu Jul 10, 2025 at 2:39 AM CEST, Alexei Starovoitov wrote:
> On Wed, Jul 9, 2025 at 4:26 PM Danilo Krummrich <dakr@...nel.org> wrote:
>>
>> On Thu Jul 10, 2025 at 1:14 AM CEST, Alexei Starovoitov wrote:
>> > On Wed, Jul 9, 2025 at 3:57 PM Danilo Krummrich <dakr@...nel.org> wrote:
>> >>
>> >> On 7/10/25 12:53 AM, Alexei Starovoitov wrote:
>> >> > On Wed, Jul 9, 2025 at 10:25 AM Vitaly Wool <vitaly.wool@...sulko.se> wrote:
>> >> >>
>> >> >>
>> >> >> -void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)
>> >> >> +void *vrealloc_node_align_noprof(const void *p, size_t size, unsigned long align,
>> >> >> +                                gfp_t flags, int node)
>> >> >>   {
>> >> >
>> >> > imo this is a silly pattern to rename functions because they
>> >> > got new arguments.
>> >> > The names of the args are clear enough "align" and "node".
>> >> > I see no point in adding the same suffixes to a function name.
>> >> > In the future this function will receive another argument and
>> >> > the function would be renamed again?!
>> >> > "_noprof" suffix makes sense, since it's there for alloc_hooks,
>> >> > but "_node_align_" is unnecessary.
>> >>
>> >> Do you have an alternative proposal given that we also have vrealloc() and
>> >> vrealloc_node()?
>> >
>> > vrealloc_node()?! There is no such thing in the tree.
>> > There are various k[zm]alloc_node() which are artifacts of the past
>> > when NUMA just appeared and people cared about CONFIG_NUMA vs not.
>> > Nowadays NUMA is everywhere and any new code must support NUMA
>> > from the start. Hence no point in carrying old baggage and obsolete names.
>>
>> This patch adds it; do you suggest to redefine vrealloc_noprof() to take align
>> and nid? If we don't mind being inconsistent with krealloc_noprof() and
>> kvrealloc_noprof() that's fine I guess.
>>
>> FWIW, I prefer consistency.
>
> What inconsistency are you talking about? That
> krealloc_noprof(const void *p, size_t new_size, gfp_t flags)
> and
> vrealloc_noprof(const void *p, size_t size, unsigned long align,
>                 gfp_t flags, int node)
> have different number of arguments?!
>
> See:
> alloc_pages_noprof(gfp_t gfp, unsigned int order);
> __alloc_pages_noprof(gfp_t gfp, unsigned int order, int preferred_nid,
>                 nodemask_t *nodemask);
>
> Adding double underscore to keep all existing callers of
> vrealloc_noprof() without changes and do:
>
> vrealloc_noprof(const void *p, size_t size, gfp_t flags);
> __vrealloc_noprof(const void *p, size_t size, unsigned long align,
> gfp_t flags, int node);
>
> is fine and consistent with how things were done in the past,
> but adding "_node_align_" to the function name and code churn to all
> callsites is a cargo cult.

As Vitaly mentioned in a different reply, this would be inconsistent with the
'k' and 'kv' variants, which have the suffix '_node'.

Anyways, in general I don't think that adding underscores for functions that
basically do the same thing but are getting more specialized is a great pattern
for things that are not strictly limited to a narrow context.

Please note, I'm not saying we should encode additional arguments in the name
either. I think it really depends on the actual case.

In this case, it seems to make sense to me that there is e.g. kmalloc() and
kmalloc_node().

For a caller that's much more useful, i.e. I want classic kmalloc(), but want to
set the node, hence kmalloc_node(). Calling it __kmalloc() instead seems a bit
random.

Or do you only refer to the *_noprof() variants, which are not exported to
users? But even then, underscores still don't seem very expressive.

I'm not maintaining this code though, so just take it FWIW. :)