[PATCH v2 0/2] rust: workqueue: remove HasWork::OFFSET

[PATCH v2 0/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

The bulk of this change occurs in the later commits, please see their
messages for details. The first commit was developed in another series
not yet shared with the list but was picked into this series to allow
`HasWork::work_container_of` to return `container_of!` without the need
to cast from `*const Self` to `*mut Self`.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Changes in v2:
- Rebase on rust-next.
- Add WORKQUEUE maintainers to cc.
- Link to v1: https://lore.kernel.org/r/20250307-no-offset-v1-0-0c728f63b69c@gmail.com

---
Tamir Duberstein (2):
      rust: retain pointer mut-ness in `container_of!`
      rust: workqueue: remove HasWork::OFFSET

 rust/kernel/lib.rs       |  5 ++---
 rust/kernel/rbtree.rs    | 23 ++++++++++-------------
 rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
 3 files changed, 24 insertions(+), 49 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250307-no-offset-e463667a72fb

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH v2 1/2] rust: retain pointer mut-ness in `container_of!`

[Tamir Duberstein]

Avoid casting the input pointer to `*const _`, allowing the output
pointer to be `*mut` if the input is `*mut`. This allows a number of
`*const` to `*mut` conversions to be removed at the cost of slightly
worse ergonomics when the macro is used with a reference rather than a
pointer; the only example of this was in the macro's own doctest.

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/lib.rs    |  5 ++---
 rust/kernel/rbtree.rs | 23 ++++++++++-------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index de07aadd1ff5..1df11156302a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -190,7 +190,7 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 /// }
 ///
 /// let test = Test { a: 10, b: 20 };
-/// let b_ptr = &test.b;
+/// let b_ptr: *const _ = &test.b;
 /// // SAFETY: The pointer points at the `b` field of a `Test`, so the resulting pointer will be
 /// // in-bounds of the same allocation as `b_ptr`.
 /// let test_alias = unsafe { container_of!(b_ptr, Test, b) };
@@ -199,9 +199,8 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 #[macro_export]
 macro_rules! container_of {
     ($ptr:expr, $type:ty, $($f:tt)*) => {{
-        let ptr = $ptr as *const _ as *const u8;
         let offset: usize = ::core::mem::offset_of!($type, $($f)*);
-        ptr.sub(offset) as *const $type
+        $ptr.byte_sub(offset).cast::<$type>()
     }}
 }
 
diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index 5246b2c8a4ff..8d978c896747 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -424,7 +424,7 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
         while !node.is_null() {
             // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
             // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(node, Node<K, V>, links) }.cast_mut();
+            let this = unsafe { container_of!(node, Node<K, V>, links) };
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
             let this_key = unsafe { &(*this).key };
             // SAFETY: `node` is a non-null node so it is valid by the type invariants.
@@ -496,7 +496,7 @@ fn drop(&mut self) {
             // but it is not observable. The loop invariant is still maintained.
 
             // SAFETY: `this` is valid per the loop invariant.
-            unsafe { drop(KBox::from_raw(this.cast_mut())) };
+            unsafe { drop(KBox::from_raw(this)) };
         }
     }
 }
@@ -761,7 +761,7 @@ pub fn remove_current(self) -> (Option<Self>, RBTreeNode<K, V>) {
         let next = self.get_neighbor_raw(Direction::Next);
         // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
         // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) }.cast_mut();
+        let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) };
         // SAFETY: `this` is valid by the type invariants as described above.
         let node = unsafe { KBox::from_raw(this) };
         let node = RBTreeNode { node };
@@ -806,7 +806,7 @@ fn remove_neighbor(&mut self, direction: Direction) -> Option<RBTreeNode<K, V>>
             unsafe { bindings::rb_erase(neighbor, addr_of_mut!(self.tree.root)) };
             // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
             // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(neighbor, Node<K, V>, links) }.cast_mut();
+            let this = unsafe { container_of!(neighbor, Node<K, V>, links) };
             // SAFETY: `this` is valid by the type invariants as described above.
             let node = unsafe { KBox::from_raw(this) };
             return Some(RBTreeNode { node });
@@ -912,7 +912,7 @@ unsafe fn to_key_value_mut<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b
     unsafe fn to_key_value_raw<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, *mut V) {
         // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
         // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) }.cast_mut();
+        let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) };
         // SAFETY: The passed `node` is the current node or a non-null neighbor,
         // thus `this` is valid by the type invariants.
         let k = unsafe { &(*this).key };
@@ -1021,7 +1021,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         // SAFETY: By the type invariant of `IterRaw`, `self.next` is a valid node in an `RBTree`,
         // and by the type invariant of `RBTree`, all nodes point to the links field of `Node<K, V>` objects.
-        let cur = unsafe { container_of!(self.next, Node<K, V>, links) }.cast_mut();
+        let cur = unsafe { container_of!(self.next, Node<K, V>, links) };
 
         // SAFETY: `self.next` is a valid tree node by the type invariants.
         self.next = unsafe { bindings::rb_next(self.next) };
@@ -1216,7 +1216,7 @@ pub fn get_mut(&mut self) -> &mut V {
         // SAFETY:
         // - `self.node_links` is a valid pointer to a node in the tree.
         // - We have exclusive access to the underlying tree, and can thus give out a mutable reference.
-        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value }
+        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value }
     }
 
     /// Converts the entry into a mutable reference to its value.
@@ -1226,7 +1226,7 @@ pub fn into_mut(self) -> &'a mut V {
         // SAFETY:
         // - `self.node_links` is a valid pointer to a node in the tree.
         // - This consumes the `&'a mut RBTree<K, V>`, therefore it can give out a mutable reference that lives for `'a`.
-        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value }
+        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value }
     }
 
     /// Remove this entry from the [`RBTree`].
@@ -1239,9 +1239,7 @@ pub fn remove_node(self) -> RBTreeNode<K, V> {
         RBTreeNode {
             // SAFETY: The node was a node in the tree, but we removed it, so we can convert it
             // back into a box.
-            node: unsafe {
-                KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut())
-            },
+            node: unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) },
         }
     }
 
@@ -1272,8 +1270,7 @@ fn replace(self, node: RBTreeNode<K, V>) -> RBTreeNode<K, V> {
         // SAFETY:
         // - `self.node_ptr` produces a valid pointer to a node in the tree.
         // - Now that we removed this entry from the tree, we can convert the node to a box.
-        let old_node =
-            unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut()) };
+        let old_node = unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) };
 
         RBTreeNode { node: old_node }
     }

-- 
2.49.0

[PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

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.
 ///
 /// [`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) }
+            }
         }
     )*};
 }

-- 
2.49.0

Re: [PATCH v2 1/2] rust: retain pointer mut-ness in `container_of!`

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> Avoid casting the input pointer to `*const _`, allowing the output
> pointer to be `*mut` if the input is `*mut`. This allows a number of
> `*const` to `*mut` conversions to be removed at the cost of slightly
> worse ergonomics when the macro is used with a reference rather than a
> pointer; the only example of this was in the macro's own doctest.
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-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

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Andreas Hindborg]

"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

Re: [PATCH v2 0/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Wed, Apr 9, 2025 at 6:03 AM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The bulk of this change occurs in the later commits, please see their
> messages for details. The first commit was developed in another series
> not yet shared with the list but was picked into this series to allow
> `HasWork::work_container_of` to return `container_of!` without the need
> to cast from `*const Self` to `*mut Self`.

That first commit has been extracted into its own series:
https://lore.kernel.org/all/20250409-container-of-mutness-v1-1-64f472b94534@gmail.com/.

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Alice Ryhl]

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

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

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?

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Alice Ryhl]

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

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

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?

Re: [PATCH v2 2/2] rust: workqueue: remove HasWork::OFFSET

[Alice Ryhl]

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