[PATCH v4 0/7] rust: workqueue: add bindings for the workqueue

[PATCH v4 0/7] rust: workqueue: add bindings for the workqueue

[Alice Ryhl]

This patchset contains bindings for the kernel workqueue.

One of the primary goals behind the design used in this patch is that we
must support embedding the `work_struct` as a field in user-provided
types, because this allows you to submit things to the workqueue without
having to allocate, making the submission infallible. If we didn't have
to support this, then the patch would be much simpler. One of the main
things that make it complicated is that we must ensure that the function
pointer in the `work_struct` is compatible with the struct it is
contained within.

The original version of the workqueue bindings was written by Wedson,
but I have rewritten much of it so that it uses the pin-init
infrastructure and can be used with containers other than `Arc`.

This patchset depends on the patch [1] that changes `Opaque` to be
`!Unpin` for correctness. The patchset is based on the rust-6.6 tag,
since that tag includes that patch.

Changes v3 -> v4:

 * Use `offset_of!` macro from standard library, and drop patch with
   custom implementation.
 * Add name parameter to `work_struct` constructor, and drop patch with
   __INIT_WORK_WITH_KEY.
 * Various improvements to documentation and comments.

More detailed changelogs can be found on the individual patches.

Previous versions:
v1: https://lore.kernel.org/all/20230517203119.3160435-1-aliceryhl@google.com/
v2: https://lore.kernel.org/all/20230601134946.3887870-1-aliceryhl@google.com/
v3: https://lore.kernel.org/all/20230711093303.1433770-1-aliceryhl@google.com/

[1]: https://lore.kernel.org/all/20230630150216.109789-1-benno.lossin@proton.me/

Alice Ryhl (5):
  rust: workqueue: add low-level workqueue bindings
  rust: workqueue: add helper for defining work_struct fields
  rust: workqueue: implement `WorkItemPointer` for pointer types
  rust: workqueue: add `try_spawn` helper method
  rust: workqueue: add examples

Wedson Almeida Filho (2):
  rust: sync: add `Arc::{from_raw, into_raw}`
  rust: workqueue: define built-in queues

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  |  13 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/sync/arc.rs         |  46 ++-
 rust/kernel/workqueue.rs        | 679 ++++++++++++++++++++++++++++++++
 scripts/Makefile.build          |   2 +-
 6 files changed, 741 insertions(+), 3 deletions(-)
 create mode 100644 rust/kernel/workqueue.rs

base-commit: 4af84c6a85c63bec24611e46bb3de2c0a6602a51
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 1/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Alice Ryhl]

From: Wedson Almeida Filho <walmeida@microsoft.com>

These methods can be used to turn an `Arc` into a raw pointer and back,
in a way that preserves the metadata for fat pointers.

This is done using the unstable ptr_metadata feature [1]. However, it
could also be done using the unstable pointer_byte_offsets feature [2],
which is likely to have a shorter path to stabilization than
ptr_metadata.

Link: https://github.com/rust-lang/rust/issues/81513 [1]
Link: https://github.com/rust-lang/rust/issues/96283 [2]
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
---
v3 -> v4:
 * Update doc comment for `Arc::from_raw`.
 * Add Reviewed-by for Martin, Benno, and Andreas.

 rust/kernel/lib.rs      |  1 +
 rust/kernel/sync/arc.rs | 46 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index d59041ff5ff2..abeef423d3a8 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -16,6 +16,7 @@
 #![feature(coerce_unsized)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
+#![feature(ptr_metadata)]
 #![feature(receiver_trait)]
 #![feature(unsize)]
 
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 172f563976a9..cae2ff44920b 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -24,13 +24,13 @@
 };
 use alloc::boxed::Box;
 use core::{
-    alloc::AllocError,
+    alloc::{AllocError, Layout},
     fmt,
     marker::{PhantomData, Unsize},
     mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     pin::Pin,
-    ptr::NonNull,
+    ptr::{NonNull, Pointee},
 };
 use macros::pin_data;
 
@@ -212,6 +212,48 @@ unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
         }
     }
 
+    /// Convert the [`Arc`] into a raw pointer.
+    ///
+    /// The raw pointer has ownership of the refcount that this Arc object owned.
+    pub fn into_raw(self) -> *const T {
+        let ptr = self.ptr.as_ptr();
+        core::mem::forget(self);
+        // SAFETY: The pointer is valid.
+        unsafe { core::ptr::addr_of!((*ptr).data) }
+    }
+
+    /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
+    /// must not be called more than once for each previous call to [`Arc::into_raw`].
+    pub unsafe fn from_raw(ptr: *const T) -> Self {
+        let refcount_layout = Layout::new::<bindings::refcount_t>();
+        // SAFETY: The caller guarantees that the pointer is valid.
+        let val_layout = Layout::for_value(unsafe { &*ptr });
+        // SAFETY: We're computing the layout of a real struct that existed when compiling this
+        // binary, so its layout is not so large that it can trigger arithmetic overflow.
+        let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 };
+
+        let metadata: <T as Pointee>::Metadata = core::ptr::metadata(ptr);
+        // SAFETY: The metadata of `T` and `ArcInner<T>` is the same because `ArcInner` is a struct
+        // with `T` as its last field.
+        //
+        // This is documented at:
+        // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>.
+        let metadata: <ArcInner<T> as Pointee>::Metadata =
+            unsafe { core::mem::transmute_copy(&metadata) };
+        // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the
+        // pointer, since it originates from a previous call to `Arc::into_raw` and is still valid.
+        let ptr = unsafe { (ptr as *mut u8).sub(val_offset) as *mut () };
+        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);
+
+        // SAFETY: By the safety requirements we know that `ptr` came from `Arc::into_raw`, so the
+        // reference count held then will be owned by the new `Arc` object.
+        unsafe { Self::from_inner(NonNull::new_unchecked(ptr)) }
+    }
+
     /// Returns an [`ArcBorrow`] from the given [`Arc`].
     ///
     /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 2/7] rust: workqueue: add low-level workqueue bindings

[Alice Ryhl]

Define basic low-level bindings to a kernel workqueue. The API defined
here can only be used unsafely. Later commits will provide safe
wrappers.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
---
v3 -> v4:
 * Update safety comment for Send and Sync impls.
 * Add Reviewed-by for Boqun.

 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/workqueue.rs        | 109 ++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 rust/kernel/workqueue.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 058954961bfc..ca8ac8d8decb 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -11,6 +11,7 @@
 #include <linux/refcount.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/workqueue.h>
 
 /* `bindgen` gets confused at certain things. */
 const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index abeef423d3a8..b56aaea3de7a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -44,6 +44,7 @@
 pub mod sync;
 pub mod task;
 pub mod types;
+pub mod workqueue;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
new file mode 100644
index 000000000000..19e5c50127af
--- /dev/null
+++ b/rust/kernel/workqueue.rs
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Work queues.
+//!
+//! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
+
+use crate::{bindings, types::Opaque};
+
+/// A kernel work queue.
+///
+/// Wraps the kernel's C `struct workqueue_struct`.
+///
+/// It allows work items to be queued to run on thread pools managed by the kernel. Several are
+/// always available, for example, `system`, `system_highpri`, `system_long`, etc.
+#[repr(transparent)]
+pub struct Queue(Opaque<bindings::workqueue_struct>);
+
+// SAFETY: Accesses to workqueues used by [`Queue`] are thread-safe.
+unsafe impl Send for Queue {}
+// SAFETY: Accesses to workqueues used by [`Queue`] are thread-safe.
+unsafe impl Sync for Queue {}
+
+impl Queue {
+    /// Use the provided `struct workqueue_struct` with Rust.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that the provided raw pointer is not dangling, that it points at a
+    /// valid workqueue, and that it remains valid until the end of 'a.
+    pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue {
+        // SAFETY: The `Queue` type is `#[repr(transparent)]`, so the pointer cast is valid. The
+        // caller promises that the pointer is not dangling.
+        unsafe { &*(ptr as *const Queue) }
+    }
+
+    /// Enqueues a work item.
+    ///
+    /// This may fail if the work item is already enqueued in a workqueue.
+    ///
+    /// The work item will be submitted using `WORK_CPU_UNBOUND`.
+    pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
+    where
+        W: RawWorkItem<ID> + Send + 'static,
+    {
+        let queue_ptr = self.0.get();
+
+        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue. The other
+        // `__enqueue` requirements are not relevant since `W` is `Send` and static.
+        //
+        // The call to `bindings::queue_work_on` will dereference the provided raw pointer, which
+        // is ok because `__enqueue` guarantees that the pointer is valid for the duration of this
+        // closure.
+        //
+        // Furthermore, if the C workqueue code accesses the pointer after this call to
+        // `__enqueue`, then the work item was successfully enqueued, and `bindings::queue_work_on`
+        // will have returned true. In this case, `__enqueue` promises that the raw pointer will
+        // stay valid until we call the function pointer in the `work_struct`, so the access is ok.
+        unsafe {
+            w.__enqueue(move |work_ptr| {
+                bindings::queue_work_on(bindings::WORK_CPU_UNBOUND as _, queue_ptr, work_ptr)
+            })
+        }
+    }
+}
+
+/// A raw work item.
+///
+/// This is the low-level trait that is designed for being as general as possible.
+///
+/// The `ID` parameter to this trait exists so that a single type can provide multiple
+/// implementations of this trait. For example, if a struct has multiple `work_struct` fields, then
+/// you will implement this trait once for each field, using a different id for each field. The
+/// actual value of the id is not important as long as you use different ids for different fields
+/// of the same struct. (Fields of different structs need not use different ids.)
+///
+/// Note that the id is used only to select the right method to call during compilation. It wont be
+/// part of the final executable.
+///
+/// # Safety
+///
+/// Implementers must ensure that any pointers passed to a `queue_work_on` closure by `__enqueue`
+/// remain valid for the duration specified in the guarantees section of the documentation for
+/// `__enqueue`.
+pub unsafe trait RawWorkItem<const ID: u64> {
+    /// The return type of [`Queue::enqueue`].
+    type EnqueueOutput;
+
+    /// Enqueues this work item on a queue using the provided `queue_work_on` method.
+    ///
+    /// # Guarantees
+    ///
+    /// If this method calls the provided closure, then the raw pointer is guaranteed to point at a
+    /// valid `work_struct` for the duration of the call to the closure. If the closure returns
+    /// true, then it is further guaranteed that the pointer remains valid until someone calls the
+    /// function pointer stored in the `work_struct`.
+    ///
+    /// # Safety
+    ///
+    /// The provided closure may only return `false` if the `work_struct` is already in a workqueue.
+    ///
+    /// If the work item type is annotated with any lifetimes, then you must not call the function
+    /// pointer after any such lifetime expires. (Never calling the function pointer is okay.)
+    ///
+    /// If the work item type is not [`Send`], then the function pointer must be called on the same
+    /// thread as the call to `__enqueue`.
+    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
+    where
+        F: FnOnce(*mut bindings::work_struct) -> bool;
+}
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 3/7] rust: workqueue: define built-in queues

[Alice Ryhl]

From: Wedson Almeida Filho <walmeida@microsoft.com>

We provide these methods because it lets us access these queues from
Rust without using unsafe code.

These methods return `&'static Queue`. References annotated with the
'static lifetime are used when the referent will stay alive forever.
That is ok for these queues because they are global variables and cannot
be destroyed.

Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
v3 -> v4:
 * No changes.

 rust/kernel/workqueue.rs | 71 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 19e5c50127af..6dbc5b9b3da9 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -107,3 +107,74 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
     where
         F: FnOnce(*mut bindings::work_struct) -> bool;
 }
+
+/// Returns the system work queue (`system_wq`).
+///
+/// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
+/// users which expect relatively short queue flush time.
+///
+/// Callers shouldn't queue work items which can run for too long.
+pub fn system() -> &'static Queue {
+    // SAFETY: `system_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_wq) }
+}
+
+/// Returns the system high-priority work queue (`system_highpri_wq`).
+///
+/// It is similar to the one returned by [`system`] but for work items which require higher
+/// scheduling priority.
+pub fn system_highpri() -> &'static Queue {
+    // SAFETY: `system_highpri_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_highpri_wq) }
+}
+
+/// Returns the system work queue for potentially long-running work items (`system_long_wq`).
+///
+/// It is similar to the one returned by [`system`] but may host long running work items. Queue
+/// flushing might take relatively long.
+pub fn system_long() -> &'static Queue {
+    // SAFETY: `system_long_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_long_wq) }
+}
+
+/// Returns the system unbound work queue (`system_unbound_wq`).
+///
+/// Workers are not bound to any specific CPU, not concurrency managed, and all queued work items
+/// are executed immediately as long as `max_active` limit is not reached and resources are
+/// available.
+pub fn system_unbound() -> &'static Queue {
+    // SAFETY: `system_unbound_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_unbound_wq) }
+}
+
+/// Returns the system freezable work queue (`system_freezable_wq`).
+///
+/// It is equivalent to the one returned by [`system`] except that it's freezable.
+///
+/// A freezable workqueue participates in the freeze phase of the system suspend operations. Work
+/// items on the workqueue are drained and no new work item starts execution until thawed.
+pub fn system_freezable() -> &'static Queue {
+    // SAFETY: `system_freezable_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_freezable_wq) }
+}
+
+/// Returns the system power-efficient work queue (`system_power_efficient_wq`).
+///
+/// It is inclined towards saving power and is converted to "unbound" variants if the
+/// `workqueue.power_efficient` kernel parameter is specified; otherwise, it is similar to the one
+/// returned by [`system`].
+pub fn system_power_efficient() -> &'static Queue {
+    // SAFETY: `system_power_efficient_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_power_efficient_wq) }
+}
+
+/// Returns the system freezable power-efficient work queue (`system_freezable_power_efficient_wq`).
+///
+/// It is similar to the one returned by [`system_power_efficient`] except that is freezable.
+///
+/// A freezable workqueue participates in the freeze phase of the system suspend operations. Work
+/// items on the workqueue are drained and no new work item starts execution until thawed.
+pub fn system_freezable_power_efficient() -> &'static Queue {
+    // SAFETY: `system_freezable_power_efficient_wq` is a C global, always available.
+    unsafe { Queue::from_raw(bindings::system_freezable_power_efficient_wq) }
+}
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Alice Ryhl]

The main challenge with defining `work_struct` fields is making sure
that the function pointer stored in the `work_struct` is appropriate for
the work item type it is embedded in. It needs to know the offset of the
`work_struct` field being used (even if there are several!) so that it
can do a `container_of`, and it needs to know the type of the work item
so that it can call into the right user-provided code. All of this needs
to happen in a way that provides a safe API to the user, so that users
of the workqueue cannot mix up the function pointers.

There are three important pieces that are relevant when doing this:

 * The pointer type.
 * The work item struct. This is what the pointer points at.
 * The `work_struct` field. This is a field of the work item struct.

This patch introduces a separate trait for each piece. The pointer type
is given a `WorkItemPointer` trait, which pointer types need to
implement to be usable with the workqueue. This trait will be
implemented for `Arc` and `Box` in a later patch in this patchset.
Implementing this trait is unsafe because this is where the
`container_of` operation happens, but user-code will not need to
implement it themselves.

The work item struct should then implement the `WorkItem` trait. This
trait is where user-code specifies what they want to happen when a work
item is executed. It also specifies what the correct pointer type is.

Finally, to make the work item struct know the offset of its
`work_struct` field, we use a trait called `HasWork<T, ID>`. If a type
implements this trait, then the type declares that, at the given offset,
there is a field of type `Work<T, ID>`. The trait is marked unsafe
because the OFFSET constant must be correct, but we provide an
`impl_has_work!` macro that can safely implement `HasWork<T>` on a type.
The macro expands to something that only compiles if the specified field
really has the type `Work<T>`. It is used like this:

```
struct MyWorkItem {
    work_field: Work<MyWorkItem, 1>,
}

impl_has_work! {
    impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
}
```

Note that since the `Work` type is annotated with an id, you can have
several `work_struct` fields by using a different id for each one.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
v3 -> v4:
 * The helper was changed to take a name argument, and is implemented
   directly.
 * `Work::new` now takes a name argument, and a `new_work!` macro was
   introduced to help call it.
 * Use `core::mem::offset_of` rather than a custom implementation.
 * Fix imports in examples.
 * Dropped Reviewed-bys due to changes.

 rust/helpers.c           |  13 ++
 rust/kernel/lib.rs       |   1 +
 rust/kernel/workqueue.rs | 257 ++++++++++++++++++++++++++++++++++++++-
 scripts/Makefile.build   |   2 +-
 4 files changed, 271 insertions(+), 2 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index ebd69490127b..45cf9702d4e4 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -29,6 +29,7 @@
 #include <linux/sched/signal.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -137,6 +138,18 @@ void rust_helper_put_task_struct(struct task_struct *t)
 }
 EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
 
+void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
+				    bool onstack, const char *name,
+				    struct lock_class_key *key)
+{
+	__init_work(work, onstack);
+	work->data = (atomic_long_t)WORK_DATA_INIT();
+	lockdep_init_map(&work->lockdep_map, name, key, 0);
+	INIT_LIST_HEAD(&work->entry);
+	work->func = func;
+}
+EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
+
 /*
  * `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/lib.rs b/rust/kernel/lib.rs
index b56aaea3de7a..c537d2edb4c8 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -16,6 +16,7 @@
 #![feature(coerce_unsized)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
+#![feature(offset_of)]
 #![feature(ptr_metadata)]
 #![feature(receiver_trait)]
 #![feature(unsize)]
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 82e3fb19fdaf..da37bfa97211 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -262,7 +262,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := new_uninit
+rust_allowed_features := new_uninit,offset_of
 
 # `--out-dir` is required to avoid temporaries being created by `rustc` in the
 # current working directory, which may be not accessible in the out-of-tree
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 6dbc5b9b3da9..251541f1cd68 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -2,9 +2,42 @@
 
 //! Work queues.
 //!
+//! This file has two components: The raw work item API, and the safe work item API.
+//!
+//! One pattern that is used in both APIs is the `ID` const generic, which exists to allow a single
+//! type to define multiple `work_struct` fields. This is done by choosing an id for each field,
+//! and using that id to specify which field you wish to use. (The actual value doesn't matter, as
+//! long as you use different values for different fields of the same struct.) Since these IDs are
+//! generic, they are used only at compile-time, so they shouldn't exist in the final binary.
+//!
+//! # The raw API
+//!
+//! The raw API consists of the `RawWorkItem` trait, where the work item needs to provide an
+//! arbitrary function that knows how to enqueue the work item. It should usually not be used
+//! directly, but if you want to, you can use it without using the pieces from the safe API.
+//!
+//! # The safe API
+//!
+//! The safe API is used via the `Work` struct and `WorkItem` traits. Furthermore, it also includes
+//! a trait called `WorkItemPointer`, which is usually not used directly by the user.
+//!
+//!  * The `Work` struct is the Rust wrapper for the C `work_struct` type.
+//!  * The `WorkItem` trait is implemented for structs that can be enqueued to a workqueue.
+//!  * The `WorkItemPointer` trait is implemented for the pointer type that points at a something
+//!    that implements `WorkItem`.
+//!
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
-use crate::{bindings, types::Opaque};
+use crate::{bindings, prelude::*, sync::LockClassKey, types::Opaque};
+use core::marker::PhantomData;
+
+/// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
+#[macro_export]
+macro_rules! new_work {
+    ($($name:literal)?) => {
+        $crate::workqueue::Work::new($crate::optional_name!($($name)?), $crate::static_lock_class!())
+    };
+}
 
 /// A kernel work queue.
 ///
@@ -108,6 +141,228 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
         F: FnOnce(*mut bindings::work_struct) -> bool;
 }
 
+/// Defines the method that should be called directly when a work item is executed.
+///
+/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
+/// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
+/// instead. The `run` method on this trait will usually just perform the appropriate
+/// `container_of` translation and then call into the `run` method from the [`WorkItem`] trait.
+///
+/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
+///
+/// # Safety
+///
+/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
+/// method of this trait as the function pointer.
+///
+/// [`__enqueue`]: RawWorkItem::__enqueue
+/// [`run`]: WorkItemPointer::run
+pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
+    /// Run this work item.
+    ///
+    /// # Safety
+    ///
+    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
+    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
+}
+
+/// Defines the method that should be called when this work item is executed.
+///
+/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
+pub trait WorkItem<const ID: u64 = 0> {
+    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
+    /// `Pin<Box<Self>>`.
+    type Pointer: WorkItemPointer<ID>;
+
+    /// The method that should be called when this work item is executed.
+    fn run(this: Self::Pointer);
+}
+
+/// Links for a work item.
+///
+/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
+/// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
+///
+/// Wraps the kernel's C `struct work_struct`.
+///
+/// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
+#[repr(transparent)]
+pub struct Work<T: ?Sized, const ID: u64 = 0> {
+    work: Opaque<bindings::work_struct>,
+    _inner: PhantomData<T>,
+}
+
+// SAFETY: Kernel work items are usable from any thread.
+//
+// We do not need to constrain `T` since the work item does not actually contain a `T`.
+unsafe impl<T: ?Sized, const ID: u64> Send for Work<T, ID> {}
+// SAFETY: Kernel work items are usable from any thread.
+//
+// We do not need to constrain `T` since the work item does not actually contain a `T`.
+unsafe impl<T: ?Sized, const ID: u64> Sync for Work<T, ID> {}
+
+impl<T: ?Sized, const ID: u64> Work<T, ID> {
+    /// Creates a new instance of [`Work`].
+    #[inline]
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
+    where
+        T: WorkItem<ID>,
+    {
+        // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the work
+        // item function.
+        unsafe {
+            kernel::init::pin_init_from_closure(move |slot| {
+                let slot = Self::raw_get(slot);
+                bindings::init_work_with_key(
+                    slot,
+                    Some(T::Pointer::run),
+                    false,
+                    name.as_char_ptr(),
+                    key.as_ptr(),
+                );
+                Ok(())
+            })
+        }
+    }
+
+    /// Get a pointer to the inner `work_struct`.
+    ///
+    /// # Safety
+    ///
+    /// The provided pointer must not be dangling and must be properly aligned. (But the memory
+    /// need not be initialized.)
+    #[inline]
+    pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
+        // SAFETY: The caller promises that the pointer is aligned and not dangling.
+        //
+        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
+        // the compiler does not complain that the `work` field is unused.
+        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
+    }
+}
+
+/// Declares that a type has a [`Work<T, ID>`] field.
+///
+/// The intended way of using this trait is via the [`impl_has_work!`] macro. You can use the macro
+/// like this:
+///
+/// ```no_run
+/// use kernel::impl_has_work;
+/// use kernel::prelude::*;
+/// use kernel::workqueue::Work;
+///
+/// struct MyWorkItem {
+///     work_field: Work<MyWorkItem, 1>,
+/// }
+///
+/// impl_has_work! {
+///     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
+/// }
+/// ```
+///
+/// Note that since the `Work` type is annotated with an id, you can have several `work_struct`
+/// fields by using a different id for each one.
+///
+/// # 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.
+///
+/// [`Work<T, ID>`]: Work
+/// [`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.
+    ///
+    /// [`Work<T, ID>`]: Work
+    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.
+    ///
+    /// [`Work<T, ID>`]: Work
+    /// [`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`.
+    ///
+    /// [`Work<T, ID>`]: Work
+    #[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> }
+    }
+
+    /// 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`.
+    ///
+    /// [`Work<T, ID>`]: Work
+    #[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 }
+    }
+}
+
+/// Used to safely implement the [`HasWork<T, ID>`] trait.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::impl_has_work;
+/// use kernel::sync::Arc;
+/// use kernel::workqueue::{self, Work};
+///
+/// struct MyStruct {
+///     work_field: Work<MyStruct, 17>,
+/// }
+///
+/// impl_has_work! {
+///     impl HasWork<MyStruct, 17> for MyStruct { self.work_field }
+/// }
+/// ```
+///
+/// [`HasWork<T, ID>`]: HasWork
+#[macro_export]
+macro_rules! impl_has_work {
+    ($(impl$(<$($implarg:ident),*>)?
+       HasWork<$work_type:ty $(, $id:tt)?>
+       for $self:ident $(<$($selfarg:ident),*>)?
+       { self.$field:ident }
+    )*) => {$(
+        // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
+        // type.
+        unsafe impl$(<$($implarg),*>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self $(<$($selfarg),*>)? {
+            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.
+                unsafe {
+                    ::core::ptr::addr_of_mut!((*ptr).$field)
+                }
+            }
+        }
+    )*};
+}
+
 /// Returns the system work queue (`system_wq`).
 ///
 /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 5/7] rust: workqueue: implement `WorkItemPointer` for pointer types

[Alice Ryhl]

This implements the `WorkItemPointer` trait for the pointer types that
you are likely to use the workqueue with. The `Arc` type is for
reference counted objects, and the `Pin<Box<T>>` type is for objects
where the caller has exclusive ownership of the object.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
v3 -> v4:
 * No changes.

 rust/kernel/workqueue.rs | 97 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 251541f1cd68..c1334c48b96b 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -28,8 +28,10 @@
 //!
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
-use crate::{bindings, prelude::*, sync::LockClassKey, types::Opaque};
+use crate::{bindings, prelude::*, sync::Arc, sync::LockClassKey, types::Opaque};
+use alloc::boxed::Box;
 use core::marker::PhantomData;
+use core::pin::Pin;
 
 /// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
 #[macro_export]
@@ -363,6 +365,99 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
     )*};
 }
 
+unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T>
+where
+    T: WorkItem<ID, Pointer = Self>,
+    T: HasWork<T, ID>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`.
+        let ptr = ptr as *mut Work<T, ID>;
+        // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
+        let ptr = unsafe { T::work_container_of(ptr) };
+        // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
+        let arc = unsafe { Arc::from_raw(ptr) };
+
+        T::run(arc)
+    }
+}
+
+unsafe impl<T, const ID: u64> RawWorkItem<ID> for Arc<T>
+where
+    T: WorkItem<ID, Pointer = Self>,
+    T: HasWork<T, ID>,
+{
+    type EnqueueOutput = Result<(), Self>;
+
+    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
+    where
+        F: FnOnce(*mut bindings::work_struct) -> bool,
+    {
+        // Casting between const and mut is not a problem as long as the pointer is a raw pointer.
+        let ptr = Arc::into_raw(self).cast_mut();
+
+        // SAFETY: Pointers into an `Arc` point at a valid value.
+        let work_ptr = unsafe { T::raw_get_work(ptr) };
+        // SAFETY: `raw_get_work` returns a pointer to a valid value.
+        let work_ptr = unsafe { Work::raw_get(work_ptr) };
+
+        if queue_work_on(work_ptr) {
+            Ok(())
+        } else {
+            // SAFETY: The work queue has not taken ownership of the pointer.
+            Err(unsafe { Arc::from_raw(ptr) })
+        }
+    }
+}
+
+unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>>
+where
+    T: WorkItem<ID, Pointer = Self>,
+    T: HasWork<T, ID>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`.
+        let ptr = ptr as *mut Work<T, ID>;
+        // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
+        let ptr = unsafe { T::work_container_of(ptr) };
+        // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
+        let boxed = unsafe { Box::from_raw(ptr) };
+        // SAFETY: The box was already pinned when it was enqueued.
+        let pinned = unsafe { Pin::new_unchecked(boxed) };
+
+        T::run(pinned)
+    }
+}
+
+unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<Box<T>>
+where
+    T: WorkItem<ID, Pointer = Self>,
+    T: HasWork<T, ID>,
+{
+    type EnqueueOutput = ();
+
+    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
+    where
+        F: FnOnce(*mut bindings::work_struct) -> bool,
+    {
+        // SAFETY: We're not going to move `self` or any of its fields, so its okay to temporarily
+        // remove the `Pin` wrapper.
+        let boxed = unsafe { Pin::into_inner_unchecked(self) };
+        let ptr = Box::into_raw(boxed);
+
+        // SAFETY: Pointers into a `Box` point at a valid value.
+        let work_ptr = unsafe { T::raw_get_work(ptr) };
+        // SAFETY: `raw_get_work` returns a pointer to a valid value.
+        let work_ptr = unsafe { Work::raw_get(work_ptr) };
+
+        if !queue_work_on(work_ptr) {
+            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
+            // workqueue.
+            unsafe { ::core::hint::unreachable_unchecked() }
+        }
+    }
+}
+
 /// Returns the system work queue (`system_wq`).
 ///
 /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 6/7] rust: workqueue: add `try_spawn` helper method

[Alice Ryhl]

This adds a convenience method that lets you spawn a closure for
execution on a workqueue. This will be the most convenient way to use
workqueues, but it is fallible because it needs to allocate memory.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
v3 -> v4:
 * Use new `new_work!` macro.

 rust/kernel/workqueue.rs | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index c1334c48b96b..40ccc53f6e93 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -29,6 +29,7 @@
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
 use crate::{bindings, prelude::*, sync::Arc, sync::LockClassKey, types::Opaque};
+use alloc::alloc::AllocError;
 use alloc::boxed::Box;
 use core::marker::PhantomData;
 use core::pin::Pin;
@@ -96,6 +97,44 @@ pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
             })
         }
     }
+
+    /// Tries to spawn the given function or closure as a work item.
+    ///
+    /// This method can fail because it allocates memory to store the work item.
+    pub fn try_spawn<T: 'static + Send + FnOnce()>(&self, func: T) -> Result<(), AllocError> {
+        let init = pin_init!(ClosureWork {
+            work <- new_work!("Queue::try_spawn"),
+            func: Some(func),
+        });
+
+        self.enqueue(Box::pin_init(init).map_err(|_| AllocError)?);
+        Ok(())
+    }
+}
+
+/// A helper type used in `try_spawn`.
+#[pin_data]
+struct ClosureWork<T> {
+    #[pin]
+    work: Work<ClosureWork<T>>,
+    func: Option<T>,
+}
+
+impl<T> ClosureWork<T> {
+    fn project(self: Pin<&mut Self>) -> &mut Option<T> {
+        // SAFETY: The `func` field is not structurally pinned.
+        unsafe { &mut self.get_unchecked_mut().func }
+    }
+}
+
+impl<T: FnOnce()> WorkItem for ClosureWork<T> {
+    type Pointer = Pin<Box<Self>>;
+
+    fn run(mut this: Pin<Box<Self>>) {
+        if let Some(func) = this.as_mut().project().take() {
+            (func)()
+        }
+    }
 }
 
 /// A raw work item.
@@ -365,6 +404,10 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
     )*};
 }
 
+impl_has_work! {
+    impl<T> HasWork<Self> for ClosureWork<T> { self.work }
+}
+
 unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T>
 where
     T: WorkItem<ID, Pointer = Self>,
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v4 7/7] rust: workqueue: add examples

[Alice Ryhl]

This adds two examples of how to use the workqueue. The first example
shows how to use it when you only have one `work_struct` field, and the
second example shows how to use it when you have multiple `work_struct`
fields.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
---
v3 -> v4:
 * Use `new_work!` macro.
 * Fix example imports.

 rust/kernel/workqueue.rs | 106 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 40ccc53f6e93..b67fb1ba168e 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -26,6 +26,112 @@
 //!  * The `WorkItemPointer` trait is implemented for the pointer type that points at a something
 //!    that implements `WorkItem`.
 //!
+//! ## Example
+//!
+//! This example defines a struct that holds an integer and can be scheduled on the workqueue. When
+//! the struct is executed, it will print the integer. Since there is only one `work_struct` field,
+//! we do not need to specify ids for the fields.
+//!
+//! ```
+//! use kernel::prelude::*;
+//! use kernel::sync::Arc;
+//! use kernel::workqueue::{self, Work, WorkItem};
+//! use kernel::{impl_has_work, new_work};
+//!
+//! #[pin_data]
+//! struct MyStruct {
+//!     value: i32,
+//!     #[pin]
+//!     work: Work<MyStruct>,
+//! }
+//!
+//! impl_has_work! {
+//!     impl HasWork<Self> for MyStruct { self.work }
+//! }
+//!
+//! impl MyStruct {
+//!     fn new(value: i32) -> Result<Arc<Self>> {
+//!         Arc::pin_init(pin_init!(MyStruct {
+//!             value,
+//!             work <- new_work!("MyStruct::work"),
+//!         }))
+//!     }
+//! }
+//!
+//! impl WorkItem for MyStruct {
+//!     type Pointer = Arc<MyStruct>;
+//!
+//!     fn run(this: Arc<MyStruct>) {
+//!         pr_info!("The value is: {}", this.value);
+//!     }
+//! }
+//!
+//! /// This method will enqueue the struct for execution on the system workqueue, where its value
+//! /// will be printed.
+//! fn print_later(val: Arc<MyStruct>) {
+//!     let _ = workqueue::system().enqueue(val);
+//! }
+//! ```
+//!
+//! The following example shows how multiple `work_struct` fields can be used:
+//!
+//! ```
+//! use kernel::prelude::*;
+//! use kernel::sync::Arc;
+//! use kernel::workqueue::{self, Work, WorkItem};
+//! use kernel::{impl_has_work, new_work};
+//!
+//! #[pin_data]
+//! struct MyStruct {
+//!     value_1: i32,
+//!     value_2: i32,
+//!     #[pin]
+//!     work_1: Work<MyStruct, 1>,
+//!     #[pin]
+//!     work_2: Work<MyStruct, 2>,
+//! }
+//!
+//! impl_has_work! {
+//!     impl HasWork<Self, 1> for MyStruct { self.work_1 }
+//!     impl HasWork<Self, 2> for MyStruct { self.work_2 }
+//! }
+//!
+//! impl MyStruct {
+//!     fn new(value_1: i32, value_2: i32) -> Result<Arc<Self>> {
+//!         Arc::pin_init(pin_init!(MyStruct {
+//!             value_1,
+//!             value_2,
+//!             work_1 <- new_work!("MyStruct::work_1"),
+//!             work_2 <- new_work!("MyStruct::work_2"),
+//!         }))
+//!     }
+//! }
+//!
+//! impl WorkItem<1> for MyStruct {
+//!     type Pointer = Arc<MyStruct>;
+//!
+//!     fn run(this: Arc<MyStruct>) {
+//!         pr_info!("The value is: {}", this.value_1);
+//!     }
+//! }
+//!
+//! impl WorkItem<2> for MyStruct {
+//!     type Pointer = Arc<MyStruct>;
+//!
+//!     fn run(this: Arc<MyStruct>) {
+//!         pr_info!("The second value is: {}", this.value_2);
+//!     }
+//! }
+//!
+//! fn print_1_later(val: Arc<MyStruct>) {
+//!     let _ = workqueue::system().enqueue::<Arc<MyStruct>, 1>(val);
+//! }
+//!
+//! fn print_2_later(val: Arc<MyStruct>) {
+//!     let _ = workqueue::system().enqueue::<Arc<MyStruct>, 2>(val);
+//! }
+//! ```
+//!
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
 use crate::{bindings, prelude::*, sync::Arc, sync::LockClassKey, types::Opaque};
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> The main challenge with defining `work_struct` fields is making sure
> that the function pointer stored in the `work_struct` is appropriate for
> the work item type it is embedded in. It needs to know the offset of the
> `work_struct` field being used (even if there are several!) so that it
> can do a `container_of`, and it needs to know the type of the work item
> so that it can call into the right user-provided code. All of this needs
> to happen in a way that provides a safe API to the user, so that users
> of the workqueue cannot mix up the function pointers.
>
> There are three important pieces that are relevant when doing this:
>
>  * The pointer type.
>  * The work item struct. This is what the pointer points at.
>  * The `work_struct` field. This is a field of the work item struct.
>
> This patch introduces a separate trait for each piece. The pointer type
> is given a `WorkItemPointer` trait, which pointer types need to
> implement to be usable with the workqueue. This trait will be
> implemented for `Arc` and `Box` in a later patch in this patchset.
> Implementing this trait is unsafe because this is where the
> `container_of` operation happens, but user-code will not need to
> implement it themselves.
>
> The work item struct should then implement the `WorkItem` trait. This
> trait is where user-code specifies what they want to happen when a work
> item is executed. It also specifies what the correct pointer type is.
>
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T, ID>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T, ID>`. The trait is marked unsafe
> because the OFFSET constant must be correct, but we provide an
> `impl_has_work!` macro that can safely implement `HasWork<T>` on a type.
> The macro expands to something that only compiles if the specified field
> really has the type `Work<T>`. It is used like this:
>
> ```
> struct MyWorkItem {
>     work_field: Work<MyWorkItem, 1>,
> }
>
> impl_has_work! {
>     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> }
> ```
>
> Note that since the `Work` type is annotated with an id, you can have
> several `work_struct` fields by using a different id for each one.
>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---

Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Martin Rodriguez Reboredo]

On 8/28/23 07:48, Alice Ryhl wrote:
> The main challenge with defining `work_struct` fields is making sure
> that the function pointer stored in the `work_struct` is appropriate for
> the work item type it is embedded in. It needs to know the offset of the
> `work_struct` field being used (even if there are several!) so that it
> can do a `container_of`, and it needs to know the type of the work item
> so that it can call into the right user-provided code. All of this needs
> to happen in a way that provides a safe API to the user, so that users
> of the workqueue cannot mix up the function pointers.
> 
> There are three important pieces that are relevant when doing this:
> 
>   * The pointer type.
>   * The work item struct. This is what the pointer points at.
>   * The `work_struct` field. This is a field of the work item struct.
> 
> This patch introduces a separate trait for each piece. The pointer type
> is given a `WorkItemPointer` trait, which pointer types need to
> implement to be usable with the workqueue. This trait will be
> implemented for `Arc` and `Box` in a later patch in this patchset.
> Implementing this trait is unsafe because this is where the
> `container_of` operation happens, but user-code will not need to
> implement it themselves.
> 
> The work item struct should then implement the `WorkItem` trait. This
> trait is where user-code specifies what they want to happen when a work
> item is executed. It also specifies what the correct pointer type is.
> 
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T, ID>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T, ID>`. The trait is marked unsafe
> because the OFFSET constant must be correct, but we provide an
> `impl_has_work!` macro that can safely implement `HasWork<T>` on a type.
> The macro expands to something that only compiles if the specified field
> really has the type `Work<T>`. It is used like this:
> 
> ```
> struct MyWorkItem {
>      work_field: Work<MyWorkItem, 1>,
> }
> 
> impl_has_work! {
>      impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> }
> ```
> 
> Note that since the `Work` type is annotated with an id, you can have
> several `work_struct` fields by using a different id for each one.
> 
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Benno Lossin]

> The main challenge with defining `work_struct` fields is making sure
> that the function pointer stored in the `work_struct` is appropriate for
> the work item type it is embedded in. It needs to know the offset of the
> `work_struct` field being used (even if there are several!) so that it
> can do a `container_of`, and it needs to know the type of the work item
> so that it can call into the right user-provided code. All of this needs
> to happen in a way that provides a safe API to the user, so that users
> of the workqueue cannot mix up the function pointers.
> 
> There are three important pieces that are relevant when doing this:
> 
>  * The pointer type.
>  * The work item struct. This is what the pointer points at.
>  * The `work_struct` field. This is a field of the work item struct.
> 
> This patch introduces a separate trait for each piece. The pointer type
> is given a `WorkItemPointer` trait, which pointer types need to
> implement to be usable with the workqueue. This trait will be
> implemented for `Arc` and `Box` in a later patch in this patchset.
> Implementing this trait is unsafe because this is where the
> `container_of` operation happens, but user-code will not need to
> implement it themselves.
> 
> The work item struct should then implement the `WorkItem` trait. This
> trait is where user-code specifies what they want to happen when a work
> item is executed. It also specifies what the correct pointer type is.
> 
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T, ID>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T, ID>`. The trait is marked unsafe
> because the OFFSET constant must be correct, but we provide an
> `impl_has_work!` macro that can safely implement `HasWork<T>` on a type.
> The macro expands to something that only compiles if the specified field
> really has the type `Work<T>`. It is used like this:
> 
> ```
> struct MyWorkItem {
>     work_field: Work<MyWorkItem, 1>,
> }
> 
> impl_has_work! {
>     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> }
> ```
> 
> Note that since the `Work` type is annotated with an id, you can have
> several `work_struct` fields by using a different id for each one.
> 
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

I have a small suggestion below, but nothing critical, so

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> v3 -> v4:
>  * The helper was changed to take a name argument, and is implemented
>    directly.
>  * `Work::new` now takes a name argument, and a `new_work!` macro was
>    introduced to help call it.
>  * Use `core::mem::offset_of` rather than a custom implementation.
>  * Fix imports in examples.
>  * Dropped Reviewed-bys due to changes.
> 
>  rust/helpers.c           |  13 ++
>  rust/kernel/lib.rs       |   1 +
>  rust/kernel/workqueue.rs | 257 ++++++++++++++++++++++++++++++++++++++-
>  scripts/Makefile.build   |   2 +-
>  4 files changed, 271 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index ebd69490127b..45cf9702d4e4 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -29,6 +29,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/spinlock.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
> 
>  __noreturn void rust_helper_BUG(void)
>  {
> @@ -137,6 +138,18 @@ void rust_helper_put_task_struct(struct task_struct *t)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
> 
> +void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> +				    bool onstack, const char *name,
> +				    struct lock_class_key *key)
> +{
> +	__init_work(work, onstack);
> +	work->data = (atomic_long_t)WORK_DATA_INIT();
> +	lockdep_init_map(&work->lockdep_map, name, key, 0);
> +	INIT_LIST_HEAD(&work->entry);
> +	work->func = func;
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> +
>  /*
>   * `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/lib.rs b/rust/kernel/lib.rs
> index b56aaea3de7a..c537d2edb4c8 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -16,6 +16,7 @@
>  #![feature(coerce_unsized)]
>  #![feature(dispatch_from_dyn)]
>  #![feature(new_uninit)]
> +#![feature(offset_of)]
>  #![feature(ptr_metadata)]
>  #![feature(receiver_trait)]
>  #![feature(unsize)]
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 82e3fb19fdaf..da37bfa97211 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -262,7 +262,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
> 
> -rust_allowed_features := new_uninit
> +rust_allowed_features := new_uninit,offset_of
> 
>  # `--out-dir` is required to avoid temporaries being created by `rustc` in the
>  # current working directory, which may be not accessible in the out-of-tree
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 6dbc5b9b3da9..251541f1cd68 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -2,9 +2,42 @@
> 
>  //! Work queues.
>  //!
> +//! This file has two components: The raw work item API, and the safe work item API.
> +//!
> +//! One pattern that is used in both APIs is the `ID` const generic, which exists to allow a single
> +//! type to define multiple `work_struct` fields. This is done by choosing an id for each field,
> +//! and using that id to specify which field you wish to use. (The actual value doesn't matter, as
> +//! long as you use different values for different fields of the same struct.) Since these IDs are
> +//! generic, they are used only at compile-time, so they shouldn't exist in the final binary.
> +//!
> +//! # The raw API
> +//!
> +//! The raw API consists of the `RawWorkItem` trait, where the work item needs to provide an
> +//! arbitrary function that knows how to enqueue the work item. It should usually not be used
> +//! directly, but if you want to, you can use it without using the pieces from the safe API.
> +//!
> +//! # The safe API
> +//!
> +//! The safe API is used via the `Work` struct and `WorkItem` traits. Furthermore, it also includes
> +//! a trait called `WorkItemPointer`, which is usually not used directly by the user.
> +//!
> +//!  * The `Work` struct is the Rust wrapper for the C `work_struct` type.
> +//!  * The `WorkItem` trait is implemented for structs that can be enqueued to a workqueue.
> +//!  * The `WorkItemPointer` trait is implemented for the pointer type that points at a something
> +//!    that implements `WorkItem`.
> +//!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
> 
> -use crate::{bindings, types::Opaque};
> +use crate::{bindings, prelude::*, sync::LockClassKey, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
> +#[macro_export]
> +macro_rules! new_work {
> +    ($($name:literal)?) => {
> +        $crate::workqueue::Work::new($crate::optional_name!($($name)?), $crate::static_lock_class!())
> +    };
> +}
> 
>  /// A kernel work queue.
>  ///
> @@ -108,6 +141,228 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>          F: FnOnce(*mut bindings::work_struct) -> bool;
>  }
> 
> +/// Defines the method that should be called directly when a work item is executed.
> +///
> +/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
> +/// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
> +/// instead. The `run` method on this trait will usually just perform the appropriate
> +/// `container_of` translation and then call into the `run` method from the [`WorkItem`] trait.
> +///
> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
> +/// method of this trait as the function pointer.
> +///
> +/// [`__enqueue`]: RawWorkItem::__enqueue
> +/// [`run`]: WorkItemPointer::run
> +pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
> +    /// Run this work item.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
> +    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
> +}
> +
> +/// Defines the method that should be called when this work item is executed.
> +///
> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
> +pub trait WorkItem<const ID: u64 = 0> {
> +    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
> +    /// `Pin<Box<Self>>`.
> +    type Pointer: WorkItemPointer<ID>;
> +
> +    /// The method that should be called when this work item is executed.
> +    fn run(this: Self::Pointer);
> +}
> +
> +/// Links for a work item.
> +///
> +/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
> +/// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
> +///
> +/// Wraps the kernel's C `struct work_struct`.
> +///
> +/// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
> +#[repr(transparent)]
> +pub struct Work<T: ?Sized, const ID: u64 = 0> {
> +    work: Opaque<bindings::work_struct>,
> +    _inner: PhantomData<T>,
> +}
> +
> +// SAFETY: Kernel work items are usable from any thread.
> +//
> +// We do not need to constrain `T` since the work item does not actually contain a `T`.
> +unsafe impl<T: ?Sized, const ID: u64> Send for Work<T, ID> {}
> +// SAFETY: Kernel work items are usable from any thread.
> +//
> +// We do not need to constrain `T` since the work item does not actually contain a `T`.
> +unsafe impl<T: ?Sized, const ID: u64> Sync for Work<T, ID> {}
> +
> +impl<T: ?Sized, const ID: u64> Work<T, ID> {
> +    /// Creates a new instance of [`Work`].
> +    #[inline]
> +    #[allow(clippy::new_ret_no_self)]
> +    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
> +    where
> +        T: WorkItem<ID>,
> +    {
> +        // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the work
> +        // item function.
> +        unsafe {
> +            kernel::init::pin_init_from_closure(move |slot| {
> +                let slot = Self::raw_get(slot);
> +                bindings::init_work_with_key(
> +                    slot,
> +                    Some(T::Pointer::run),
> +                    false,
> +                    name.as_char_ptr(),
> +                    key.as_ptr(),
> +                );
> +                Ok(())
> +            })
> +        }

I would suggest this instead:
```
        pin_init!(Self {
            // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the
            // work item function.
            work <- Opaque::ffi_init(|slot| unsafe {
                bindings::init_work_with_key(
                    slot,
                    Some(T::Pointer::run),
                    false,
                    name.as_char_ptr(),
                    key.as_ptr(),
                )
            }),
            _inner: PhantomData,
        })
```

> +    }
> +
> +    /// Get a pointer to the inner `work_struct`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided pointer must not be dangling and must be properly aligned. (But the memory
> +    /// need not be initialized.)
> +    #[inline]
> +    pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> +        // SAFETY: The caller promises that the pointer is aligned and not dangling.
> +        //
> +        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
> +        // the compiler does not complain that the `work` field is unused.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
> +    }
> +}
> +
> +/// Declares that a type has a [`Work<T, ID>`] field.
> +///
> +/// The intended way of using this trait is via the [`impl_has_work!`] macro. You can use the macro
> +/// like this:
> +///
> +/// ```no_run
> +/// use kernel::impl_has_work;
> +/// use kernel::prelude::*;
> +/// use kernel::workqueue::Work;
> +///
> +/// struct MyWorkItem {
> +///     work_field: Work<MyWorkItem, 1>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> +/// }
> +/// ```
> +///
> +/// Note that since the `Work` type is annotated with an id, you can have several `work_struct`
> +/// fields by using a different id for each one.
> +///
> +/// # 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.
> +///
> +/// [`Work<T, ID>`]: Work
> +/// [`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.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    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.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    /// [`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`.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    #[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> }
> +    }
> +
> +    /// 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`.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    #[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 }
> +    }
> +}
> +
> +/// Used to safely implement the [`HasWork<T, ID>`] trait.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::impl_has_work;
> +/// use kernel::sync::Arc;
> +/// use kernel::workqueue::{self, Work};
> +///
> +/// struct MyStruct {
> +///     work_field: Work<MyStruct, 17>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyStruct, 17> for MyStruct { self.work_field }
> +/// }
> +/// ```
> +///
> +/// [`HasWork<T, ID>`]: HasWork
> +#[macro_export]
> +macro_rules! impl_has_work {
> +    ($(impl$(<$($implarg:ident),*>)?
> +       HasWork<$work_type:ty $(, $id:tt)?>
> +       for $self:ident $(<$($selfarg:ident),*>)?
> +       { self.$field:ident }
> +    )*) => {$(
> +        // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
> +        // type.
> +        unsafe impl$(<$($implarg),*>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self $(<$($selfarg),*>)? {
> +            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.
> +                unsafe {
> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
> +                }
> +            }
> +        }
> +    )*};
> +}
> +
>  /// Returns the system work queue (`system_wq`).
>  ///
>  /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
> --
> 2.42.0.rc1.204.g551eb34607-goog
> 

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Alice Ryhl]

Benno Lossin <benno.lossin@proton.me> writes:
>> +impl<T: ?Sized, const ID: u64> Work<T, ID> {
>> +    /// Creates a new instance of [`Work`].
>> +    #[inline]
>> +    #[allow(clippy::new_ret_no_self)]
>> +    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
>> +    where
>> +        T: WorkItem<ID>,
>> +    {
>> +        // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the work
>> +        // item function.
>> +        unsafe {
>> +            kernel::init::pin_init_from_closure(move |slot| {
>> +                let slot = Self::raw_get(slot);
>> +                bindings::init_work_with_key(
>> +                    slot,
>> +                    Some(T::Pointer::run),
>> +                    false,
>> +                    name.as_char_ptr(),
>> +                    key.as_ptr(),
>> +                );
>> +                Ok(())
>> +            })
>> +        }
> 
> I would suggest this instead:
> ```
>         pin_init!(Self {
>             // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the
>             // work item function.
>             work <- Opaque::ffi_init(|slot| unsafe {
>                 bindings::init_work_with_key(
>                     slot,
>                     Some(T::Pointer::run),
>                     false,
>                     name.as_char_ptr(),
>                     key.as_ptr(),
>                 )
>             }),
>             _inner: PhantomData,
>         })
> ```

I thought that I changed this in this patchset ...

Anyway, I don't think it's a big deal. If I need to send a v5 for some
other reason, then I will fix this there. Otherwise, I don't think it's
necessary to send a v5 just for this.

Alice

Re: [PATCH v4 0/7] rust: workqueue: add bindings for the workqueue

[Boqun Feng]

On Mon, Aug 28, 2023 at 10:48:00AM +0000, Alice Ryhl wrote:
> This patchset contains bindings for the kernel workqueue.
> 
> One of the primary goals behind the design used in this patch is that we
> must support embedding the `work_struct` as a field in user-provided
> types, because this allows you to submit things to the workqueue without
> having to allocate, making the submission infallible. If we didn't have
> to support this, then the patch would be much simpler. One of the main
> things that make it complicated is that we must ensure that the function
> pointer in the `work_struct` is compatible with the struct it is
> contained within.
> 
> The original version of the workqueue bindings was written by Wedson,
> but I have rewritten much of it so that it uses the pin-init
> infrastructure and can be used with containers other than `Arc`.
> 
> This patchset depends on the patch [1] that changes `Opaque` to be
> `!Unpin` for correctness. The patchset is based on the rust-6.6 tag,
> since that tag includes that patch.
> 
> Changes v3 -> v4:
> 
>  * Use `offset_of!` macro from standard library, and drop patch with
>    custom implementation.
>  * Add name parameter to `work_struct` constructor, and drop patch with
>    __INIT_WORK_WITH_KEY.
>  * Various improvements to documentation and comments.
> 
> More detailed changelogs can be found on the individual patches.
> 

For the whole series:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> Previous versions:
> v1: https://lore.kernel.org/all/20230517203119.3160435-1-aliceryhl@google.com/
> v2: https://lore.kernel.org/all/20230601134946.3887870-1-aliceryhl@google.com/
> v3: https://lore.kernel.org/all/20230711093303.1433770-1-aliceryhl@google.com/
> 
> [1]: https://lore.kernel.org/all/20230630150216.109789-1-benno.lossin@proton.me/
> 
> Alice Ryhl (5):
>   rust: workqueue: add low-level workqueue bindings
>   rust: workqueue: add helper for defining work_struct fields
>   rust: workqueue: implement `WorkItemPointer` for pointer types
>   rust: workqueue: add `try_spawn` helper method
>   rust: workqueue: add examples
> 
> Wedson Almeida Filho (2):
>   rust: sync: add `Arc::{from_raw, into_raw}`
>   rust: workqueue: define built-in queues
> 
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers.c                  |  13 +
>  rust/kernel/lib.rs              |   3 +
>  rust/kernel/sync/arc.rs         |  46 ++-
>  rust/kernel/workqueue.rs        | 679 ++++++++++++++++++++++++++++++++
>  scripts/Makefile.build          |   2 +-
>  6 files changed, 741 insertions(+), 3 deletions(-)
>  create mode 100644 rust/kernel/workqueue.rs
> 
> base-commit: 4af84c6a85c63bec24611e46bb3de2c0a6602a51
> -- 
> 2.42.0.rc1.204.g551eb34607-goog
> 

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Gary Guo]

On Mon, 28 Aug 2023 10:48:04 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> The main challenge with defining `work_struct` fields is making sure
> that the function pointer stored in the `work_struct` is appropriate for
> the work item type it is embedded in. It needs to know the offset of the
> `work_struct` field being used (even if there are several!) so that it
> can do a `container_of`, and it needs to know the type of the work item
> so that it can call into the right user-provided code. All of this needs
> to happen in a way that provides a safe API to the user, so that users
> of the workqueue cannot mix up the function pointers.
> 
> There are three important pieces that are relevant when doing this:
> 
>  * The pointer type.
>  * The work item struct. This is what the pointer points at.
>  * The `work_struct` field. This is a field of the work item struct.
> 
> This patch introduces a separate trait for each piece. The pointer type
> is given a `WorkItemPointer` trait, which pointer types need to
> implement to be usable with the workqueue. This trait will be
> implemented for `Arc` and `Box` in a later patch in this patchset.
> Implementing this trait is unsafe because this is where the
> `container_of` operation happens, but user-code will not need to
> implement it themselves.
> 
> The work item struct should then implement the `WorkItem` trait. This
> trait is where user-code specifies what they want to happen when a work
> item is executed. It also specifies what the correct pointer type is.
> 
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T, ID>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T, ID>`. The trait is marked unsafe
> because the OFFSET constant must be correct, but we provide an
> `impl_has_work!` macro that can safely implement `HasWork<T>` on a type.
> The macro expands to something that only compiles if the specified field
> really has the type `Work<T>`. It is used like this:
> 
> ```
> struct MyWorkItem {
>     work_field: Work<MyWorkItem, 1>,
> }
> 
> impl_has_work! {
>     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> }
> ```
> 
> Note that since the `Work` type is annotated with an id, you can have
> several `work_struct` fields by using a different id for each one.
> 
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> v3 -> v4:
>  * The helper was changed to take a name argument, and is implemented
>    directly.
>  * `Work::new` now takes a name argument, and a `new_work!` macro was
>    introduced to help call it.
>  * Use `core::mem::offset_of` rather than a custom implementation.
>  * Fix imports in examples.
>  * Dropped Reviewed-bys due to changes.
> 
>  rust/helpers.c           |  13 ++
>  rust/kernel/lib.rs       |   1 +
>  rust/kernel/workqueue.rs | 257 ++++++++++++++++++++++++++++++++++++++-
>  scripts/Makefile.build   |   2 +-
>  4 files changed, 271 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index ebd69490127b..45cf9702d4e4 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -29,6 +29,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/spinlock.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  
>  __noreturn void rust_helper_BUG(void)
>  {
> @@ -137,6 +138,18 @@ void rust_helper_put_task_struct(struct task_struct *t)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
>  
> +void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> +				    bool onstack, const char *name,
> +				    struct lock_class_key *key)
> +{
> +	__init_work(work, onstack);
> +	work->data = (atomic_long_t)WORK_DATA_INIT();
> +	lockdep_init_map(&work->lockdep_map, name, key, 0);
> +	INIT_LIST_HEAD(&work->entry);
> +	work->func = func;
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> +
>  /*
>   * `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/lib.rs b/rust/kernel/lib.rs
> index b56aaea3de7a..c537d2edb4c8 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -16,6 +16,7 @@
>  #![feature(coerce_unsized)]
>  #![feature(dispatch_from_dyn)]
>  #![feature(new_uninit)]
> +#![feature(offset_of)]
>  #![feature(ptr_metadata)]
>  #![feature(receiver_trait)]
>  #![feature(unsize)]
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 82e3fb19fdaf..da37bfa97211 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -262,7 +262,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
>  
> -rust_allowed_features := new_uninit
> +rust_allowed_features := new_uninit,offset_of
>  
>  # `--out-dir` is required to avoid temporaries being created by `rustc` in the
>  # current working directory, which may be not accessible in the out-of-tree
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 6dbc5b9b3da9..251541f1cd68 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -2,9 +2,42 @@
>  
>  //! Work queues.
>  //!
> +//! This file has two components: The raw work item API, and the safe work item API.
> +//!
> +//! One pattern that is used in both APIs is the `ID` const generic, which exists to allow a single
> +//! type to define multiple `work_struct` fields. This is done by choosing an id for each field,
> +//! and using that id to specify which field you wish to use. (The actual value doesn't matter, as
> +//! long as you use different values for different fields of the same struct.) Since these IDs are
> +//! generic, they are used only at compile-time, so they shouldn't exist in the final binary.
> +//!
> +//! # The raw API
> +//!
> +//! The raw API consists of the `RawWorkItem` trait, where the work item needs to provide an
> +//! arbitrary function that knows how to enqueue the work item. It should usually not be used
> +//! directly, but if you want to, you can use it without using the pieces from the safe API.
> +//!
> +//! # The safe API
> +//!
> +//! The safe API is used via the `Work` struct and `WorkItem` traits. Furthermore, it also includes
> +//! a trait called `WorkItemPointer`, which is usually not used directly by the user.
> +//!
> +//!  * The `Work` struct is the Rust wrapper for the C `work_struct` type.
> +//!  * The `WorkItem` trait is implemented for structs that can be enqueued to a workqueue.
> +//!  * The `WorkItemPointer` trait is implemented for the pointer type that points at a something
> +//!    that implements `WorkItem`.
> +//!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
>  
> -use crate::{bindings, types::Opaque};
> +use crate::{bindings, prelude::*, sync::LockClassKey, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Creates a [`Work`] initialiser with the given name and a newly-created lock class.
> +#[macro_export]
> +macro_rules! new_work {
> +    ($($name:literal)?) => {
> +        $crate::workqueue::Work::new($crate::optional_name!($($name)?), $crate::static_lock_class!())
> +    };
> +}
>  
>  /// A kernel work queue.
>  ///
> @@ -108,6 +141,228 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>          F: FnOnce(*mut bindings::work_struct) -> bool;
>  }
>  
> +/// Defines the method that should be called directly when a work item is executed.
> +///
> +/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
> +/// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
> +/// instead. The `run` method on this trait will usually just perform the appropriate
> +/// `container_of` translation and then call into the `run` method from the [`WorkItem`] trait.
> +///
> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
> +/// method of this trait as the function pointer.
> +///
> +/// [`__enqueue`]: RawWorkItem::__enqueue
> +/// [`run`]: WorkItemPointer::run
> +pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
> +    /// Run this work item.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
> +    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
> +}
> +
> +/// Defines the method that should be called when this work item is executed.
> +///
> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
> +pub trait WorkItem<const ID: u64 = 0> {
> +    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
> +    /// `Pin<Box<Self>>`.
> +    type Pointer: WorkItemPointer<ID>;
> +
> +    /// The method that should be called when this work item is executed.
> +    fn run(this: Self::Pointer);
> +}
> +
> +/// Links for a work item.
> +///
> +/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
> +/// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
> +///
> +/// Wraps the kernel's C `struct work_struct`.
> +///
> +/// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
> +#[repr(transparent)]
> +pub struct Work<T: ?Sized, const ID: u64 = 0> {
> +    work: Opaque<bindings::work_struct>,
> +    _inner: PhantomData<T>,
> +}
> +
> +// SAFETY: Kernel work items are usable from any thread.
> +//
> +// We do not need to constrain `T` since the work item does not actually contain a `T`.
> +unsafe impl<T: ?Sized, const ID: u64> Send for Work<T, ID> {}
> +// SAFETY: Kernel work items are usable from any thread.
> +//
> +// We do not need to constrain `T` since the work item does not actually contain a `T`.
> +unsafe impl<T: ?Sized, const ID: u64> Sync for Work<T, ID> {}
> +
> +impl<T: ?Sized, const ID: u64> Work<T, ID> {
> +    /// Creates a new instance of [`Work`].
> +    #[inline]
> +    #[allow(clippy::new_ret_no_self)]

nit: this line can now be removed.

> +    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
> +    where
> +        T: WorkItem<ID>,
> +    {
> +        // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the work
> +        // item function.
> +        unsafe {
> +            kernel::init::pin_init_from_closure(move |slot| {
> +                let slot = Self::raw_get(slot);
> +                bindings::init_work_with_key(
> +                    slot,
> +                    Some(T::Pointer::run),
> +                    false,
> +                    name.as_char_ptr(),
> +                    key.as_ptr(),
> +                );
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Get a pointer to the inner `work_struct`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided pointer must not be dangling and must be properly aligned. (But the memory
> +    /// need not be initialized.)
> +    #[inline]
> +    pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> +        // SAFETY: The caller promises that the pointer is aligned and not dangling.
> +        //
> +        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
> +        // the compiler does not complain that the `work` field is unused.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
> +    }
> +}
> +
> +/// Declares that a type has a [`Work<T, ID>`] field.
> +///
> +/// The intended way of using this trait is via the [`impl_has_work!`] macro. You can use the macro
> +/// like this:
> +///
> +/// ```no_run
> +/// use kernel::impl_has_work;
> +/// use kernel::prelude::*;
> +/// use kernel::workqueue::Work;
> +///
> +/// struct MyWorkItem {
> +///     work_field: Work<MyWorkItem, 1>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyWorkItem, 1> for MyWorkItem { self.work_field }
> +/// }
> +/// ```
> +///
> +/// Note that since the `Work` type is annotated with an id, you can have several `work_struct`
> +/// fields by using a different id for each one.
> +///
> +/// # 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.
> +///
> +/// [`Work<T, ID>`]: Work
> +/// [`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.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    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.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    /// [`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`.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    #[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> }
> +    }
> +
> +    /// 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`.
> +    ///
> +    /// [`Work<T, ID>`]: Work
> +    #[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 }
> +    }
> +}
> +
> +/// Used to safely implement the [`HasWork<T, ID>`] trait.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::impl_has_work;
> +/// use kernel::sync::Arc;
> +/// use kernel::workqueue::{self, Work};
> +///
> +/// struct MyStruct {
> +///     work_field: Work<MyStruct, 17>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyStruct, 17> for MyStruct { self.work_field }
> +/// }
> +/// ```
> +///
> +/// [`HasWork<T, ID>`]: HasWork
> +#[macro_export]
> +macro_rules! impl_has_work {
> +    ($(impl$(<$($implarg:ident),*>)?
> +       HasWork<$work_type:ty $(, $id:tt)?>
> +       for $self:ident $(<$($selfarg:ident),*>)?
> +       { self.$field:ident }
> +    )*) => {$(
> +        // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
> +        // type.
> +        unsafe impl$(<$($implarg),*>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self $(<$($selfarg),*>)? {
> +            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.
> +                unsafe {
> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
> +                }
> +            }
> +        }
> +    )*};
> +}
> +
>  /// Returns the system work queue (`system_wq`).
>  ///
>  /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are

Re: [PATCH v4 0/7] rust: workqueue: add bindings for the workqueue

[Tejun Heo]

On Mon, Aug 28, 2023 at 10:48:00AM +0000, Alice Ryhl wrote:
> This patchset contains bindings for the kernel workqueue.
> 
> One of the primary goals behind the design used in this patch is that we
> must support embedding the `work_struct` as a field in user-provided
> types, because this allows you to submit things to the workqueue without
> having to allocate, making the submission infallible. If we didn't have
> to support this, then the patch would be much simpler. One of the main
> things that make it complicated is that we must ensure that the function
> pointer in the `work_struct` is compatible with the struct it is
> contained within.
> 
> The original version of the workqueue bindings was written by Wedson,
> but I have rewritten much of it so that it uses the pin-init
> infrastructure and can be used with containers other than `Arc`.
> 
> This patchset depends on the patch [1] that changes `Opaque` to be
> `!Unpin` for correctness. The patchset is based on the rust-6.6 tag,
> since that tag includes that patch.

Applied to wq/for-6.7-rust-bindings with Boqun's reviewed-by's added.

Thanks.

-- 
tejun

Re: [PATCH v4 4/7] rust: workqueue: add helper for defining work_struct fields

[Alice Ryhl]

>> +impl<T: ?Sized, const ID: u64> Work<T, ID> {
>> +    /// Creates a new instance of [`Work`].
>> +    #[inline]
>> +    #[allow(clippy::new_ret_no_self)]
>> +    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
>> +    where
>> +        T: WorkItem<ID>,
>> +    {
>> +        // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the work
>> +        // item function.
>> +        unsafe {
>> +            kernel::init::pin_init_from_closure(move |slot| {
>> +                let slot = Self::raw_get(slot);
>> +                bindings::init_work_with_key(
>> +                    slot,
>> +                    Some(T::Pointer::run),
>> +                    false,
>> +                    name.as_char_ptr(),
>> +                    key.as_ptr(),
>> +                );
>> +                Ok(())
>> +            })
>> +        }
> 
> I would suggest this instead:
> ```
>         pin_init!(Self {
>             // SAFETY: The `WorkItemPointer` implementation promises that `run` can be used as the
>             // work item function.
>             work <- Opaque::ffi_init(|slot| unsafe {
>                 bindings::init_work_with_key(
>                     slot,
>                     Some(T::Pointer::run),
>                     false,
>                     name.as_char_ptr(),
>                     key.as_ptr(),
>                 )
>             }),
>             _inner: PhantomData,
>         })
> ```

I tried setting up a patch with a few nits fixed, including this one.
However, I ran into an error when adding #[pin_data] to the Work struct:

error: defaults for const parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions
   --> rust/kernel/workqueue.rs:192:28
    |
192 | pub struct Work<T: ?Sized, const ID: u64 = 0> {
    |                            ^^^^^^^^^^^^^^^^^

This nit can be fixed when #[pin_data] is updated to support default
values in struct declarations.

Alice

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Konstantin Shelekhin]

+//! #[pin_data]
+//! struct MyStruct {
+//!     value: i32,
+//!     #[pin]
+//!     work: Work<MyStruct>,
+//! }
+//!
+//! impl_has_work! {
+//!     impl HasWork<Self> for MyStruct { self.work }
+//! }
+//!
+//! impl MyStruct {
+//!     fn new(value: i32) -> Result<Arc<Self>> {
+//!         Arc::pin_init(pin_init!(MyStruct {
+//!             value,
+//!             work <- new_work!("MyStruct::work"),
+//!         }))
+//!     }
+//! }
+//!
+//! impl WorkItem for MyStruct {
+//!     type Pointer = Arc<MyStruct>;
+//!
+//!     fn run(this: Arc<MyStruct>) {
+//!         pr_info!("The value is: {}", this.value);
+//!     }
+//! }
+//!
+//! /// This method will enqueue the struct for execution on the system workqueue, where its value
+//! /// will be printed.
+//! fn print_later(val: Arc<MyStruct>) {
+//!     let _ = workqueue::system().enqueue(val);
+//! }

I understand that this is highly opionated, but is it possible to make
the initialization less verbose?

Because the C version looks much, much cleaner and easier to grasp:

    struct my_struct {
        i32 value;
        struct work_struct work;
    };

    void log_value(struct work_struct *work)
    {
        struct my_struct *s = container_of(work, struct my_struct, work);
        pr_info("The value is: %d\n", s->value);
    }

    void print_later(struct my_struct &s)
    {
        INIT_WORK(&s->work, log_value);
        schedule_work(&s->work);
    }

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Alice Ryhl]

On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@ftml.net> wrote:
> +//! #[pin_data]
> +//! struct MyStruct {
> +//!     value: i32,
> +//!     #[pin]
> +//!     work: Work<MyStruct>,
> +//! }
> +//!
> +//! impl_has_work! {
> +//!     impl HasWork<Self> for MyStruct { self.work }
> +//! }
> +//!
> +//! impl MyStruct {
> +//!     fn new(value: i32) -> Result<Arc<Self>> {
> +//!         Arc::pin_init(pin_init!(MyStruct {
> +//!             value,
> +//!             work <- new_work!("MyStruct::work"),
> +//!         }))
> +//!     }
> +//! }
> +//!
> +//! impl WorkItem for MyStruct {
> +//!     type Pointer = Arc<MyStruct>;
> +//!
> +//!     fn run(this: Arc<MyStruct>) {
> +//!         pr_info!("The value is: {}", this.value);
> +//!     }
> +//! }
> +//!
> +//! /// This method will enqueue the struct for execution on the system workqueue, where its value
> +//! /// will be printed.
> +//! fn print_later(val: Arc<MyStruct>) {
> +//!     let _ = workqueue::system().enqueue(val);
> +//! }
>
> I understand that this is highly opionated, but is it possible to make
> the initialization less verbose?

The short answer is yes. There are safe alternatives that are much less
verbose. Unfortunately, those alternatives give up some of the features
that this design has. Specifically, they give up the feature that allows
you to embed the work_struct inside custom structs. I need to be able to
embed the work_struct inside of custom structs, so I did not go that
route.

There are also some parts of this (mainly `impl_has_work!`) that I am
unhappy with. I would be happy to see a solution that doesn't need it,
but I couldn't figure out how to avoid it.

> Because the C version looks much, much cleaner and easier to grasp:
>
>     struct my_struct {
>         i32 value;
>         struct work_struct work;
>     };
> 
>     void log_value(struct work_struct *work)
>     {
>         struct my_struct *s = container_of(work, struct my_struct, work);
>         pr_info("The value is: %d\n", s->value);
>     }
> 
>     void print_later(struct my_struct &s)
>     {
>         INIT_WORK(&s->work, log_value);
>         schedule_work(&s->work);
>     }

Although I think that a part of this is just whether you are familiar
with Rust syntax, there is definitely some truth to this. Your code is a
lot closer to the machine code of what actually happens here. Perhaps it
would be interesting to see what you get if you just unsafely do exactly
the same thing in Rust? It would look something like this:

    struct MyStruct {
        value: i32,
        work: bindings::work_struct,
    }

    unsafe fn log_value(work: *mut bindings::work_struct) {
        unsafe {
            let s = container_of!(work, MyStruct, work);
            pr_info!("The value is: {}", (*s).value);
        }
    }

    unsafe fn print_later(s: *mut bindings::work_struct) {
        unsafe {
            bindings::INIT_WORK(&mut (*s).work, log_value);
            bindings::schedule_work(&mut (*s).work);
        }
    }

(I didn't try to compile this.)

The problem with this approach is that it uses unsafe in driver code,
but the goal behind Rust abstractions is to isolate all of the related
unsafe code. The idea being that drivers using the workqueue do not need
any unsafe code to use it. This means that, assuming these workqueue
abstractions are correct, no driver can accidentally cause memory
unsafety by using the workqueue wrong.

The main difficult part of making this happen is the container_of
operation. We need to somehow verify *at compile time* that the
container_of in log_value really is given a pointer to the work field of
a MyStruct. Other than the things that are just how Rust looks, most of
the verbosity is necessary to make this compile-time check possible.

Another thing it does is handle proper transfer of ownership. In my
original example, MyStruct is reference counted (due to the use of Arc),
so the workqueue passes ownership of one refcount to the workqueue,
which eventually passes the refcount to run. When `this` goes out of
scope at the end of `run`, the refcount is decremented and the struct is
freed if the refcount dropped to zero.

If you wanted to just have exclusive ownership of my_struct, you could
do that by using Box instead of Arc. In either case, the ownership is
correctly passed to run, and you cannot accidentally forget to free it
at the end of log_value.

So, ultimately there's a tradeoff here. The code corresponds less
directly to what the machine code will be. On the other hand, it will be
*more* difficult to use incorrectly since incorrect uses will usually
result in compilation errors. The claim of Rust is that this tradeoff is
worth it.

Alice

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Konstantin Shelekhin]

On Wed Oct 4, 2023 at 1:29 AM MSK, Alice Ryhl wrote:
> On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@ftml.net> wrote:
> > +//! #[pin_data]
> > +//! struct MyStruct {
> > +//!     value: i32,
> > +//!     #[pin]
> > +//!     work: Work<MyStruct>,
> > +//! }
> > +//!
> > +//! impl_has_work! {
> > +//!     impl HasWork<Self> for MyStruct { self.work }
> > +//! }
> > +//!
> > +//! impl MyStruct {
> > +//!     fn new(value: i32) -> Result<Arc<Self>> {
> > +//!         Arc::pin_init(pin_init!(MyStruct {
> > +//!             value,
> > +//!             work <- new_work!("MyStruct::work"),
> > +//!         }))
> > +//!     }
> > +//! }
> > +//!
> > +//! impl WorkItem for MyStruct {
> > +//!     type Pointer = Arc<MyStruct>;
> > +//!
> > +//!     fn run(this: Arc<MyStruct>) {
> > +//!         pr_info!("The value is: {}", this.value);
> > +//!     }
> > +//! }
> > +//!
> > +//! /// This method will enqueue the struct for execution on the system workqueue, where its value
> > +//! /// will be printed.
> > +//! fn print_later(val: Arc<MyStruct>) {
> > +//!     let _ = workqueue::system().enqueue(val);
> > +//! }
> >
> > I understand that this is highly opionated, but is it possible to make
> > the initialization less verbose?
>
> The short answer is yes. There are safe alternatives that are much less
> verbose. Unfortunately, those alternatives give up some of the features
> that this design has. Specifically, they give up the feature that allows
> you to embed the work_struct inside custom structs. I need to be able to
> embed the work_struct inside of custom structs, so I did not go that
> route.

My concern with the approach of using traits instead of calling an
initialization function is that a some of existing code uses the
following pattern:

    static void nvmet_file_submit_buffered_io(struct nvmet_req *req)
    {
            INIT_WORK(&req->f.work, nvmet_file_buffered_io_work);
            queue_work(buffered_io_wq, &req->f.work);
    }

    static void nvmet_file_execute_flush(struct nvmet_req *req)
    {
            if (!nvmet_check_transfer_len(req, 0))
                    return;
            INIT_WORK(&req->f.work, nvmet_file_flush_work);
            queue_work(nvmet_wq, &req->f.work);
    }

    static void nvmet_file_execute_dsm(struct nvmet_req *req)
    {
            if (!nvmet_check_data_len_lte(req, nvmet_dsm_len(req)))
                    return;
            INIT_WORK(&req->f.work, nvmet_file_dsm_work);
            queue_work(nvmet_wq, &req->f.work);
    }

As you can see a single work struct is used here, and dispatching
happens beforehands. While it's possible to do the dispatching later in
run(), it's IMO cleaner to do this earlier.

> There are also some parts of this (mainly `impl_has_work!`) that I am
> unhappy with. I would be happy to see a solution that doesn't need it,
> but I couldn't figure out how to avoid it.
>
> > Because the C version looks much, much cleaner and easier to grasp:
> >
> >     struct my_struct {
> >         i32 value;
> >         struct work_struct work;
> >     };
> > 
> >     void log_value(struct work_struct *work)
> >     {
> >         struct my_struct *s = container_of(work, struct my_struct, work);
> >         pr_info("The value is: %d\n", s->value);
> >     }
> > 
> >     void print_later(struct my_struct &s)
> >     {
> >         INIT_WORK(&s->work, log_value);
> >         schedule_work(&s->work);
> >     }
>
> Although I think that a part of this is just whether you are familiar
> with Rust syntax, there is definitely some truth to this. Your code is a
> lot closer to the machine code of what actually happens here. Perhaps it
> would be interesting to see what you get if you just unsafely do exactly
> the same thing in Rust? It would look something like this:
>
>     struct MyStruct {
>         value: i32,
>         work: bindings::work_struct,
>     }
>
>     unsafe fn log_value(work: *mut bindings::work_struct) {
>         unsafe {
>             let s = container_of!(work, MyStruct, work);
>             pr_info!("The value is: {}", (*s).value);
>         }
>     }
>
>     unsafe fn print_later(s: *mut bindings::work_struct) {
>         unsafe {
>             bindings::INIT_WORK(&mut (*s).work, log_value);
>             bindings::schedule_work(&mut (*s).work);
>         }
>     }
>
> (I didn't try to compile this.)
>
> The problem with this approach is that it uses unsafe in driver code,
> but the goal behind Rust abstractions is to isolate all of the related
> unsafe code. The idea being that drivers using the workqueue do not need
> any unsafe code to use it. This means that, assuming these workqueue
> abstractions are correct, no driver can accidentally cause memory
> unsafety by using the workqueue wrong.
>
> The main difficult part of making this happen is the container_of
> operation. We need to somehow verify *at compile time* that the
> container_of in log_value really is given a pointer to the work field of
> a MyStruct. Other than the things that are just how Rust looks, most of
> the verbosity is necessary to make this compile-time check possible.
>
> Another thing it does is handle proper transfer of ownership. In my
> original example, MyStruct is reference counted (due to the use of Arc),
> so the workqueue passes ownership of one refcount to the workqueue,
> which eventually passes the refcount to run. When `this` goes out of
> scope at the end of `run`, the refcount is decremented and the struct is
> freed if the refcount dropped to zero.
>
> If you wanted to just have exclusive ownership of my_struct, you could
> do that by using Box instead of Arc. In either case, the ownership is
> correctly passed to run, and you cannot accidentally forget to free it
> at the end of log_value.
>
> So, ultimately there's a tradeoff here. The code corresponds less
> directly to what the machine code will be. On the other hand, it will be
> *more* difficult to use incorrectly since incorrect uses will usually
> result in compilation errors. The claim of Rust is that this tradeoff is
> worth it.

I get where all this coming from, I just really dislike the idea to
write all this code every time I need to pass something down the
workqueue. Maybe it's possible to hide most of the boilerplate behind a
derive.

Something like this, for example:

    #[pin_data, derive(WorkContainer)]
    struct MyStruct {
        value: i32,
        #[pin, work(fn = log_value)]
        work: Work,
    }

    fn log_value(s: Arc<MyStruct>) {
        pr_info!("The value is: {}", s.value);
    }

    fn print_later(s: Arc<MyStruct>) {
        workqueue::system().enqueue(s);
    }

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Boqun Feng]

On Wed, Oct 04, 2023 at 02:06:05PM +0300, Konstantin Shelekhin wrote:
> On Wed Oct 4, 2023 at 1:29 AM MSK, Alice Ryhl wrote:
> > On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@ftml.net> wrote:
> > > +//! #[pin_data]
> > > +//! struct MyStruct {
> > > +//!     value: i32,
> > > +//!     #[pin]
> > > +//!     work: Work<MyStruct>,
> > > +//! }
> > > +//!
> > > +//! impl_has_work! {
> > > +//!     impl HasWork<Self> for MyStruct { self.work }
> > > +//! }
> > > +//!
> > > +//! impl MyStruct {
> > > +//!     fn new(value: i32) -> Result<Arc<Self>> {
> > > +//!         Arc::pin_init(pin_init!(MyStruct {
> > > +//!             value,
> > > +//!             work <- new_work!("MyStruct::work"),
> > > +//!         }))
> > > +//!     }
> > > +//! }
> > > +//!
> > > +//! impl WorkItem for MyStruct {
> > > +//!     type Pointer = Arc<MyStruct>;
> > > +//!
> > > +//!     fn run(this: Arc<MyStruct>) {
> > > +//!         pr_info!("The value is: {}", this.value);
> > > +//!     }
> > > +//! }
> > > +//!
> > > +//! /// This method will enqueue the struct for execution on the system workqueue, where its value
> > > +//! /// will be printed.
> > > +//! fn print_later(val: Arc<MyStruct>) {
> > > +//!     let _ = workqueue::system().enqueue(val);
> > > +//! }
> > >
> > > I understand that this is highly opionated, but is it possible to make
> > > the initialization less verbose?
> >
> > The short answer is yes. There are safe alternatives that are much less
> > verbose. Unfortunately, those alternatives give up some of the features
> > that this design has. Specifically, they give up the feature that allows
> > you to embed the work_struct inside custom structs. I need to be able to
> > embed the work_struct inside of custom structs, so I did not go that
> > route.
> 
> My concern with the approach of using traits instead of calling an
> initialization function is that a some of existing code uses the
> following pattern:
> 
>     static void nvmet_file_submit_buffered_io(struct nvmet_req *req)
>     {
>             INIT_WORK(&req->f.work, nvmet_file_buffered_io_work);
>             queue_work(buffered_io_wq, &req->f.work);
>     }
> 
>     static void nvmet_file_execute_flush(struct nvmet_req *req)
>     {
>             if (!nvmet_check_transfer_len(req, 0))
>                     return;
>             INIT_WORK(&req->f.work, nvmet_file_flush_work);
>             queue_work(nvmet_wq, &req->f.work);
>     }
> 
>     static void nvmet_file_execute_dsm(struct nvmet_req *req)
>     {
>             if (!nvmet_check_data_len_lte(req, nvmet_dsm_len(req)))
>                     return;
>             INIT_WORK(&req->f.work, nvmet_file_dsm_work);
>             queue_work(nvmet_wq, &req->f.work);
>     }
> 
> As you can see a single work struct is used here, and dispatching
> happens beforehands. While it's possible to do the dispatching later in
> run(), it's IMO cleaner to do this earlier.

This is not a problem until nvmet actually uses/switches to Rust, right?
;-) We can certainly improve the API when a real user needs something.
Or you know someone is already working on this?

[...]
> 
> I get where all this coming from, I just really dislike the idea to
> write all this code every time I need to pass something down the
> workqueue. Maybe it's possible to hide most of the boilerplate behind a
> derive.
> 
> Something like this, for example:
> 
>     #[pin_data, derive(WorkContainer)]
>     struct MyStruct {
>         value: i32,
>         #[pin, work(fn = log_value)]
>         work: Work,
>     }
> 
>     fn log_value(s: Arc<MyStruct>) {
>         pr_info!("The value is: {}", s.value);
>     }
> 
>     fn print_later(s: Arc<MyStruct>) {
>         workqueue::system().enqueue(s);
>     }

All of your suggestions make senses to me, but because we don't have
many users right now, it's actually hard to determine a "best" API. I
like what we have right now because it's explicit: people won't need to
learn much about procedure macros to understand how it works, and it
also provides better opportunities for people who's yet not familiar
with Rust to give some reviews. So starting with something relatively
simple and verbose may not be a bad idea ;-)

Again, I like your idea, we need to explore that direction, but one
dragon at a time ;-)

Regards,
Boqun

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Konstantin Shelekhin]

> This is not a problem until nvmet actually uses/switches to Rust, right?
> ;-) We can certainly improve the API when a real user needs something.
> Or you know someone is already working on this?

Nope, not at this moment. I have an itch to experiment with Rust and
iSCSI, but that's my personal toy without any plans to at least propose
it to the subsystem maintainers yet.

> All of your suggestions make senses to me, but because we don't have
> many users right now, it's actually hard to determine a "best" API. I
> like what we have right now because it's explicit: people won't need to
> learn much about procedure macros to understand how it works, and it
> also provides better opportunities for people who's yet not familiar
> with Rust to give some reviews. So starting with something relatively
> simple and verbose may not be a bad idea ;-)
>
> Again, I like your idea, we need to explore that direction, but one
> dragon at a time ;-)

Oh yeah, completely understand :)

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Andreas Hindborg (Samsung)]

Hi,

"Konstantin Shelekhin" <k.shelekhin@ftml.net> writes:

>> This is not a problem until nvmet actually uses/switches to Rust, right?
>> ;-) We can certainly improve the API when a real user needs something.
>> Or you know someone is already working on this?
>
> Nope, not at this moment. I have an itch to experiment with Rust and
> iSCSI, but that's my personal toy without any plans to at least propose
> it to the subsystem maintainers yet.

If you are so inclined, I would suggest you take a look at the blk-mq
bindings and the nvme (pci) [1] and null_blk [2] Rust drivers. I am
available if you have any questions!

BR Andreas

[1] https://github.com/metaspace/linux/tree/nvme-next-for-6.6
[2] https://github.com/metaspace/linux/tree/null_blk-next-for-6.6

Re: [PATCH v4 7/7] rust: workqueue: add examples

[Trevor Gross]

On Tue, Oct 3, 2023 at 6:30 PM Alice Ryhl <aliceryhl@google.com> wrote:
> On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@ftml.net> wrote:
> > +//! #[pin_data]
> > +//! struct MyStruct {
> > +//!     value: i32,
> > +//!     #[pin]
> > +//!     work: Work<MyStruct>,
> > +//! }
> > +//!
> > +//! impl_has_work! {
> > +//!     impl HasWork<Self> for MyStruct { self.work }
> > +//! }
> > +//!
> > +//! impl MyStruct {
> > +//!     fn new(value: i32) -> Result<Arc<Self>> {
> > +//!         Arc::pin_init(pin_init!(MyStruct {
> > +//!             value,
> > +//!             work <- new_work!("MyStruct::work"),
> > +//!         }))
> > +//!     }
> > +//! }
> > +//!

For what it's worth, I think that using a binding for return items
usually looks ever so slightly more clear than passing a multiline
argument

    fn new(value: i32) -> Result<Arc<Self>> {
        let tmp = pin_init!(MyStruct {
            value,
            work <- new_work!("MyStruct::work")
        });
        Arc::pin_init(tmp)
    }