| Message ID | 20240816001216.26575-2-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Generic `Allocator` support for Rust | expand |
On 16.08.24 02:10, Danilo Krummrich wrote: > Add a kernel specific `Allocator` trait, that in contrast to the one in > Rust's core library doesn't require unstable features and supports GFP > flags. > > Subsequent patches add the following trait implementors: `Kmalloc`, > `Vmalloc` and `KVmalloc`. > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the `old_layout` parameter, I can see if I am happy with it. If I am, I would give you my RB. --- Cheers, Benno
On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: > On 16.08.24 02:10, Danilo Krummrich wrote: > > Add a kernel specific `Allocator` trait, that in contrast to the one in > > Rust's core library doesn't require unstable features and supports GFP > > flags. > > > > Subsequent patches add the following trait implementors: `Kmalloc`, > > `Vmalloc` and `KVmalloc`. > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > We discussed this in our weekly meeting (I think ~one week ago?). If you > give me a draft version of the comment that you plan to add regarding > the `old_layout` parameter, I can see if I am happy with it. If I am, I > would give you my RB. May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-) > > --- > Cheers, > Benno >
On 29.08.24 23:56, Danilo Krummrich wrote: > On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: >> On 16.08.24 02:10, Danilo Krummrich wrote: >>> Add a kernel specific `Allocator` trait, that in contrast to the one in >>> Rust's core library doesn't require unstable features and supports GFP >>> flags. >>> >>> Subsequent patches add the following trait implementors: `Kmalloc`, >>> `Vmalloc` and `KVmalloc`. >>> >>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> >>> Signed-off-by: Danilo Krummrich <dakr@kernel.org> >> >> We discussed this in our weekly meeting (I think ~one week ago?). If you >> give me a draft version of the comment that you plan to add regarding >> the `old_layout` parameter, I can see if I am happy with it. If I am, I >> would give you my RB. > > May I propose you let me know what you would like to see covered, rather than > me trying to guess it. :-) I was hoping that we put that in our meeting notes, but I failed to find them... I would put this in a normal comment, so it doesn't show up in the documentation. Preface it like implementation decision/detail: - Why do `Allocator::{realloc,free}` not have an `old_layout` parameter like in the stdlib? (the reasons you had for that decision, like we don't need it etc.) - Then something along the lines of "Note that no technical reason is listed above, so if you need/want to implement an allocator taking advantage of that, you can change it" I don't think we need a lot here. Additionally it would be very useful to also put this in an issue to not lose track of it. --- Cheers, Benno
On 16.08.24 02:10, Danilo Krummrich wrote: > +/// The kernel's [`Allocator`] trait. > +/// > +/// An implementation of [`Allocator`] can allocate, re-allocate and free memory buffer described > +/// via [`Layout`]. > +/// > +/// [`Allocator`] is designed to be implemented as a ZST; [`Allocator`] functions do not operate on > +/// an object instance. > +/// > +/// In order to be able to support `#[derive(SmartPointer)]` later on, we need to avoid a design > +/// that requires an `Allocator` to be instantiated, hence its functions must not contain any kind > +/// of `self` parameter. > +/// > +/// # Safety > +/// > +/// A memory allocation returned from an allocator must remain valid until it is explicitly freed. > +/// > +/// Any pointer to a valid memory allocation must be valid to be passed to any other [`Allocator`] > +/// function of the same type. > +/// > +/// Implementers must ensure that all trait functions abide by the guarantees documented in the > +/// `# Guarantees` sections. Can you make a bullet point list out of these three paragraphs? --- Cheers, Benno
On Fri, 16 Aug 2024 02:10:43 +0200 Danilo Krummrich <dakr@kernel.org> wrote: > Add a kernel specific `Allocator` trait, that in contrast to the one in > Rust's core library doesn't require unstable features and supports GFP > flags. > > Subsequent patches add the following trait implementors: `Kmalloc`, > `Vmalloc` and `KVmalloc`. > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/kernel/alloc.rs | 102 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 102 insertions(+) > > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs > index 1966bd407017..9932f21b0539 100644 > --- a/rust/kernel/alloc.rs > +++ b/rust/kernel/alloc.rs > @@ -11,6 +11,7 @@ > /// Indicates an allocation error. > #[derive(Copy, Clone, PartialEq, Eq, Debug)] > pub struct AllocError; > +use core::{alloc::Layout, ptr::NonNull}; > > /// Flags to be used when allocating memory. > /// > @@ -86,3 +87,104 @@ pub mod flags { > /// small allocations. > pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT); > } > + > +/// The kernel's [`Allocator`] trait. > +/// > +/// An implementation of [`Allocator`] can allocate, re-allocate and free memory buffer described > +/// via [`Layout`]. > +/// > +/// [`Allocator`] is designed to be implemented as a ZST; [`Allocator`] functions do not operate on > +/// an object instance. > +/// > +/// In order to be able to support `#[derive(SmartPointer)]` later on, we need to avoid a design > +/// that requires an `Allocator` to be instantiated, hence its functions must not contain any kind > +/// of `self` parameter. > +/// > +/// # Safety > +/// > +/// A memory allocation returned from an allocator must remain valid until it is explicitly freed. > +/// > +/// Any pointer to a valid memory allocation must be valid to be passed to any other [`Allocator`] > +/// function of the same type. > +/// > +/// Implementers must ensure that all trait functions abide by the guarantees documented in the > +/// `# Guarantees` sections. > +pub unsafe trait Allocator { > + /// Allocate memory based on `layout` and `flags`. > + /// > + /// On success, returns a buffer represented as `NonNull<[u8]>` that satisfies the layout > + /// constraints (i.e. minimum size and alignment as specified by `layout`). > + /// > + /// This function is equivalent to `realloc` when called with `None`. > + /// > + /// # Guarantees > + /// > + /// When the return value is `Ok(ptr)`, then `ptr` is > + /// - valid for reads and writes for `layout.size()` bytes, until it is passed to > + /// [`Allocator::free`] or [`Allocator::realloc`], > + /// - aligned to `layout.align()`, > + /// > + /// Additionally, `Flags` are honored as documented in > + /// <https://docs.kernel.org/core-api/mm-api.html#mm-api-gfp-flags>. > + fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> { > + // SAFETY: Passing `None` to `realloc` is valid by it's safety requirements and asks for a > + // new memory allocation. > + unsafe { Self::realloc(None, layout, flags) } > + } > + > + /// Re-allocate an existing memory allocation to satisfy the requested `layout`. > + /// > + /// If the requested size is zero, `realloc` behaves equivalent to `free`. > + /// > + /// If the requested size is larger than the size of the existing allocation, 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 the size of the existing allocation, `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 > + /// > + /// If `ptr == Some(p)`, then `p` must point to an existing and valid memory allocation created > + /// by this allocator. The alignment encoded in `layout` must be smaller than or equal to the > + /// alignment requested in the previous `alloc` or `realloc` call of the same allocation. > + /// > + /// Additionally, `ptr` is allowed to be `None`; in this case a new memory allocation is > + /// created. > + /// > + /// # Guarantees > + /// > + /// This function has the same guarantees as [`Allocator::alloc`]. When `ptr == Some(p)`, then > + /// it additionally guarantees that: > + /// - the contents of the memory pointed to by `p` are preserved up to the lesser of the new > + /// and old size, > + /// and old size, i.e. > + /// `ret_ptr[0..min(layout.size(), old_size)] == p[0..min(layout.size(), old_size)]`, where > + /// `old_size` is the size of the allocation that `p` points at. > + This line seems to be missing `///`? > + /// - when the return value is `Err(AllocError)`, then `p` is still valid. > + unsafe fn realloc( > + ptr: Option<NonNull<u8>>, > + 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` and > + /// must not be a dangling pointer. > + /// > + /// The memory allocation at `ptr` must never again be read from or written to. > + unsafe fn free(ptr: NonNull<u8>) { > + // SAFETY: The caller guarantees that `ptr` points at a valid allocation created by this > + // allocator. We are passing a `Layout` with the smallest possible alignment, so it is > + // smaller than or equal to the alignment previously used with this allocation. > + let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), Flags(0)) }; > + } > +}
On Fri, Aug 30, 2024 at 01:06:00PM +0000, Benno Lossin wrote: > On 29.08.24 23:56, Danilo Krummrich wrote: > > On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: > >> On 16.08.24 02:10, Danilo Krummrich wrote: > >>> Add a kernel specific `Allocator` trait, that in contrast to the one in > >>> Rust's core library doesn't require unstable features and supports GFP > >>> flags. > >>> > >>> Subsequent patches add the following trait implementors: `Kmalloc`, > >>> `Vmalloc` and `KVmalloc`. > >>> > >>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> > >>> Signed-off-by: Danilo Krummrich <dakr@kernel.org> > >> > >> We discussed this in our weekly meeting (I think ~one week ago?). If you > >> give me a draft version of the comment that you plan to add regarding > >> the `old_layout` parameter, I can see if I am happy with it. If I am, I > >> would give you my RB. > > > > May I propose you let me know what you would like to see covered, rather than > > me trying to guess it. :-) > > I was hoping that we put that in our meeting notes, but I failed to find > them... I would put this in a normal comment, so it doesn't show up in the > documentation. Preface it like implementation decision/detail: > - Why do `Allocator::{realloc,free}` not have an `old_layout` parameter > like in the stdlib? (the reasons you had for that decision, like we > don't need it etc.) Ok. > - Then something along the lines of "Note that no technical reason is > listed above, so if you need/want to implement an allocator taking > advantage of that, you can change it" I don't really want to set the conditions for this to change in the documentation. It really depends on whether it's actually needed or the advantage of having it is huge enough to leave the core kernel allocators with unused arguments. This can really only be properly evaluated case by case in a discussion. > > I don't think we need a lot here. Additionally it would be very useful > to also put this in an issue to not lose track of it. > > --- > Cheers, > Benno >
On 03.09.24 13:56, Danilo Krummrich wrote: > On Fri, Aug 30, 2024 at 01:06:00PM +0000, Benno Lossin wrote: >> On 29.08.24 23:56, Danilo Krummrich wrote: >>> On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: >>>> On 16.08.24 02:10, Danilo Krummrich wrote: >>>>> Add a kernel specific `Allocator` trait, that in contrast to the one in >>>>> Rust's core library doesn't require unstable features and supports GFP >>>>> flags. >>>>> >>>>> Subsequent patches add the following trait implementors: `Kmalloc`, >>>>> `Vmalloc` and `KVmalloc`. >>>>> >>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> >>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org> >>>> >>>> We discussed this in our weekly meeting (I think ~one week ago?). If you >>>> give me a draft version of the comment that you plan to add regarding >>>> the `old_layout` parameter, I can see if I am happy with it. If I am, I >>>> would give you my RB. >>> >>> May I propose you let me know what you would like to see covered, rather than >>> me trying to guess it. :-) >> >> I was hoping that we put that in our meeting notes, but I failed to find >> them... I would put this in a normal comment, so it doesn't show up in the >> documentation. Preface it like implementation decision/detail: >> - Why do `Allocator::{realloc,free}` not have an `old_layout` parameter >> like in the stdlib? (the reasons you had for that decision, like we >> don't need it etc.) > > Ok. > >> - Then something along the lines of "Note that no technical reason is >> listed above, so if you need/want to implement an allocator taking >> advantage of that, you can change it" > > I don't really want to set the conditions for this to change in the > documentation. It really depends on whether it's actually needed or the > advantage of having it is huge enough to leave the core kernel allocators with > unused arguments. > > This can really only be properly evaluated case by case in a discussion. Agreed, but I don't want people to think that we have a reason against doing it in the future. Do you have an idea how to convey this? --- Cheers, Benno
On Tue, Sep 10, 2024 at 01:03:48PM +0000, Benno Lossin wrote: > On 03.09.24 13:56, Danilo Krummrich wrote: > > On Fri, Aug 30, 2024 at 01:06:00PM +0000, Benno Lossin wrote: > >> On 29.08.24 23:56, Danilo Krummrich wrote: > >>> On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: > >>>> On 16.08.24 02:10, Danilo Krummrich wrote: > >>>>> Add a kernel specific `Allocator` trait, that in contrast to the one in > >>>>> Rust's core library doesn't require unstable features and supports GFP > >>>>> flags. > >>>>> > >>>>> Subsequent patches add the following trait implementors: `Kmalloc`, > >>>>> `Vmalloc` and `KVmalloc`. > >>>>> > >>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> > >>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org> > >>>> > >>>> We discussed this in our weekly meeting (I think ~one week ago?). If you > >>>> give me a draft version of the comment that you plan to add regarding > >>>> the `old_layout` parameter, I can see if I am happy with it. If I am, I > >>>> would give you my RB. > >>> > >>> May I propose you let me know what you would like to see covered, rather than > >>> me trying to guess it. :-) > >> > >> I was hoping that we put that in our meeting notes, but I failed to find > >> them... I would put this in a normal comment, so it doesn't show up in the > >> documentation. Preface it like implementation decision/detail: > >> - Why do `Allocator::{realloc,free}` not have an `old_layout` parameter > >> like in the stdlib? (the reasons you had for that decision, like we > >> don't need it etc.) > > > > Ok. > > > >> - Then something along the lines of "Note that no technical reason is > >> listed above, so if you need/want to implement an allocator taking > >> advantage of that, you can change it" > > > > I don't really want to set the conditions for this to change in the > > documentation. It really depends on whether it's actually needed or the > > advantage of having it is huge enough to leave the core kernel allocators with > > unused arguments. > > > > This can really only be properly evaluated case by case in a discussion. > > Agreed, but I don't want people to think that we have a reason against > doing it in the future. Do you have an idea how to convey this? I understand (and agree with) your intention. But I don't think it's necessary to document, because, ideally, this is already true for the whole kernel. Generally, I think it's valid to assume that people are willing to change the code if the advantages outweigh the disadvantages. So, we could write something like "This may be changed if the advantages outweigh the disadvantages.", but it'd be a bit random, since we could probably sprinkle this everywhere. > > --- > Cheers, > Benno >
On 10.09.24 15:23, Danilo Krummrich wrote: > On Tue, Sep 10, 2024 at 01:03:48PM +0000, Benno Lossin wrote: >> On 03.09.24 13:56, Danilo Krummrich wrote: >>> On Fri, Aug 30, 2024 at 01:06:00PM +0000, Benno Lossin wrote: >>>> On 29.08.24 23:56, Danilo Krummrich wrote: >>>>> On Thu, Aug 29, 2024 at 06:19:09PM +0000, Benno Lossin wrote: >>>>>> On 16.08.24 02:10, Danilo Krummrich wrote: >>>>>>> Add a kernel specific `Allocator` trait, that in contrast to the one in >>>>>>> Rust's core library doesn't require unstable features and supports GFP >>>>>>> flags. >>>>>>> >>>>>>> Subsequent patches add the following trait implementors: `Kmalloc`, >>>>>>> `Vmalloc` and `KVmalloc`. >>>>>>> >>>>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> >>>>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org> >>>>>> >>>>>> We discussed this in our weekly meeting (I think ~one week ago?). If you >>>>>> give me a draft version of the comment that you plan to add regarding >>>>>> the `old_layout` parameter, I can see if I am happy with it. If I am, I >>>>>> would give you my RB. >>>>> >>>>> May I propose you let me know what you would like to see covered, rather than >>>>> me trying to guess it. :-) >>>> >>>> I was hoping that we put that in our meeting notes, but I failed to find >>>> them... I would put this in a normal comment, so it doesn't show up in the >>>> documentation. Preface it like implementation decision/detail: >>>> - Why do `Allocator::{realloc,free}` not have an `old_layout` parameter >>>> like in the stdlib? (the reasons you had for that decision, like we >>>> don't need it etc.) >>> >>> Ok. >>> >>>> - Then something along the lines of "Note that no technical reason is >>>> listed above, so if you need/want to implement an allocator taking >>>> advantage of that, you can change it" >>> >>> I don't really want to set the conditions for this to change in the >>> documentation. It really depends on whether it's actually needed or the >>> advantage of having it is huge enough to leave the core kernel allocators with >>> unused arguments. >>> >>> This can really only be properly evaluated case by case in a discussion. >> >> Agreed, but I don't want people to think that we have a reason against >> doing it in the future. Do you have an idea how to convey this? > > I understand (and agree with) your intention. But I don't think it's necessary > to document, because, ideally, this is already true for the whole kernel. > > Generally, I think it's valid to assume that people are willing to change the > code if the advantages outweigh the disadvantages. Sure, but for certain areas you might need a very, very good reason to do so. In this case, I fear that people might believe that this is the case, even though it isn't (one of course still needs a good reason). > So, we could write something like "This may be changed if the advantages > outweigh the disadvantages.", but it'd be a bit random, since we could probably > sprinkle this everywhere. Sure, that works for me. I don't think that we can sprinkle this everywhere (but of course a in lot of places). --- Cheers, Benno
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs index 1966bd407017..9932f21b0539 100644 --- a/rust/kernel/alloc.rs +++ b/rust/kernel/alloc.rs @@ -11,6 +11,7 @@ /// Indicates an allocation error. #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub struct AllocError; +use core::{alloc::Layout, ptr::NonNull}; /// Flags to be used when allocating memory. /// @@ -86,3 +87,104 @@ pub mod flags { /// small allocations. pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT); } + +/// The kernel's [`Allocator`] trait. +/// +/// An implementation of [`Allocator`] can allocate, re-allocate and free memory buffer described +/// via [`Layout`]. +/// +/// [`Allocator`] is designed to be implemented as a ZST; [`Allocator`] functions do not operate on +/// an object instance. +/// +/// In order to be able to support `#[derive(SmartPointer)]` later on, we need to avoid a design +/// that requires an `Allocator` to be instantiated, hence its functions must not contain any kind +/// of `self` parameter. +/// +/// # Safety +/// +/// A memory allocation returned from an allocator must remain valid until it is explicitly freed. +/// +/// Any pointer to a valid memory allocation must be valid to be passed to any other [`Allocator`] +/// function of the same type. +/// +/// Implementers must ensure that all trait functions abide by the guarantees documented in the +/// `# Guarantees` sections. +pub unsafe trait Allocator { + /// Allocate memory based on `layout` and `flags`. + /// + /// On success, returns a buffer represented as `NonNull<[u8]>` that satisfies the layout + /// constraints (i.e. minimum size and alignment as specified by `layout`). + /// + /// This function is equivalent to `realloc` when called with `None`. + /// + /// # Guarantees + /// + /// When the return value is `Ok(ptr)`, then `ptr` is + /// - valid for reads and writes for `layout.size()` bytes, until it is passed to + /// [`Allocator::free`] or [`Allocator::realloc`], + /// - aligned to `layout.align()`, + /// + /// Additionally, `Flags` are honored as documented in + /// <https://docs.kernel.org/core-api/mm-api.html#mm-api-gfp-flags>. + fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> { + // SAFETY: Passing `None` to `realloc` is valid by it's safety requirements and asks for a + // new memory allocation. + unsafe { Self::realloc(None, layout, flags) } + } + + /// Re-allocate an existing memory allocation to satisfy the requested `layout`. + /// + /// If the requested size is zero, `realloc` behaves equivalent to `free`. + /// + /// If the requested size is larger than the size of the existing allocation, 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 the size of the existing allocation, `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 + /// + /// If `ptr == Some(p)`, then `p` must point to an existing and valid memory allocation created + /// by this allocator. The alignment encoded in `layout` must be smaller than or equal to the + /// alignment requested in the previous `alloc` or `realloc` call of the same allocation. + /// + /// Additionally, `ptr` is allowed to be `None`; in this case a new memory allocation is + /// created. + /// + /// # Guarantees + /// + /// This function has the same guarantees as [`Allocator::alloc`]. When `ptr == Some(p)`, then + /// it additionally guarantees that: + /// - the contents of the memory pointed to by `p` are preserved up to the lesser of the new + /// and old size, + /// and old size, i.e. + /// `ret_ptr[0..min(layout.size(), old_size)] == p[0..min(layout.size(), old_size)]`, where + /// `old_size` is the size of the allocation that `p` points at. + + /// - when the return value is `Err(AllocError)`, then `p` is still valid. + unsafe fn realloc( + ptr: Option<NonNull<u8>>, + 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` and + /// must not be a dangling pointer. + /// + /// The memory allocation at `ptr` must never again be read from or written to. + unsafe fn free(ptr: NonNull<u8>) { + // SAFETY: The caller guarantees that `ptr` points at a valid allocation created by this + // allocator. We are passing a `Layout` with the smallest possible alignment, so it is + // smaller than or equal to the alignment previously used with this allocation. + let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), Flags(0)) }; + } +}