Message-ID: <Zr4PSuIOpiE-8OkJ@cassiopeiae>
Date: Thu, 15 Aug 2024 16:23:06 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v5 06/26] rust: alloc: implement `Vmalloc` allocator

On Thu, Aug 15, 2024 at 01:44:27PM +0000, Benno Lossin wrote:
> On 15.08.24 14:29, Danilo Krummrich wrote:
> > On Thu, Aug 15, 2024 at 06:48:19AM +0000, Benno Lossin wrote:
> >> On 15.08.24 01:20, Danilo Krummrich wrote:
> >>> On Thu, Aug 15, 2024 at 12:13:06AM +0200, Danilo Krummrich wrote:
> >>>>
> >>>>>
> >>>>>> +        ptr: Option<NonNull<u8>>,
> >>>>>> +        layout: Layout,
> >>>>>> +        flags: Flags,
> >>>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>>>>> +        // TODO: Support alignments larger than PAGE_SIZE.
> >>>>>> +        if layout.align() > bindings::PAGE_SIZE {
> >>>>>> +            pr_warn!("Vmalloc does not support alignments larger than PAGE_SIZE yet.\n");
> >>>>>> +            return Err(AllocError);
> >>>>>
> >>>>> I think here we should first try to use `build_error!`, most often the
> >>>>> alignment will be specified statically, so it should get optimized away.
> >>>>
> >>>> Sure, we can try that first.
> >>>
> >>> I think I spoke too soon here. I don't think `build_error!` or `build_assert!`
> >>> can work here, it would also fail the build when the compiler doesn't know the
> >>> value of the alignment, wouldn't it? I remember that I wasn't overly happy about
> >>> failing this on runtime either when I first thought about this case, but I also
> >>> couldn't think of something better.
> >>
> >> Yes, it might fail even though the alignment at runtime will be fine.
> >> But that's why I suggested trying `build_error!`(or `build_assert!`)
> >> first, if nobody hits the case where the compiler cannot figure it out,
> >> then we can keep it. If there are instances, where it fails, but the
> >> alignment would be fine at runtime, then we can change it to the above.
> >> (I would add such a comment above the assert).
> > 
> > Unfortunately, it already does fail with just the test cases.
> 
> Aw that's sad.
> 
> > Anyway, even if it would have been fine, I don't think it would have been nice
> > for a future user to run into a build error even though the alignment is
> > perfectlly within bounds.
> 
> I think it would have been better compared to failing with a warning at
> runtime.

Generally, yes. But I think it's not acceptable to make calls fail that should
actually succeed.

> 
> >>> In the end it's rather unlikely to ever hit this case, and probably even more
> >>> unlikely to hit it for a sane reason.
> >>
> >> Yeah, but I still prefer the build to fail, rather than emitting a warn
> >> message that can be overlooked at runtime.
> >>
> >>>>> How difficult will it be to support this? (it is a weird requirement,
> >>>>> but I dislike just returning an error...)
> >>>>
> >>>> It's not difficult to support at all. But it requires a C API taking an
> >>>> alignment argument (same for `KVmalloc`).
> >>
> >> I see, that's good to know.
> >>
> >>>> Coming up with a vrealloc_aligned() is rather trivial. kvrealloc_aligned() would
> >>>> be a bit weird though, because the alignment argument could only be really
> >>>> honored if we run into the vrealloc() case. For the krealloc() case it'd still
> >>>> depend on the bucket size that is selected for the requested size.
> >>
> >> Yeah... Maybe some more logic on the Rust side can help with that.
> > 
> > Only if we reimplement `KVmalloc` in Rust, However, there are quite some special
> > cases in __kvmalloc_node_noprof(), i.e. fixup page flags, sanity check the size
> > on kmalloc failure, fail on certain page flags, etc.
> > 
> > I don't really want to duplicate this code, unless we absolutely have to.
> 
> I am under the (probably wrong) impression that kvmalloc has some size
> check and selects vmalloc or kmalloc depending on that. 

Basically, yes. But as mentioned above, there are quite some corner cases [1].

> I think that we
> could check the size and if it is going to allocate via kmalloc, then we
> adjust the size for alignment as usual

We don't need this adjustment any longer, see commit ad59baa31695 ("slab, rust:
extend kmalloc() alignment guarantees to remove Rust padding").

> and if it is going to select
> vmalloc, then we can just pass the alignment (if the vmalloc alignment
> patch is done first).

Yeah, but as mentioned, I'd prefer to do this in C, such that we don't need to
open code everything the C code already does.

[1] https://elixir.bootlin.com/linux/v6.11-rc3/source/mm/util.c#L628
> 
> >>>> Adding the C API, I'm also pretty sure someone's gonna ask what we need an
> >>>> alignment larger than PAGE_SIZE for and if we have a real use case for that.
> >>>> I'm not entirely sure we have a reasonable answer for that.
> >>
> >> We could argue that we can remove an "ugly hack" (when we don't have the
> >> build assert, if we do have that, I don't mind not supporting it), but I
> >> agree that finding a user will be difficult.
> > 
> > I'd argue it's not really a hack to fail on something that's not supported
> > (yet). Allocations can (almost) always fail, this is just another case.
> 
> I guess since this is a deterministic failure, it's better than other
> failures. But I would still say this is hacky.
> 
> ---
> Cheers,
> Benno
> 

Powered by blists - more mailing lists