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

[Vitaly Wool <vitaly.wool@konsulko.se>]

> On Jul 10, 2025, at 2:39 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Wed, Jul 9, 2025 at 4:26 PM Danilo Krummrich <dakr@kernel.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@kernel.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@konsulko.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.
> 

I see your point but your approach is currently only applicable to vmalloc and it will not work for slub because the latter already has __kmalloc_node, __kvmalloc_node etc. and we want to keep at least some naming consistency between k[v]* and v* functions.

This whole patchset is only intended to add the capability to set node and properly handle alignment requests in Rust allocators, and is thus well aligned with your idea that the new code must support NUMA (which I do share). I would suggest that we get this in as it is, and then I can take the burden of straightening out the naming which will inevitably lead to many modifications in other parts of the kernel. The latter I am fine with, too, but not in this series.

~Vitaly