| Message ID | 20240812182355.11641-7-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Generic `Allocator` support for Rust | expand |
On 12.08.24 20:22, Danilo Krummrich wrote: > Implement `Allocator` for `Vmalloc`, the kernel's virtually contiguous > allocator, typically used for larger objects, (much) larger than page > size. > > All memory allocations made with `Vmalloc` end up in `vrealloc()`. > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/helpers.c | 7 +++++++ > rust/kernel/alloc/allocator.rs | 28 ++++++++++++++++++++++++++++ > rust/kernel/alloc/allocator_test.rs | 1 + > 3 files changed, 36 insertions(+) > > diff --git a/rust/helpers.c b/rust/helpers.c > index 9f7275493365..7406943f887d 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -33,6 +33,7 @@ > #include <linux/sched/signal.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/vmalloc.h> > #include <linux/wait.h> > #include <linux/workqueue.h> > > @@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > } > EXPORT_SYMBOL_GPL(rust_helper_krealloc); > > +void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags) > +{ > + return vrealloc(p, size, flags); > +} > +EXPORT_SYMBOL_GPL(rust_helper_vrealloc); > + > /* > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can > * use it in contexts where Rust expects a `usize` like slice (array) indices. > diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs > index b46883d87715..fdda22c6983f 100644 > --- a/rust/kernel/alloc/allocator.rs > +++ b/rust/kernel/alloc/allocator.rs > @@ -9,6 +9,7 @@ > > use crate::alloc::{AllocError, Allocator}; > use crate::bindings; > +use crate::pr_warn; > > /// The contiguous kernel allocator. > /// > @@ -16,6 +17,12 @@ > /// `bindings::krealloc`. > pub struct Kmalloc; > > +/// The virtually contiguous kernel allocator. > +/// > +/// The vmalloc allocator allocates pages from the page level allocator and maps them into the > +/// contiguous kernel virtual space. > +pub struct Vmalloc; > + > /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment. > fn aligned_size(new_layout: Layout) -> usize { > // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first. > @@ -55,6 +62,9 @@ impl ReallocFunc { > // INVARIANT: `krealloc` satisfies the type invariants. > const KREALLOC: Self = Self(bindings::krealloc); > > + // INVARIANT: `vrealloc` satisfies the type invariants. > + const VREALLOC: Self = Self(bindings::vrealloc); > + > /// # Safety > /// > /// This method has the same safety requirements as `Allocator::realloc`. > @@ -132,6 +142,24 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { > } > } > > +unsafe impl Allocator for Vmalloc { Missing SAFETY comment. > + unsafe fn realloc( Does this need `#[inline]`? > + 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. How difficult will it be to support this? (it is a weird requirement, but I dislike just returning an error...) --- Cheers, Benno > + } > + > + // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously > + // allocated with this `Allocator`. > + unsafe { ReallocFunc::VREALLOC.call(ptr, layout, flags) } > + } > +} > + > #[global_allocator] > static ALLOCATOR: Kmalloc = Kmalloc; > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs > index 4785efc474a7..e7bf2982f68f 100644 > --- a/rust/kernel/alloc/allocator_test.rs > +++ b/rust/kernel/alloc/allocator_test.rs > @@ -7,6 +7,7 @@ > use core::ptr::NonNull; > > pub struct Kmalloc; > +pub type Vmalloc = Kmalloc; > > unsafe impl Allocator for Kmalloc { > unsafe fn realloc( > -- > 2.45.2 >
On Wed, Aug 14, 2024 at 04:32:34PM +0000, Benno Lossin wrote: > On 12.08.24 20:22, Danilo Krummrich wrote: > > Implement `Allocator` for `Vmalloc`, the kernel's virtually contiguous > > allocator, typically used for larger objects, (much) larger than page > > size. > > > > All memory allocations made with `Vmalloc` end up in `vrealloc()`. > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > --- > > rust/helpers.c | 7 +++++++ > > rust/kernel/alloc/allocator.rs | 28 ++++++++++++++++++++++++++++ > > rust/kernel/alloc/allocator_test.rs | 1 + > > 3 files changed, 36 insertions(+) > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > index 9f7275493365..7406943f887d 100644 > > --- a/rust/helpers.c > > +++ b/rust/helpers.c > > @@ -33,6 +33,7 @@ > > #include <linux/sched/signal.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > +#include <linux/vmalloc.h> > > #include <linux/wait.h> > > #include <linux/workqueue.h> > > > > @@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > } > > EXPORT_SYMBOL_GPL(rust_helper_krealloc); > > > > +void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags) > > +{ > > + return vrealloc(p, size, flags); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_vrealloc); > > + > > /* > > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can > > * use it in contexts where Rust expects a `usize` like slice (array) indices. > > diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs > > index b46883d87715..fdda22c6983f 100644 > > --- a/rust/kernel/alloc/allocator.rs > > +++ b/rust/kernel/alloc/allocator.rs > > @@ -9,6 +9,7 @@ > > > > use crate::alloc::{AllocError, Allocator}; > > use crate::bindings; > > +use crate::pr_warn; > > > > /// The contiguous kernel allocator. > > /// > > @@ -16,6 +17,12 @@ > > /// `bindings::krealloc`. > > pub struct Kmalloc; > > > > +/// The virtually contiguous kernel allocator. > > +/// > > +/// The vmalloc allocator allocates pages from the page level allocator and maps them into the > > +/// contiguous kernel virtual space. > > +pub struct Vmalloc; > > + > > /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment. > > fn aligned_size(new_layout: Layout) -> usize { > > // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first. > > @@ -55,6 +62,9 @@ impl ReallocFunc { > > // INVARIANT: `krealloc` satisfies the type invariants. > > const KREALLOC: Self = Self(bindings::krealloc); > > > > + // INVARIANT: `vrealloc` satisfies the type invariants. > > + const VREALLOC: Self = Self(bindings::vrealloc); > > + > > /// # Safety > > /// > > /// This method has the same safety requirements as `Allocator::realloc`. > > @@ -132,6 +142,24 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { > > } > > } > > > > +unsafe impl Allocator for Vmalloc { > > Missing SAFETY comment. > > > + unsafe fn realloc( > > Does this need `#[inline]`? Given that we almost only call `ReallocFunc::VREALLOC.call`, inlining this seems reasonable. > > > + 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. > > 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`). 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. 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. I got some hacked up patches for that, but I'd rather polish and send them once we actually need it. > > --- > Cheers, > Benno > > > + } > > + > > + // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously > > + // allocated with this `Allocator`. > > + unsafe { ReallocFunc::VREALLOC.call(ptr, layout, flags) } > > + } > > +} > > + > > #[global_allocator] > > static ALLOCATOR: Kmalloc = Kmalloc; > > > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs > > index 4785efc474a7..e7bf2982f68f 100644 > > --- a/rust/kernel/alloc/allocator_test.rs > > +++ b/rust/kernel/alloc/allocator_test.rs > > @@ -7,6 +7,7 @@ > > use core::ptr::NonNull; > > > > pub struct Kmalloc; > > +pub type Vmalloc = Kmalloc; > > > > unsafe impl Allocator for Kmalloc { > > unsafe fn realloc( > > -- > > 2.45.2 > > >
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. In the end it's rather unlikely to ever hit this case, and probably even more unlikely to hit it for a sane reason. > > > > > 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`). > > 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. > > 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. > > I got some hacked up patches for that, but I'd rather polish and send them once > we actually need it.
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
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 >
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
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 >
On 15.08.24 16:23, Danilo Krummrich wrote: > 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: >>>>>>> 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 I see, then it's probably better to just add an align parameter variant on the C side. Instead of rebuilding it in Rust. --- Cheers, Benno
diff --git a/rust/helpers.c b/rust/helpers.c index 9f7275493365..7406943f887d 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -33,6 +33,7 @@ #include <linux/sched/signal.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/vmalloc.h> #include <linux/wait.h> #include <linux/workqueue.h> @@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) } EXPORT_SYMBOL_GPL(rust_helper_krealloc); +void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags) +{ + return vrealloc(p, size, flags); +} +EXPORT_SYMBOL_GPL(rust_helper_vrealloc); + /* * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can * use it in contexts where Rust expects a `usize` like slice (array) indices. diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs index b46883d87715..fdda22c6983f 100644 --- a/rust/kernel/alloc/allocator.rs +++ b/rust/kernel/alloc/allocator.rs @@ -9,6 +9,7 @@ use crate::alloc::{AllocError, Allocator}; use crate::bindings; +use crate::pr_warn; /// The contiguous kernel allocator. /// @@ -16,6 +17,12 @@ /// `bindings::krealloc`. pub struct Kmalloc; +/// The virtually contiguous kernel allocator. +/// +/// The vmalloc allocator allocates pages from the page level allocator and maps them into the +/// contiguous kernel virtual space. +pub struct Vmalloc; + /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment. fn aligned_size(new_layout: Layout) -> usize { // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first. @@ -55,6 +62,9 @@ impl ReallocFunc { // INVARIANT: `krealloc` satisfies the type invariants. const KREALLOC: Self = Self(bindings::krealloc); + // INVARIANT: `vrealloc` satisfies the type invariants. + const VREALLOC: Self = Self(bindings::vrealloc); + /// # Safety /// /// This method has the same safety requirements as `Allocator::realloc`. @@ -132,6 +142,24 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { } } +unsafe impl Allocator for Vmalloc { + unsafe fn realloc( + 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); + } + + // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously + // allocated with this `Allocator`. + unsafe { ReallocFunc::VREALLOC.call(ptr, layout, flags) } + } +} + #[global_allocator] static ALLOCATOR: Kmalloc = Kmalloc; diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs index 4785efc474a7..e7bf2982f68f 100644 --- a/rust/kernel/alloc/allocator_test.rs +++ b/rust/kernel/alloc/allocator_test.rs @@ -7,6 +7,7 @@ use core::ptr::NonNull; pub struct Kmalloc; +pub type Vmalloc = Kmalloc; unsafe impl Allocator for Kmalloc { unsafe fn realloc(