Re: [PATCH v3 2/2] rust: support align and NUMA id in allocations

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

> On Jun 25, 2025, at 9:07 PM, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Wed, Jun 25, 2025 at 08:56:05PM +0200, Danilo Krummrich wrote:
>> On Wed, Jun 25, 2025 at 08:30:26AM +0200, Vitaly Wool wrote:
>>> Add support for large (> PAGE_SIZE) alignments in Rust allocators
>>> (Kmalloc support for large alignments is limited to the requested
>>> size, which is a reasonable limitation anyway).
>> 
>> Please split this..
>> 
>>> Besides, add support for NUMA id to Vmalloc.
>> 
>> and this into separate patches.
>> 
>> Please also add some information to the commit message what you need node
>> support for. Do you also have patches to add node support to Box and Vec?

No, but there is a zswap backend implementation written in Rust and it should be  NUMA id aware.
I’m planning on submitting that basically as soon as this piece gets accepted.

>> 
>>> 
>>> Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
>>> ---
>>> rust/helpers/slab.c            |  8 +++++--
>>> rust/helpers/vmalloc.c         |  4 ++--
>>> rust/kernel/alloc.rs           | 28 ++++++++++++++++++++++--
>>> rust/kernel/alloc/allocator.rs | 40 +++++++++++++++++++---------------
>>> rust/kernel/alloc/kvec.rs      |  3 ++-
>>> 5 files changed, 59 insertions(+), 24 deletions(-)
>>> 
>>> diff --git a/rust/helpers/slab.c b/rust/helpers/slab.c
>>> index a842bfbddcba..221c517f57a1 100644
>>> --- a/rust/helpers/slab.c
>>> +++ b/rust/helpers/slab.c
>>> @@ -3,13 +3,17 @@
>>> #include <linux/slab.h>
>>> 
>>> void * __must_check __realloc_size(2)
>>> -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
>>> +rust_helper_krealloc(const void *objp, size_t new_size, unsigned long align, gfp_t flags, int nid)
>> 
>> This should have a comment making it obvious why the function has two arguments
>> that are discarded. I think we should even separate it with an additional inline
>> function.
>> 
>> I do agree with discarding the align argument, given that it's not exposed to
>> users though the Allocator API.
> 
> What I meant is that proper alignment is implied when krealloc() succeeds.

I agree, I need to add some comments explaining this.

> 
>> I do disagree with discarding the nid argument though, since you change the
>> generic Allocator::realloc() API to take a node argument, which for KREALLOC and
>> KVREALLOC is silently discarded. If we introduce it, we should do so for all
>> three allocators.
>> 
>>> {
>>> + if (WARN_ON(new_size & (align - 1)))
>>> + return NULL;
>> 
>> I don't think we should have this WARN_ON(). If we want to warn about this, we
>> should already do so on the Rust side. The helper functions in this file should
>> not contain any logic.

Agreed.

>> 
>>> return krealloc(objp, new_size, flags);
>>> }
>>> 
>>> void * __must_check __realloc_size(2)
>>> -rust_helper_kvrealloc(const void *p, size_t size, gfp_t flags)
>>> +rust_helper_kvrealloc(const void *p, size_t size, unsigned long align, gfp_t flags, int nid)
>>> {
>>> + if (WARN_ON(size & (align - 1)))
>>> + return NULL;
>>> return kvrealloc(p, size, flags);
>>> }
>> 
>> Same as above.
> 
> This is actually different though, here kvrealloc() may succeed even if the
> requested alignment is not fulfilled, so this is incorrect.

I can move this logic to the Rust part, too. My point here is, for Kvrealloc with a large alignment we’ll just make the decision to use vmalloc, period. We can indeed do that on the Rust side.

~Vitaly