| Message ID | 20240812182355.11641-5-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Generic `Allocator` support for Rust | expand |
On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > typically used for objects smaller than page size. > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > It serves as allocator for the subsequently introduced types `KBox` and > `KVec`. > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/helpers.c | 3 +- > rust/kernel/alloc.rs | 2 +- > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > 3 files changed, 64 insertions(+), 4 deletions(-) > > diff --git a/rust/helpers.c b/rust/helpers.c > index 92d3c03ae1bd..9f7275493365 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > } > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > -void * __must_check __realloc_size(2) > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > { > return krealloc(objp, new_size, flags); > } Why are the various annotations on this helper being removed? This deserves an explanation in the commit message. Alice
On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote: > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > > typically used for objects smaller than page size. > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > > > It serves as allocator for the subsequently introduced types `KBox` and > > `KVec`. > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > --- > > rust/helpers.c | 3 +- > > rust/kernel/alloc.rs | 2 +- > > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > > 3 files changed, 64 insertions(+), 4 deletions(-) > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > index 92d3c03ae1bd..9f7275493365 100644 > > --- a/rust/helpers.c > > +++ b/rust/helpers.c > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > > } > > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > > > -void * __must_check __realloc_size(2) > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > { > > return krealloc(objp, new_size, flags); > > } > > Why are the various annotations on this helper being removed? rust_helper_krealloc() is only called from Rust, hence neither __must_check nor __realloc_size() should have any effect. I also do not apply them in subsequent commits for the vrealloc() and kvrealloc() helpers for this reason and removed them here for consistency. > This deserves an explanation in the commit message. I can also add a separate commit for that. > > Alice >
On Wed, Aug 14, 2024 at 3:36 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote: > > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > > > typically used for objects smaller than page size. > > > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > > > > > It serves as allocator for the subsequently introduced types `KBox` and > > > `KVec`. > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > --- > > > rust/helpers.c | 3 +- > > > rust/kernel/alloc.rs | 2 +- > > > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > > > 3 files changed, 64 insertions(+), 4 deletions(-) > > > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > > index 92d3c03ae1bd..9f7275493365 100644 > > > --- a/rust/helpers.c > > > +++ b/rust/helpers.c > > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > > > } > > > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > > > > > -void * __must_check __realloc_size(2) > > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > { > > > return krealloc(objp, new_size, flags); > > > } > > > > Why are the various annotations on this helper being removed? > > rust_helper_krealloc() is only called from Rust, hence neither __must_check nor > __realloc_size() should have any effect. > > I also do not apply them in subsequent commits for the vrealloc() and > kvrealloc() helpers for this reason and removed them here for consistency. > > > This deserves an explanation in the commit message. > > I can also add a separate commit for that. I think your change would be more obviously correct if you keep them. Alice
On Wed, Aug 14, 2024 at 03:44:56PM +0200, Alice Ryhl wrote: > On Wed, Aug 14, 2024 at 3:36 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote: > > > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > > > > typically used for objects smaller than page size. > > > > > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > > > > > > > It serves as allocator for the subsequently introduced types `KBox` and > > > > `KVec`. > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > --- > > > > rust/helpers.c | 3 +- > > > > rust/kernel/alloc.rs | 2 +- > > > > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > > > > 3 files changed, 64 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > > > index 92d3c03ae1bd..9f7275493365 100644 > > > > --- a/rust/helpers.c > > > > +++ b/rust/helpers.c > > > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > > > > } > > > > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > > > > > > > -void * __must_check __realloc_size(2) > > > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > { > > > > return krealloc(objp, new_size, flags); > > > > } > > > > > > Why are the various annotations on this helper being removed? > > > > rust_helper_krealloc() is only called from Rust, hence neither __must_check nor > > __realloc_size() should have any effect. > > > > I also do not apply them in subsequent commits for the vrealloc() and > > kvrealloc() helpers for this reason and removed them here for consistency. > > > > > This deserves an explanation in the commit message. > > > > I can also add a separate commit for that. > > I think your change would be more obviously correct if you keep them. As in generally, or just for this patch? Generally, I don't think we should indicate compiler checks that actually are never done. For this patch, yes, it's probably better to separate it. > > Alice >
On Wed, Aug 14, 2024 at 3:48 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Aug 14, 2024 at 03:44:56PM +0200, Alice Ryhl wrote: > > On Wed, Aug 14, 2024 at 3:36 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote: > > > > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > > > > > typically used for objects smaller than page size. > > > > > > > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > > > > > > > > > It serves as allocator for the subsequently introduced types `KBox` and > > > > > `KVec`. > > > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > > --- > > > > > rust/helpers.c | 3 +- > > > > > rust/kernel/alloc.rs | 2 +- > > > > > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > > > > > 3 files changed, 64 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > > > > index 92d3c03ae1bd..9f7275493365 100644 > > > > > --- a/rust/helpers.c > > > > > +++ b/rust/helpers.c > > > > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > > > > > } > > > > > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > > > > > > > > > -void * __must_check __realloc_size(2) > > > > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > > { > > > > > return krealloc(objp, new_size, flags); > > > > > } > > > > > > > > Why are the various annotations on this helper being removed? > > > > > > rust_helper_krealloc() is only called from Rust, hence neither __must_check nor > > > __realloc_size() should have any effect. > > > > > > I also do not apply them in subsequent commits for the vrealloc() and > > > kvrealloc() helpers for this reason and removed them here for consistency. > > > > > > > This deserves an explanation in the commit message. > > > > > > I can also add a separate commit for that. > > > > I think your change would be more obviously correct if you keep them. > > As in generally, or just for this patch? > > Generally, I don't think we should indicate compiler checks that actually are > never done. > > For this patch, yes, it's probably better to separate it. In general. If you keep it, then I don't have to think about whether it affects bindgen's output. That's the main reason. Alice
On Wed, Aug 14, 2024 at 03:50:27PM +0200, Alice Ryhl wrote: > On Wed, Aug 14, 2024 at 3:48 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, Aug 14, 2024 at 03:44:56PM +0200, Alice Ryhl wrote: > > > On Wed, Aug 14, 2024 at 3:36 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > On Wed, Aug 14, 2024 at 09:51:34AM +0200, Alice Ryhl wrote: > > > > > On Mon, Aug 12, 2024 at 8:24 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > > > > > > > Implement `Allocator` for `Kmalloc`, the kernel's default allocator, > > > > > > typically used for objects smaller than page size. > > > > > > > > > > > > All memory allocations made with `Kmalloc` end up in `krealloc()`. > > > > > > > > > > > > It serves as allocator for the subsequently introduced types `KBox` and > > > > > > `KVec`. > > > > > > > > > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > > > > > --- > > > > > > rust/helpers.c | 3 +- > > > > > > rust/kernel/alloc.rs | 2 +- > > > > > > rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- > > > > > > 3 files changed, 64 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > > > > > index 92d3c03ae1bd..9f7275493365 100644 > > > > > > --- a/rust/helpers.c > > > > > > +++ b/rust/helpers.c > > > > > > @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, > > > > > > } > > > > > > EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); > > > > > > > > > > > > -void * __must_check __realloc_size(2) > > > > > > -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > > > +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > > > > > > { > > > > > > return krealloc(objp, new_size, flags); > > > > > > } > > > > > > > > > > Why are the various annotations on this helper being removed? > > > > > > > > rust_helper_krealloc() is only called from Rust, hence neither __must_check nor > > > > __realloc_size() should have any effect. > > > > > > > > I also do not apply them in subsequent commits for the vrealloc() and > > > > kvrealloc() helpers for this reason and removed them here for consistency. > > > > > > > > > This deserves an explanation in the commit message. > > > > > > > > I can also add a separate commit for that. > > > > > > I think your change would be more obviously correct if you keep them. > > > > As in generally, or just for this patch? > > > > Generally, I don't think we should indicate compiler checks that actually are > > never done. > > > > For this patch, yes, it's probably better to separate it. > > In general. If you keep it, then I don't have to think about whether > it affects bindgen's output. That's the main reason. Well, it doesn't. If we keep them, we'd consequently also need to add them for vrealloc() and kvrealloc(). But again, they don't do anything for us, and hence are more misleading than helpful IMO. > > Alice >
On Wed, Aug 14, 2024 at 05:03:21PM +0200, Miguel Ojeda wrote: > On Wed, Aug 14, 2024 at 4:00 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > If we keep them, we'd consequently also need to add them for vrealloc() and > > kvrealloc(). But again, they don't do anything for us, and hence are more > > misleading than helpful IMO. > > In general, they could do something (e.g. `noreturn`), perhaps in the future. Indeed, and I think once they're honored we should add them again. It's just that I think as long as compiler attributes aren't honored, we should not have them in the first place to avoid confusion about whether they do or do not have any effect. > > Apart from being potentially misleading, do we gain something by > removing them? It's not so much that I want to remove them for krealloc(), it's that I don't want to intentionally add them for the vrealloc() and kvrealloc() helpers, knowing that they don't do anything (yet). And I think it's even more confusing if the krealloc() helper has those compiler attributes, but the vrealloc() and kvrealloc() helpers do not. > I guess simplicity in the file, but it is also simpler > to keep them aligned to the C side (which I guess is Alice's point), > and avoids having to keep track of what could have a present or future > impact in `bindgen`. > > Cheers, > Miguel >
On 14.08.24 17:19, Danilo Krummrich wrote: > On Wed, Aug 14, 2024 at 05:03:21PM +0200, Miguel Ojeda wrote: >> On Wed, Aug 14, 2024 at 4:00 PM Danilo Krummrich <dakr@kernel.org> wrote: >>> >>> If we keep them, we'd consequently also need to add them for vrealloc() and >>> kvrealloc(). But again, they don't do anything for us, and hence are more >>> misleading than helpful IMO. >> >> In general, they could do something (e.g. `noreturn`), perhaps in the future. > > Indeed, and I think once they're honored we should add them again. That sounds like it will be a lot of work, going through every function and checking if it has the given attribute. Especially when the attributes are enabled one by one. I think we should keep them (and of course introduce them on new functions). --- Cheers, Benno > It's just that I think as long as compiler attributes aren't honored, we should > not have them in the first place to avoid confusion about whether they do or do > not have any effect.
On Wed, Aug 14, 2024 at 03:28:10PM +0000, Benno Lossin wrote: > On 14.08.24 17:19, Danilo Krummrich wrote: > > On Wed, Aug 14, 2024 at 05:03:21PM +0200, Miguel Ojeda wrote: > >> On Wed, Aug 14, 2024 at 4:00 PM Danilo Krummrich <dakr@kernel.org> wrote: > >>> > >>> If we keep them, we'd consequently also need to add them for vrealloc() and > >>> kvrealloc(). But again, they don't do anything for us, and hence are more > >>> misleading than helpful IMO. > >> > >> In general, they could do something (e.g. `noreturn`), perhaps in the future. > > > > Indeed, and I think once they're honored we should add them again. > > That sounds like it will be a lot of work, going through every function > and checking if it has the given attribute. Especially when the > attributes are enabled one by one. I think we should keep them (and of > course introduce them on new functions). I don't think it's gonna be a lot of work, if they're honored one day, which we don't know, do we? Since it seems that everyone else prefers to have those attributes, I'll keep / add them accordingly. However, I think we should at least keep a comment in rust/helpers.c that documents which attributes are honored by bindgen and which aren't. For now, the comment should probably say that non of them are honored? > > --- > Cheers, > Benno > > > It's just that I think as long as compiler attributes aren't honored, we should > > not have them in the first place to avoid confusion about whether they do or do > > not have any effect. >
On Wed, Aug 14, 2024 at 5:20 PM Danilo Krummrich <dakr@kernel.org> wrote: > > Indeed, and I think once they're honored we should add them again. > > It's just that I think as long as compiler attributes aren't honored, we should > not have them in the first place to avoid confusion about whether they do or do > not have any effect. In the C side, many attributes are not honored anyway, depending on the compiler (or tool like sparse), compiler version, etc. So it would be "OK" in that sense (even if here we may know there is no C caller). > It's not so much that I want to remove them for krealloc(), it's that I don't > want to intentionally add them for the vrealloc() and kvrealloc() helpers, > knowing that they don't do anything (yet). One can think of them as "documentation", e.g. seeing `__must_check` tells you something about the function, so I don't think it would be bad to add them, if they are not too much work to maintain. I checked about `__must_check`, because it would be nice if it is used by `bindgen`, and it turns out it already does, but behind `--enable-function-attribute-detection` (apparently for performance reasons): int f(void); __attribute__((warn_unused_result)) int g(void); gives: extern "C" { pub fn f() -> ::std::os::raw::c_int; } extern "C" { #[must_use] pub fn g() -> ::std::os::raw::c_int; } So that one should be kept at the very least, and it is a good example of potential improvements later on based on these attributes. In general, I think it is not a bad idea to write this file like we would write other C files, even if it is not a regular C file. And adding more information to signatures is, after all, part of what we do with Rust overall! Cheers, Miguel
On 12.08.24 20:22, Danilo Krummrich wrote: > + /// # Safety > + /// > + /// This method has the same safety requirements as `Allocator::realloc`. Please make this a link. > + unsafe fn call( > + &self, > + ptr: Option<NonNull<u8>>, > + layout: Layout, > + flags: Flags, > + ) -> Result<NonNull<[u8]>, AllocError> { > + let size = aligned_size(layout); > + let ptr = match ptr { > + Some(ptr) => ptr.as_ptr(), > + None => ptr::null(), > + }; > + > + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. > + let raw_ptr = unsafe { > + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. > + self.0(ptr.cast(), size, flags.0).cast() > + }; > + > + let ptr = if size == 0 { Why do you do this check *after* calling `self.0`? --- Cheers, Benno > + NonNull::dangling() > + } else { > + NonNull::new(raw_ptr).ok_or(AllocError)? > + }; > + > + Ok(NonNull::slice_from_raw_parts(ptr, size)) > + } > +}
On 12.08.24 20:22, Danilo Krummrich wrote: > +unsafe impl Allocator for Kmalloc { There is a missing SAFETY comment here (and also for Vmalloc, probably also for VKmalloc then). --- Cheers, Benno > + unsafe fn realloc( > + ptr: Option<NonNull<u8>>, > + layout: Layout, > + flags: Flags, > + ) -> Result<NonNull<[u8]>, AllocError> { > + // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`. > + unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) } > + } > +} > + > unsafe impl GlobalAlloc for Kmalloc { > unsafe fn alloc(&self, layout: Layout) -> *mut u8 { > // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety > -- > 2.45.2 >
On Wed, Aug 14, 2024 at 06:16:34PM +0200, Miguel Ojeda wrote: > On Wed, Aug 14, 2024 at 6:02 PM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > I checked about `__must_check`, because it would be nice if it is used > > by `bindgen`, and it turns out it already does, but behind > > `--enable-function-attribute-detection` (apparently for performance > > reasons): > > I just tried, and the flag seems to work, gives us a bunch of > `#[must_use]`s which are nice, and apparently no other change (in > usual x86_64 config at least). Cool! That's even better then. > > I don't notice any significant performance difference in our case, so > I will send a quick patch to see if others find an issue with it. > > Cheers, > Miguel >
On Wed, Aug 14, 2024 at 04:21:38PM +0000, Benno Lossin wrote: > On 12.08.24 20:22, Danilo Krummrich wrote: > > + /// # Safety > > + /// > > + /// This method has the same safety requirements as `Allocator::realloc`. > > Please make this a link. Sure. > > > + unsafe fn call( > > + &self, > > + ptr: Option<NonNull<u8>>, > > + layout: Layout, > > + flags: Flags, > > + ) -> Result<NonNull<[u8]>, AllocError> { > > + let size = aligned_size(layout); > > + let ptr = match ptr { > > + Some(ptr) => ptr.as_ptr(), > > + None => ptr::null(), > > + }; > > + > > + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. > > + let raw_ptr = unsafe { > > + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. > > + self.0(ptr.cast(), size, flags.0).cast() > > + }; > > + > > + let ptr = if size == 0 { > > Why do you do this check *after* calling `self.0`? Because I need `raw_ptr` in the else case below. > > --- > Cheers, > Benno > > > + NonNull::dangling() > > + } else { > > + NonNull::new(raw_ptr).ok_or(AllocError)? > > + }; > > + > > + Ok(NonNull::slice_from_raw_parts(ptr, size)) > > + } > > +} >
On 14.08.24 18:59, Danilo Krummrich wrote: > On Wed, Aug 14, 2024 at 04:21:38PM +0000, Benno Lossin wrote: >> On 12.08.24 20:22, Danilo Krummrich wrote: >>> + unsafe fn call( >>> + &self, >>> + ptr: Option<NonNull<u8>>, >>> + layout: Layout, >>> + flags: Flags, >>> + ) -> Result<NonNull<[u8]>, AllocError> { >>> + let size = aligned_size(layout); >>> + let ptr = match ptr { >>> + Some(ptr) => ptr.as_ptr(), >>> + None => ptr::null(), >>> + }; >>> + >>> + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. >>> + let raw_ptr = unsafe { >>> + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. >>> + self.0(ptr.cast(), size, flags.0).cast() >>> + }; >>> + >>> + let ptr = if size == 0 { >> >> Why do you do this check *after* calling `self.0`? > > Because I need `raw_ptr` in the else case below. But you can just return early above? I would prefer the check be done before `self.0` is called. --- Cheers, Benno
On Wed, Aug 14, 2024 at 04:28:04PM +0000, Benno Lossin wrote: > On 12.08.24 20:22, Danilo Krummrich wrote: > > +unsafe impl Allocator for Kmalloc { > > There is a missing SAFETY comment here (and also for Vmalloc, probably > also for VKmalloc then). Any suggestion on what to write here? I'd probably come up with something like: -- Memory returned from `Kmalloc` remains valid until explicitly freed. It is valid to pass any pointer to an allocated memory buffer obtained with any function of `Kmalloc` to any other function of `Kmalloc`. If `Kmalloc::realloc` is called with a size of zero, the given memory allocation, if any, is freed. If `Kmalloc::realloc` is called with `None` it behaves like `Kmalloc::alloc`, i.e. a new memory allocation is created. -- and repeat that for `Vmalloc` and `KVmalloc`. I'm not sure how useful that is though. > > --- > Cheers, > Benno > > > + unsafe fn realloc( > > + ptr: Option<NonNull<u8>>, > > + layout: Layout, > > + flags: Flags, > > + ) -> Result<NonNull<[u8]>, AllocError> { > > + // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`. > > + unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) } > > + } > > +} > > + > > unsafe impl GlobalAlloc for Kmalloc { > > unsafe fn alloc(&self, layout: Layout) -> *mut u8 { > > // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety > > -- > > 2.45.2 > > >
On Wed, Aug 14, 2024 at 05:02:53PM +0000, Benno Lossin wrote: > On 14.08.24 18:59, Danilo Krummrich wrote: > > On Wed, Aug 14, 2024 at 04:21:38PM +0000, Benno Lossin wrote: > >> On 12.08.24 20:22, Danilo Krummrich wrote: > >>> + unsafe fn call( > >>> + &self, > >>> + ptr: Option<NonNull<u8>>, > >>> + layout: Layout, > >>> + flags: Flags, > >>> + ) -> Result<NonNull<[u8]>, AllocError> { > >>> + let size = aligned_size(layout); > >>> + let ptr = match ptr { > >>> + Some(ptr) => ptr.as_ptr(), > >>> + None => ptr::null(), > >>> + }; > >>> + > >>> + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. > >>> + let raw_ptr = unsafe { > >>> + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. > >>> + self.0(ptr.cast(), size, flags.0).cast() > >>> + }; > >>> + > >>> + let ptr = if size == 0 { > >> > >> Why do you do this check *after* calling `self.0`? > > > > Because I need `raw_ptr` in the else case below. > > But you can just return early above? I would prefer the check be done > before `self.0` is called. No, we can't return early, we need `self.0` to be called, because if `size == 0` we free the given allocation, if any. > > --- > Cheers, > Benno >
On 14.08.24 19:15, Danilo Krummrich wrote: > On Wed, Aug 14, 2024 at 05:02:53PM +0000, Benno Lossin wrote: >> On 14.08.24 18:59, Danilo Krummrich wrote: >>> On Wed, Aug 14, 2024 at 04:21:38PM +0000, Benno Lossin wrote: >>>> On 12.08.24 20:22, Danilo Krummrich wrote: >>>>> + unsafe fn call( >>>>> + &self, >>>>> + ptr: Option<NonNull<u8>>, >>>>> + layout: Layout, >>>>> + flags: Flags, >>>>> + ) -> Result<NonNull<[u8]>, AllocError> { >>>>> + let size = aligned_size(layout); >>>>> + let ptr = match ptr { >>>>> + Some(ptr) => ptr.as_ptr(), >>>>> + None => ptr::null(), >>>>> + }; >>>>> + >>>>> + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. >>>>> + let raw_ptr = unsafe { >>>>> + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. >>>>> + self.0(ptr.cast(), size, flags.0).cast() >>>>> + }; >>>>> + >>>>> + let ptr = if size == 0 { >>>> >>>> Why do you do this check *after* calling `self.0`? >>> >>> Because I need `raw_ptr` in the else case below. >> >> But you can just return early above? I would prefer the check be done >> before `self.0` is called. > > No, we can't return early, we need `self.0` to be called, because if `size == 0` > we free the given allocation, if any. Oh yeah of course... I don't know how I arrived at that comment... --- Cheers, Benno
On 14.08.24 19:13, Danilo Krummrich wrote: > On Wed, Aug 14, 2024 at 04:28:04PM +0000, Benno Lossin wrote: >> On 12.08.24 20:22, Danilo Krummrich wrote: >>> +unsafe impl Allocator for Kmalloc { >> >> There is a missing SAFETY comment here (and also for Vmalloc, probably >> also for VKmalloc then). > > Any suggestion on what to write here? > > I'd probably come up with something like: > > -- > Memory returned from `Kmalloc` remains valid until explicitly freed. > > It is valid to pass any pointer to an allocated memory buffer obtained with any > function of `Kmalloc` to any other function of `Kmalloc`. > > If `Kmalloc::realloc` is called with a size of zero, the given memory > allocation, if any, is freed. > > If `Kmalloc::realloc` is called with `None` it behaves like `Kmalloc::alloc`, > i.e. a new memory allocation is created. > -- > > and repeat that for `Vmalloc` and `KVmalloc`. > > I'm not sure how useful that is though. Ideally you would write down how the guarantees are satisfied in addition to the invariants. But I guess this is only guaranteed by the `ReallocFunc::call`, since that handles the ZSTs... Maybe this is a bit excessive. If I come up with something better I will let you know. --- Cheers, Benno
diff --git a/rust/helpers.c b/rust/helpers.c index 92d3c03ae1bd..9f7275493365 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func, } EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key); -void * __must_check __realloc_size(2) -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) +void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) { return krealloc(objp, new_size, flags); } diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs index 194745498a75..b732720cfb95 100644 --- a/rust/kernel/alloc.rs +++ b/rust/kernel/alloc.rs @@ -4,7 +4,7 @@ #[cfg(not(test))] #[cfg(not(testlib))] -mod allocator; +pub mod allocator; pub mod box_ext; pub mod vec_ext; diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs index e32182f91167..b46883d87715 100644 --- a/rust/kernel/alloc/allocator.rs +++ b/rust/kernel/alloc/allocator.rs @@ -5,8 +5,16 @@ use super::{flags::*, Flags}; use core::alloc::{GlobalAlloc, Layout}; use core::ptr; +use core::ptr::NonNull; -struct Kmalloc; +use crate::alloc::{AllocError, Allocator}; +use crate::bindings; + +/// The contiguous kernel allocator. +/// +/// The contiguous kernel allocator only ever allocates physically contiguous memory through +/// `bindings::krealloc`. +pub struct Kmalloc; /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment. fn aligned_size(new_layout: Layout) -> usize { @@ -36,6 +44,59 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 } } +/// # Invariants +/// +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`. +struct ReallocFunc( + unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void, +); + +impl ReallocFunc { + // INVARIANT: `krealloc` satisfies the type invariants. + const KREALLOC: Self = Self(bindings::krealloc); + + /// # Safety + /// + /// This method has the same safety requirements as `Allocator::realloc`. + unsafe fn call( + &self, + ptr: Option<NonNull<u8>>, + layout: Layout, + flags: Flags, + ) -> Result<NonNull<[u8]>, AllocError> { + let size = aligned_size(layout); + let ptr = match ptr { + Some(ptr) => ptr.as_ptr(), + None => ptr::null(), + }; + + // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function. + let raw_ptr = unsafe { + // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. + self.0(ptr.cast(), size, flags.0).cast() + }; + + let ptr = if size == 0 { + NonNull::dangling() + } else { + NonNull::new(raw_ptr).ok_or(AllocError)? + }; + + Ok(NonNull::slice_from_raw_parts(ptr, size)) + } +} + +unsafe impl Allocator for Kmalloc { + unsafe fn realloc( + ptr: Option<NonNull<u8>>, + layout: Layout, + flags: Flags, + ) -> Result<NonNull<[u8]>, AllocError> { + // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`. + unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) } + } +} + unsafe impl GlobalAlloc for Kmalloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
Implement `Allocator` for `Kmalloc`, the kernel's default allocator, typically used for objects smaller than page size. All memory allocations made with `Kmalloc` end up in `krealloc()`. It serves as allocator for the subsequently introduced types `KBox` and `KVec`. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- rust/helpers.c | 3 +- rust/kernel/alloc.rs | 2 +- rust/kernel/alloc/allocator.rs | 63 +++++++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 4 deletions(-)