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

[Danilo Krummrich <dakr@redhat.com>]

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?

Is it possible for rustfmt to change rules unexpectedly? If so, how would we
deal with that?

> 
> 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.

> 
> > +
> > +    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?

> 
> -- 
> Cheers,
> Benno
> 
> > +
> > +    fn default_allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL | __GFP_ZERO) }
> > +    }
> > +}
> > --
> > 2.44.0
> > 
>