Message-ID: <5dfe8bae-2c1e-47d4-9fb4-373b7d714c4f@proton.me>
Date: Thu, 15 Aug 2024 06:48:19 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
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 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).

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

>> 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 got some hacked up patches for that, but I'd rather polish and send them once
>> we actually need it.

Sure, just wanted to check why you don't want to do it this series.

---
Cheers,
Benno

Powered by blists - more mailing lists