Re: [PATCH v6 01/26] rust: alloc: add `Allocator` trait

[Danilo Krummrich]

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@xxxxxxxxxx>
> >>>>> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> >>>>
> >>>> 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
>