Message-ID: <2c322b00-20f8-4102-9f3b-edab0c0907b9@proton.me>
Date: Sat, 06 Jul 2024 13:17:19 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...hat.com>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...sung.com, aliceryhl@...gle.com, daniel.almeida@...labora.com, faith.ekstrand@...labora.com, boris.brezillon@...labora.com, lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com, acurrid@...dia.com, cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 01/20] rust: alloc: add `Allocator` trait

On 06.07.24 13:05, Danilo Krummrich wrote:
> On Sat, Jul 06, 2024 at 10:33:49AM +0000, Benno Lossin wrote:
>> On 04.07.24 19:06, Danilo Krummrich wrote:
>>> +pub unsafe trait Allocator {
>>> +    /// Allocate memory based on `layout` and `flags`.
>>> +    ///
>>> +    /// On success, returns a buffer represented as `NonNull<[u8]>` that satisfies the size an
>>
>> typo "an" -> "and"
>>
>>> +    /// alignment requirements of layout, but may exceed the requested size.
>>
>> Also if it may exceed the size, then I wouldn't call that "satisfies the
>> size [...] requirements".
> 
> Do you have a better proposal? To me "satisfies or exceeds" sounds reasonable.

I think "satisfies the layout constraints (i.e. minimum size and
alignment as specified by `layout`)" would be better.

>>> +    ///
>>> +    /// This function is equivalent to `realloc` when called with a NULL pointer and an `old_size`
>>> +    /// of `0`.
>>
>> This is only true for the default implementation and could be
>> overridden, since it is not a requirement of implementing this trait to
>> keep it this way. I would remove this sentence.
> 
> I could add a bit more generic description and say that for the default impl
> "This function is eq..."?
> 
>>
>>> +    fn alloc(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
>>
>> Instead of using the `Flags` type from the alloc module, we should have
>> an associated `Flags` type in this trait.
> 
> What does this give us?

1. IIRC not all flags can be used with every allocator (or do not have
   an effect) and it would be best if only the working ones are allowed.
2. that way the design is more flexible and could be upstreamed more
   easily.

>> Similarly, it might also be a good idea to let the implementer specify a
>> custom error type.
> 
> Same here, why?

In this case the argument is weaker, but it could allow us to implement
an allocator with `Error = Infallible`, to statically guarantee
allocation (e.g. when using GFP_ATOMIC). But at the moment there is no
user.

>>> +        // SAFETY: Passing a NULL pointer to `realloc` is valid by it's safety requirements and asks
>>> +        // for a new memory allocation.
>>> +        unsafe { self.realloc(ptr::null_mut(), 0, layout, flags) }
>>> +    }
>>> +
>>> +    /// Re-allocate an existing memory allocation to satisfy the requested `layout`. If the
>>> +    /// requested size is zero, `realloc` behaves equivalent to `free`.
>>
>> This is not guaranteed by the implementation.
> 
> Not sure what exactly you mean? Is it about "satisfy" again?

If the requested size is zero, the implementation could also leak the
memory, nothing prevents me from implementing such an Allocator.

>>> +    ///
>>> +    /// If the requested size is larger than `old_size`, a successful call to `realloc` guarantees
>>> +    /// that the new or grown buffer has at least `Layout::size` bytes, but may also be larger.
>>> +    ///
>>> +    /// If the requested size is smaller than `old_size`, `realloc` may or may not shrink the
>>> +    /// buffer; this is implementation specific to the allocator.
>>> +    ///
>>> +    /// On allocation failure, the existing buffer, if any, remains valid.
>>> +    ///
>>> +    /// The buffer is represented as `NonNull<[u8]>`.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// `ptr` must point to an existing and valid memory allocation created by this allocator
>>> +    /// instance of a size of at least `old_size`.
>>> +    ///
>>> +    /// Additionally, `ptr` is allowed to be a NULL pointer; in this case a new memory allocation is
>>> +    /// created.
>>> +    unsafe fn realloc(
>>> +        &self,
>>> +        ptr: *mut u8,
>>> +        old_size: usize,
>>
>> Why not request the old layout like the std Allocator's grow/shrink
>> functions do?
> 
> Because we only care about the size that needs to be preserved when growing the
> buffer. The `alignment` field of `Layout` would be wasted.

In the std Allocator they specified an old layout. This is probably
because of the following: if `Layout` is ever extended to hold another
property that would need to be updated, the signatures are already
correct.
In our case we could change it tree-wide, so I guess we could fix that
issue when it comes up.

>>> +        layout: Layout,
>>> +        flags: Flags,
>>> +    ) -> Result<NonNull<[u8]>, AllocError>;
>>> +
>>> +    /// Free an existing memory allocation.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// `ptr` must point to an existing and valid memory allocation created by this `Allocator`
>>> +    /// instance.
>>> +    unsafe fn free(&self, ptr: *mut u8) {
>>
>> `ptr` should be `NonNull<u8>`.
> 
> Creating a `NonNull` from a raw pointer is an extra operation for any user of
> `free` and given that all `free` functions in the kernel accept a NULL pointer,
> I think there is not much value in making this `NonNull`.

I don't think that this argument holds for Rust though. For example,
`KBox` contains a `Unique` that contains a `NonNull`, so freeing could
just be done with `free(self.0.0)`.

>>> +        // SAFETY: `ptr` is guaranteed to be previously allocated with this `Allocator` or NULL.
>>> +        // Calling `realloc` with a buffer size of zero, frees the buffer `ptr` points to.
>>> +        let _ = unsafe { self.realloc(ptr, 0, Layout::new::<()>(), Flags(0)) };
>>
>> Why does the implementer have to guarantee this?
> 
> Who else can guarantee this?

Only the implementer yes. But they are not forced to do this i.e.
nothing in the safety requirements of `Allocator` prevents me from doing
a no-op on reallocating to a zero size.

>>> +    }
>>> +}
>>> --
>>> 2.45.2
>>>
>>
>> More general questions:
>> - are there functions in the kernel to efficiently allocate zeroed
>>   memory? In that case, the Allocator trait should also have methods
>>   that do that (with a iterating default impl).
> 
> We do this with GFP flags. In particular, you can pass GFP_ZERO to `alloc` and
> `realloc` to get zeroed memory. Hence, I think having dedicated functions that
> just do "flags | GFP_ZERO" would not add much value.

Ah right, no in that case, we don't need it.

>> - I am not sure putting everything into the single realloc function is a
>>   good idea, I like the grow/shrink methods of the std allocator. Is
>>   there a reason aside from concentrating the impl to go for only a
>>   single realloc function?
> 
> Yes, `krealloc()` already provides exactly the described behaviour. See the
> implementation of `Kmalloc`.

But `kvmalloc` does not and neither does `vmalloc`. I would prefer
multiple smaller functions over one big one in this case.

---
Cheers,
Benno

Powered by blists - more mailing lists