Re: [PATCH v9 3/4] rust: add support for NUMA ids in allocations

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Jul 1, 2025 at 1:19 PM Vitaly Wool <vitaly.wool@konsulko.se> wrote:
>
>
>
> > On Jul 1, 2025, at 12:34 PM, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Tue, Jul 01, 2025 at 12:16:40AM +0200, Vitaly Wool wrote:
> >> Add a new type to support specifying NUMA identifiers in Rust
> >> allocators and extend the allocators to have NUMA id as a
> >> parameter. Thus, modify ReallocFunc to use the new extended realloc
> >> primitives from the C side of the kernel (i. e.
> >> k[v]realloc_node_align/vrealloc_node_align) and add the new function
> >> alloc_node to the Allocator trait while keeping the existing one
> >> (alloc) for backward compatibility.
> >>
> >> This will allow to specify node to use for allocation of e. g.
> >> {KV}Box, as well as for future NUMA aware users of the API.
> >>
> >> Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
> >
> > My main feedback is that we should consider introducing a new trait
> > instead of modifying Allocator. What we could do is have a NodeAllocator
> > trait that is a super-trait of Allocator and has additional methods with
> > a node parameter.
> >
> > A sketch:
> >
> > pub unsafe trait NodeAllocator: Allocator {
> >    fn alloc_node(layout: Layout, flags: Flags, nid: NumaNode)
> >                -> Result<NonNull<[u8]>, AllocError>;
> >
> >    unsafe fn realloc_node(
> >        ptr: Option<NonNull<u8>>,
> >        layout: Layout,
> >        old_layout: Layout,
> >        flags: Flags,
> >        nid: NumaNode,
> >    ) -> Result<NonNull<[u8]>, AllocError>;
> > }
> >
> > By doing this, it's possible to have allocators that do not support
> > specifying the numa node which only implement Allocator, and to have
> > other allocators that implement both Allocator and NumaAllocator where
> > you are able to specify the node.
> >
> > If all allocators in the kernel support numa nodes, then you can ignore
> > this.
>
> This is an elegant solution indeed but I think that keeping the existing approach goes better with the overall kernel trend of having better NUMA support. My point is, if we add NodeAllocator as a super-trait and in a foreseeable future all the Rust allocators will want/be required to support NUMA (which is likely to happen), we’ll have to “flatten” the traits and effectively go back to the approach expressed in this patch.

If we are not going to have allocators without numa support, then what
you did is reasonable. Though in that case I would consider just
changing the existing methods instead of having methods both with and
without a numa node argument.

Alice