Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Danilo Krummrich <dakr@redhat.com>]

On 5/1/24 17:39, Benno Lossin wrote:
> On 01.05.24 14:50, Danilo Krummrich wrote:
>> On Wed, May 01, 2024 at 08:32:01AM +0000, Benno Lossin wrote:
>>> On 29.04.24 22:11, Danilo Krummrich wrote:
>>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>>> ---
>>>>    rust/kernel/alloc.rs | 21 +++++++++++++++++++++
>>>>    1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>>> index 9bc1b48b5641..ec45ba8f77ce 100644
>>>> --- a/rust/kernel/alloc.rs
>>>> +++ b/rust/kernel/alloc.rs
>>>> @@ -8,6 +8,14 @@
>>>>    pub mod box_ext;
>>>>    pub mod vec_ext;
>>>>
>>>> +use core::{
>>>> +    alloc::{Allocator, AllocError, Layout},
>>>> +    ptr,
>>>> +    ptr::NonNull,
>>>> +};
>>>> +
>>>> +use flags::*;
>>>> +
>>>>    /// Flags to be used when allocating memory.
>>>>    ///
>>>>    /// They can be combined with the operators `|`, `&`, and `!`.
>>>> @@ -68,3 +76,16 @@ pub mod flags {
>>>>        /// small allocations.
>>>>        pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
>>>>    }
>>>> +
>>>> +pub unsafe trait AllocatorWithFlags: Allocator {
>>>> +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>>> +    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>>
>>> Please use `rustfmt`, there exists a make target that does it for you.
>>
>> Sure, gonna fix.
>>
>> Out of curiosity, given that we have this make target, the expectation obviously
>> seems to be that all Rust code in the kernel must be formatted with this. I
>> think that's indeed desirable. Two questions regarding this:
>>
>> How do we scale (as in ensure) this once Rust code becomes more common in
>> subsystems?
> 
> There also is the make target `rustfmtcheck` which can be used to check
> if the code has been formatted correctly (ie it will error on wrongly
> formatted code).
> IIRC there were some plans to add such a check to CI, but I don't know
> the status of that.
> 
>> Is it possible for rustfmt to change rules unexpectedly? If so, how would we
>> deal with that?
> 
> AFAIK, `rustfmt` has a stable format [1] and if we really find
> something that breaks our code, we can also add our own defaults to the
> `.rustfmt.toml`.
> 
> [1]: https://rust-lang.github.io/rfcs/2437-rustfmt-stability.html
> 
>>> Also why are these functions `unsafe`? You haven't specified a `Safety`
>>> section, which means nobody knows what the requirements are.
>>
>> I think I did this because they did inherit the safety requirements from
>> krealloc() (as you also point out in another patch).
>>
>> I think they shouldn't be unsafe, just as the corresponding Allocator ones
>> aren't.
>>
>> Gonna change that.
> 
> Sounds good.
> 
>>>> +
>>>> +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
>>>> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
>>>> +    }
>>>
>>> Why do we want to have these functions? I think that it would make sense
>>> to forbid all other ways of allocating memory. So only allow allocation
>>> where the flags are explicitly passed.
>>
>> I agree that we should forbid allocating memory with implicit default page
>> flags.
>>
>> I added them, since I think we'll always need some implementation of the
>> Allocator trait. And this just was the obvious generic implementation.
>>
>> If we agree to forbid allocating without page flags, we can just always return
>> an error?
> 
> There are multiple options that we could do here:
> 1. Return an error.

I'd WARN() additionally here.

> 2. Print a warning at runtime and allocate regardless. Additionally use
>      `klint` to lint against usage of that function.
> 3. Use `build_error!`.

Agreed, that'd be even better - Will give that a shot.

> 
> I think that 1 could be very unexpected and hard to debug. 2 is better,
> but at the moment `klint` is not required to run, so we would have to
> change that. 3 is my preferred option, but I am not 100% confident that
> it will work.
> 
> So maybe give option 3 a try and if that doesn't work, we can try to
> come up with a different solution.
>