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

[Benno Lossin <benno.lossin@proton.me>]

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.
2. Print a warning at runtime and allocate regardless. Additionally use
    `klint` to lint against usage of that function.
3. Use `build_error!`.

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.

-- 
Cheers,
Benno