| Message ID | 20250409-no-offset-v2-2-dda8e141a909@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | rust: workqueue: remove HasWork::OFFSET | expand |
"Tamir Duberstein" <tamird@gmail.com> writes: > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > the interface of `HasWork` and replacing pointer arithmetic with > `container_of!`. Remove the provided implementation of > `HasWork::get_work_offset` without replacement; an implementation is > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > `HasWork::work_container_of` which was apparently necessary to access > `OFFSET` as `OFFSET` no longer exists. > > A similar API change was discussed on the hrtimer series[1]. > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > Tested-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Tamir Duberstein <tamird@gmail.com> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> Best regards, Andreas Hindborg
On Wed, Apr 09, 2025 at 06:03:22AM -0400, Tamir Duberstein wrote: > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > the interface of `HasWork` and replacing pointer arithmetic with > `container_of!`. Remove the provided implementation of > `HasWork::get_work_offset` without replacement; an implementation is > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > `HasWork::work_container_of` which was apparently necessary to access > `OFFSET` as `OFFSET` no longer exists. > > A similar API change was discussed on the hrtimer series[1]. > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > Tested-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > --- > rust/kernel/workqueue.rs | 45 ++++++++++++--------------------------------- > 1 file changed, 12 insertions(+), 33 deletions(-) > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > index f98bd02b838f..1d640dbdc6ad 100644 > --- a/rust/kernel/workqueue.rs > +++ b/rust/kernel/workqueue.rs > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { > /// > /// # Safety > /// > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The > -/// methods on this trait must have exactly the behavior that the definitions given below have. > +/// The methods on this trait must have exactly the behavior that the definitions given below have. This wording probably needs to be rephrased. You got rid of the definitions that sentence refers to. Alice
On Thu, Apr 10, 2025 at 5:16 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Wed, Apr 09, 2025 at 06:03:22AM -0400, Tamir Duberstein wrote: > > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > > the interface of `HasWork` and replacing pointer arithmetic with > > `container_of!`. Remove the provided implementation of > > `HasWork::get_work_offset` without replacement; an implementation is > > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > > `HasWork::work_container_of` which was apparently necessary to access > > `OFFSET` as `OFFSET` no longer exists. > > > > A similar API change was discussed on the hrtimer series[1]. > > > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Tested-by: Alice Ryhl <aliceryhl@google.com> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > --- > > rust/kernel/workqueue.rs | 45 ++++++++++++--------------------------------- > > 1 file changed, 12 insertions(+), 33 deletions(-) > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > > index f98bd02b838f..1d640dbdc6ad 100644 > > --- a/rust/kernel/workqueue.rs > > +++ b/rust/kernel/workqueue.rs > > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { > > /// > > /// # Safety > > /// > > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The > > -/// methods on this trait must have exactly the behavior that the definitions given below have. > > +/// The methods on this trait must have exactly the behavior that the definitions given below have. > > This wording probably needs to be rephrased. You got rid of the > definitions that sentence refers to. I don't follow. What definitions was it referring to? I interpreted it as having referred to all the items: constants *and* methods. Could you propose an alternate phrasing?
On Thu, Apr 10, 2025 at 10:15:53AM -0400, Tamir Duberstein wrote: > On Thu, Apr 10, 2025 at 5:16 AM Alice Ryhl <aliceryhl@google.com> wrote: > > > > On Wed, Apr 09, 2025 at 06:03:22AM -0400, Tamir Duberstein wrote: > > > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > > > the interface of `HasWork` and replacing pointer arithmetic with > > > `container_of!`. Remove the provided implementation of > > > `HasWork::get_work_offset` without replacement; an implementation is > > > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > > > `HasWork::work_container_of` which was apparently necessary to access > > > `OFFSET` as `OFFSET` no longer exists. > > > > > > A similar API change was discussed on the hrtimer series[1]. > > > > > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > Tested-by: Alice Ryhl <aliceryhl@google.com> > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > --- > > > rust/kernel/workqueue.rs | 45 ++++++++++++--------------------------------- > > > 1 file changed, 12 insertions(+), 33 deletions(-) > > > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > > > index f98bd02b838f..1d640dbdc6ad 100644 > > > --- a/rust/kernel/workqueue.rs > > > +++ b/rust/kernel/workqueue.rs > > > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { > > > /// > > > /// # Safety > > > /// > > > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The > > > -/// methods on this trait must have exactly the behavior that the definitions given below have. > > > +/// The methods on this trait must have exactly the behavior that the definitions given below have. > > > > This wording probably needs to be rephrased. You got rid of the > > definitions that sentence refers to. > > I don't follow. What definitions was it referring to? I interpreted it > as having referred to all the items: constants *and* methods. I meant for it to refer to the default implementations of the methods. > Could you propose an alternate phrasing? I guess the requirements are something along the lines of raw_get_work must return a value pointer, and it must roundtrip with raw_container_of. Alice
On Fri, Apr 11, 2025 at 5:10 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Thu, Apr 10, 2025 at 10:15:53AM -0400, Tamir Duberstein wrote: > > On Thu, Apr 10, 2025 at 5:16 AM Alice Ryhl <aliceryhl@google.com> wrote: > > > > > > On Wed, Apr 09, 2025 at 06:03:22AM -0400, Tamir Duberstein wrote: > > > > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > > > > the interface of `HasWork` and replacing pointer arithmetic with > > > > `container_of!`. Remove the provided implementation of > > > > `HasWork::get_work_offset` without replacement; an implementation is > > > > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > > > > `HasWork::work_container_of` which was apparently necessary to access > > > > `OFFSET` as `OFFSET` no longer exists. > > > > > > > > A similar API change was discussed on the hrtimer series[1]. > > > > > > > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > > > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > Tested-by: Alice Ryhl <aliceryhl@google.com> > > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > --- > > > > rust/kernel/workqueue.rs | 45 ++++++++++++--------------------------------- > > > > 1 file changed, 12 insertions(+), 33 deletions(-) > > > > > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > > > > index f98bd02b838f..1d640dbdc6ad 100644 > > > > --- a/rust/kernel/workqueue.rs > > > > +++ b/rust/kernel/workqueue.rs > > > > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { > > > > /// > > > > /// # Safety > > > > /// > > > > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The > > > > -/// methods on this trait must have exactly the behavior that the definitions given below have. > > > > +/// The methods on this trait must have exactly the behavior that the definitions given below have. > > > > > > This wording probably needs to be rephrased. You got rid of the > > > definitions that sentence refers to. > > > > I don't follow. What definitions was it referring to? I interpreted it > > as having referred to all the items: constants *and* methods. > > I meant for it to refer to the default implementations of the methods. > > > Could you propose an alternate phrasing? > > I guess the requirements are something along the lines of raw_get_work > must return a value pointer, and it must roundtrip with > raw_container_of. What is a value pointer?
On Fri, Apr 11, 2025 at 3:57 PM Tamir Duberstein <tamird@gmail.com> wrote: > > On Fri, Apr 11, 2025 at 5:10 AM Alice Ryhl <aliceryhl@google.com> wrote: > > > > On Thu, Apr 10, 2025 at 10:15:53AM -0400, Tamir Duberstein wrote: > > > On Thu, Apr 10, 2025 at 5:16 AM Alice Ryhl <aliceryhl@google.com> wrote: > > > > > > > > On Wed, Apr 09, 2025 at 06:03:22AM -0400, Tamir Duberstein wrote: > > > > > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing > > > > > the interface of `HasWork` and replacing pointer arithmetic with > > > > > `container_of!`. Remove the provided implementation of > > > > > `HasWork::get_work_offset` without replacement; an implementation is > > > > > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on > > > > > `HasWork::work_container_of` which was apparently necessary to access > > > > > `OFFSET` as `OFFSET` no longer exists. > > > > > > > > > > A similar API change was discussed on the hrtimer series[1]. > > > > > > > > > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > > > > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > > Tested-by: Alice Ryhl <aliceryhl@google.com> > > > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > > --- > > > > > rust/kernel/workqueue.rs | 45 ++++++++++++--------------------------------- > > > > > 1 file changed, 12 insertions(+), 33 deletions(-) > > > > > > > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > > > > > index f98bd02b838f..1d640dbdc6ad 100644 > > > > > --- a/rust/kernel/workqueue.rs > > > > > +++ b/rust/kernel/workqueue.rs > > > > > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { > > > > > /// > > > > > /// # Safety > > > > > /// > > > > > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The > > > > > -/// methods on this trait must have exactly the behavior that the definitions given below have. > > > > > +/// The methods on this trait must have exactly the behavior that the definitions given below have. > > > > > > > > This wording probably needs to be rephrased. You got rid of the > > > > definitions that sentence refers to. > > > > > > I don't follow. What definitions was it referring to? I interpreted it > > > as having referred to all the items: constants *and* methods. > > > > I meant for it to refer to the default implementations of the methods. > > > > > Could you propose an alternate phrasing? > > > > I guess the requirements are something along the lines of raw_get_work > > must return a value pointer, and it must roundtrip with > > raw_container_of. > > What is a value pointer? Sorry, I meant "valid".
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index f98bd02b838f..1d640dbdc6ad 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct { /// /// # Safety /// -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The -/// methods on this trait must have exactly the behavior that the definitions given below have. +/// The methods on this trait must have exactly the behavior that the definitions given below have. /// /// [`impl_has_work!`]: crate::impl_has_work -/// [`OFFSET`]: HasWork::OFFSET pub unsafe trait HasWork<T, const ID: u64 = 0> { - /// The offset of the [`Work<T, ID>`] field. - const OFFSET: usize; - - /// Returns the offset of the [`Work<T, ID>`] field. - /// - /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not - /// [`Sized`]. - /// - /// [`OFFSET`]: HasWork::OFFSET - #[inline] - fn get_work_offset(&self) -> usize { - Self::OFFSET - } - /// Returns a pointer to the [`Work<T, ID>`] field. /// /// # Safety /// /// The provided pointer must point at a valid struct of type `Self`. - #[inline] - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> { - // SAFETY: The caller promises that the pointer is valid. - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> } - } + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>; /// Returns a pointer to the struct containing the [`Work<T, ID>`] field. /// /// # Safety /// /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`. - #[inline] - unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self - where - Self: Sized, - { - // SAFETY: The caller promises that the pointer points at a field of the right type in the - // right kind of struct. - unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self } - } + unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self; } /// Used to safely implement the [`HasWork<T, ID>`] trait. @@ -504,8 +476,6 @@ macro_rules! impl_has_work { // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right // type. unsafe impl$(<$($generics)+>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self { - const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize; - #[inline] unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type $(, $id)?> { // SAFETY: The caller promises that the pointer is not dangling. @@ -513,6 +483,15 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ ::core::ptr::addr_of_mut!((*ptr).$field) } } + + #[inline] + unsafe fn work_container_of( + ptr: *mut $crate::workqueue::Work<$work_type $(, $id)?>, + ) -> *mut Self { + // SAFETY: The caller promises that the pointer points at a field of the right type + // in the right kind of struct. + unsafe { $crate::container_of!(ptr, Self, $field) } + } } )*}; }