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