Message-ID: <01a46c6d-0107-4455-8c87-af43426752ff@proton.me>
Date: Thu, 15 Aug 2024 13:44:27 +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 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.

>>> 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. 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 and if it is going to select
vmalloc, then we can just pass the alignment (if the vmalloc alignment
patch is done first). 

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