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

On Wed, Jul 9, 2025 at 4:26 PM Danilo Krummrich <[email protected]> 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 <[email protected]> wrote:
> >>
> >> On 7/10/25 12:53 AM, Alexei Starovoitov wrote:
> >> > On Wed, Jul 9, 2025 at 10:25 AM Vitaly Wool <[email protected]> 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.