[PATCH v1 0/7] Bindings for the workqueue

[PATCH v1 0/7] 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.

This patch could be significantly simplified if we already had the field
projection bindings. However, we have decided to upstream the current
version that does not depend on field projection first - the PR that
introduces field projections will then include a commit that simplifies
the workqueue implementation. (In particular, it would simplify the 5th
patch in this series.)

The first 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`.

Alice Ryhl (4):
  rust: workqueue: add low-level workqueue bindings
  rust: workqueue: add helper for defining work_struct fields
  rust: workqueue: add safe API to workqueue
  rust: workqueue: add `try_spawn` helper method

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

 rust/helpers.c           |   8 +
 rust/kernel/lib.rs       |  37 ++
 rust/kernel/sync/arc.rs  |  44 +++
 rust/kernel/workqueue.rs | 715 +++++++++++++++++++++++++++++++++++++++
 scripts/Makefile.build   |   2 +-
 5 files changed, 805 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/workqueue.rs


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.40.1.606.ga4b1b128d6-goog

[PATCH v1 1/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.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/lib.rs       |  1 +
 rust/kernel/workqueue.rs | 99 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 rust/kernel/workqueue.rs

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 676995d4e460..c718524056a6 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -47,6 +47,7 @@ pub mod str;
 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..e66b6b50dfae
--- /dev/null
+++ b/rust/kernel/workqueue.rs
@@ -0,0 +1,99 @@
+// 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: Kernel workqueues are usable from any thread.
+unsafe impl Send for Queue {}
+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.
+    pub fn enqueue<T: WorkItem + Send + 'static>(&self, w: T) -> T::EnqueueOutput {
+        let queue_ptr = self.0.get();
+
+        // SAFETY: There are two cases.
+        //
+        //  1. If `queue_work_on` returns false, then we failed to push the work item to the queue.
+        //     In this case, we don't touch the work item again.
+        //
+        //  2. If `queue_work_on` returns true, then we pushed the work item to the queue. The work
+        //     queue will call the function pointer in the `work_struct` at some point in the
+        //     future. We require `T` to be static, so the type has no lifetimes annotated on it.
+        //     We require `T` to be send, so there are no thread-safety issues to take care of.
+        //
+        // In either case we follow the safety requirements of `__enqueue`.
+        unsafe {
+            w.__enqueue(move |work_ptr| {
+                bindings::queue_work_on(bindings::WORK_CPU_UNBOUND as _, queue_ptr, work_ptr)
+            })
+        }
+    }
+}
+
+/// A work item.
+///
+/// This is the low-level trait that is designed for being as general as possible.
+///
+/// # Safety
+///
+/// Implementers must ensure that `__enqueue` behaves as documented.
+pub unsafe trait WorkItem {
+    /// The return type of [`Queue::enqueue`].
+    type EnqueueOutput;
+
+    /// Enqueues this work item on a queue using the provided `queue_work_on` method.
+    ///
+    /// # Safety
+    ///
+    /// Calling this method guarantees that the provided closure will be called with a raw pointer
+    /// to a `struct work_struct`. The closure should behave in the following way:
+    ///
+    ///  1. If the `struct work_struct` cannot be pushed to a workqueue because its already in one,
+    ///     then the closure should return `false`. It may not access the pointer after returning
+    ///     `false`.
+    ///  2. If the `struct work_struct` is successfully added to a workqueue, then the closure
+    ///     should return `true`. When the workqueue executes the work item, it will do so by
+    ///     calling the function pointer stored in the `struct work_struct`. The work item ensures
+    ///     that the raw pointer remains valid until that happens.
+    ///
+    /// This method may not have any other failure cases than the closure returning `false`. The
+    /// output type should reflect this, but it may also be an infallible type if the work item
+    /// statically ensures that pushing the `struct work_struct` will succeed.
+    ///
+    /// If the work item type is annotated with any lifetimes, then the workqueue must call the
+    /// function pointer before any such lifetime expires. (Or it may forget the work item and
+    /// never call the function pointer at all.)
+    ///
+    /// If the work item type is not [`Send`], then the work item must be executed 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.40.1.606.ga4b1b128d6-goog

[PATCH v1 2/7] rust: add offset_of! macro

[Alice Ryhl]

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

This macro is used to compute the offset of a field in a struct.

This commit enables two unstable features that are necessary for using
the macro in a constant. However, this is not a problem as the macro
will become available from the Rust standard library soon [1]. The
unstable features can be disabled again once that happens.

The macro in this patch does not support sub-fields. That is, you cannot
write `offset_of!(MyStruct, field.sub_field)` to get the offset of
`sub_field` with `field`'s type being a struct with a field called
`sub_field`. This is because `field` might be a `Box<SubStruct>`, which
means that you would be trying to compute the offset to something in an
entirely different allocation. There's no easy way to fix the current
macro to support subfields, but the version being added to the standard
library should support it, so the limitation is temporary and not a big
deal.

Link: https://github.com/rust-lang/rust/issues/106655 [1]
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>
---
 rust/kernel/lib.rs     | 35 +++++++++++++++++++++++++++++++++++
 scripts/Makefile.build |  2 +-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c718524056a6..cdf9fe999328 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -14,6 +14,8 @@
 #![no_std]
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
+#![feature(const_ptr_offset_from)]
+#![feature(const_refs_to_cell)]
 #![feature(core_ffi_c)]
 #![feature(dispatch_from_dyn)]
 #![feature(explicit_generic_args_with_impl_trait)]
@@ -102,3 +104,36 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
     // instead of `!`. See <https://github.com/rust-lang/rust-bindgen/issues/2094>.
     loop {}
 }
+
+/// Calculates the offset of a field from the beginning of the struct it belongs to.
+///
+/// # Examples
+///
+/// ```
+/// #[repr(C)]
+/// struct Test {
+///     a: u64,
+///     b: u32,
+/// }
+///
+/// assert_eq!(kernel::offset_of!(Test, b), 8);
+/// ```
+#[macro_export]
+macro_rules! offset_of {
+    ($type:ty, $field:ident) => {{
+        let tmp = ::core::mem::MaybeUninit::<$type>::uninit();
+        let outer = tmp.as_ptr();
+        // To avoid warnings when nesting `unsafe` blocks.
+        #[allow(unused_unsafe)]
+        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
+        // we don't actually read from `outer` (which would be UB) nor create an intermediate
+        // reference.
+        let inner = unsafe { ::core::ptr::addr_of!((*outer).$field) } as *const u8;
+        // To avoid warnings when nesting `unsafe` blocks.
+        #[allow(unused_unsafe)]
+        // SAFETY: The two pointers are within the same allocation block.
+        unsafe {
+            inner.offset_from(outer as *const u8) as usize
+        }
+    }};
+}
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9f94fc83f086..ec583d13dde2 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
+rust_allowed_features := const_ptr_offset_from,const_refs_to_cell,core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
 
 rust_common_cmd = \
 	RUST_MODFILE=$(modfile) $(RUSTC_OR_CLIPPY) $(rust_flags) \
-- 
2.40.1.606.ga4b1b128d6-goog

[PATCH v1 3/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>
---
 rust/kernel/lib.rs      |  1 +
 rust/kernel/sync/arc.rs | 44 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index cdf9fe999328..82854c86e65d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -22,6 +22,7 @@
 #![feature(generic_associated_types)]
 #![feature(new_uninit)]
 #![feature(pin_macro)]
+#![feature(ptr_metadata)]
 #![feature(receiver_trait)]
 #![feature(unsize)]
 
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index e6d206242465..7c55a9178dfb 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -210,6 +210,50 @@ impl<T: ?Sized> Arc<T> {
         }
     }
 
+    /// 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`].
+    ///
+    /// This code relies on the `repr(C)` layout of structs as described in
+    /// <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
+    /// can only be called once for each previous call to [`Arc::into_raw`].
+    pub unsafe fn from_raw(ptr: *const T) -> Self {
+        // SAFETY: The safety requirement ensures that the pointer is valid.
+        let val_align = core::mem::align_of_val(unsafe { &*ptr });
+        let refcount_size = core::mem::size_of::<Opaque<bindings::refcount_t>>();
+
+        // Use the `repr(C)` algorithm to compute the offset of `data` in `ArcInner`.
+        //
+        // Pseudo-code for the `#[repr(C)]` algorithm can be found here:
+        // <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>
+        let mut val_offset = refcount_size;
+        let val_misalign = val_offset % val_align;
+        if val_misalign > 0 {
+            val_offset += val_align - val_misalign;
+        }
+
+        // This preserves the metadata in the pointer, if any.
+        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);
+        let ptr = (ptr as *mut u8).wrapping_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.40.1.606.ga4b1b128d6-goog

[PATCH v1 4/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>
---
 rust/kernel/workqueue.rs | 65 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index e66b6b50dfae..22205d3bda72 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -97,3 +97,68 @@ pub unsafe trait WorkItem {
     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.
+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.
+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.40.1.606.ga4b1b128d6-goog

[PATCH v1 5/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. This
commit will use traits so that they know about each other according to
the following cycle:

 * The pointer type. It knows the type of the work item struct.
 * The work item struct. It knows the offset of its `work_struct` field.
 * The `work_struct` field. It knows the pointer type.

There's nothing special about making the pointer type know the type of
the struct it points at. Pointers generally always know that
information.

However, making the `work_struct` field know about the pointer type is
less commonly seen. This is done by using a generic parameter: the
`work_struct` field will have the type `Work<T>`, where T will be the
pointer type in use. The pointer type is required to implement the
`WorkItemAdapter` trait, which defines the function pointer to store in
the `work_struct` field. The `Work<T>` type guarantees that the
`work_struct` inside it uses `<T as WorkItemAdapter>::run` as its
function pointer.

Finally, to make the work item struct know the offset of its
`work_struct` field, we use a trait called `HasWork<T>`. If a type
implements this trait, then the type declares that, at the given offset,
there is a field of type `Work<T>`. 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<Arc<MyWorkItem>>,
}

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

So to summarize, given a pointer to an allocation containing a work
item, you can use the `HasWork<T>` trait to offset the pointer to the
`work_struct` field. The function pointer in the `work_struct` field is
guaranteed to be a function that knows what the original pointer type
was, and using that information, it can undo the offset operation by
looking up what the offset was via the `HasWork<T>` trait.

This design supports work items with multiple `work_struct` fields by
using different pointer types. For example, you might define structs
like these:

```
struct MyPointer1(Arc<MyWorkItem>);
struct MyPointer2(Arc<MyWorkItem>);

struct MyWorkItem {
    work1: Work<MyPointer1>,
    work2: Work<MyPointer2>,
}
```

Then, the wrapper structs `MyPointer1` and `MyPointer2` will take the
role as the pointer type. By using one or the other, you tell the
workqueue which `work_struct` field to use. This pattern is called the
"newtype" pattern.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/helpers.c           |   8 ++
 rust/kernel/workqueue.rs | 183 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 190 insertions(+), 1 deletion(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..7f0c2fe2fbeb 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/sched/signal.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -128,6 +129,13 @@ void rust_helper_put_task_struct(struct task_struct *t)
 }
 EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
 
+void rust_helper___INIT_WORK(struct work_struct *work, work_func_t func,
+			     bool on_stack)
+{
+	__INIT_WORK(work, func, on_stack);
+}
+EXPORT_SYMBOL_GPL(rust_helper___INIT_WORK);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 22205d3bda72..7509618af252 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -4,7 +4,8 @@
 //!
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
-use crate::{bindings, types::Opaque};
+use crate::{bindings, prelude::*, types::Opaque};
+use core::marker::{PhantomData, PhantomPinned};
 
 /// A kernel work queue.
 ///
@@ -98,6 +99,186 @@ pub unsafe trait WorkItem {
         F: FnOnce(*mut bindings::work_struct) -> bool;
 }
 
+/// Defines the method that should be called when a work item is executed.
+///
+/// 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`]: WorkItem::__enqueue
+/// [`run`]: WorkItemAdapter::run
+pub unsafe trait WorkItemAdapter: WorkItem {
+    /// Run this work item.
+    ///
+    /// # Safety
+    ///
+    /// Must only be called via the function pointer that [`__enqueue`] provides to the
+    /// `queue_work_on` closure, and only as described in the documentation of `queue_work_on`.
+    ///
+    /// [`__enqueue`]: WorkItem::__enqueue
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
+}
+
+/// Links for a work item.
+///
+/// This struct contains a function pointer to the `T::run` function from the [`WorkItemAdapter`]
+/// 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 [`WorkItemAdapter`] that uses
+/// it.
+#[repr(transparent)]
+pub struct Work<T: ?Sized> {
+    work: Opaque<bindings::work_struct>,
+    _pin: PhantomPinned,
+    _adapter: 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> Send for Work<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> Sync for Work<T> {}
+
+impl<T: ?Sized> Work<T> {
+    /// Creates a new instance of [`Work`].
+    #[inline]
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new() -> impl PinInit<Self>
+    where
+        T: WorkItemAdapter,
+    {
+        // SAFETY: The `WorkItemAdapter` implementation promises that `T::run` can be used as the
+        // work item function.
+        unsafe {
+            kernel::init::pin_init_from_closure(move |slot| {
+                bindings::__INIT_WORK(Self::raw_get(slot), Some(T::run), false);
+                Ok(())
+            })
+        }
+    }
+
+    /// Get a pointer to the inner `work_struct`.
+    ///
+    /// # Safety
+    ///
+    /// The provided pointer must not be dangling. (But it 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 valid.
+        //
+        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
+        // the compiler does not complain that `work` is unused.
+        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
+    }
+}
+
+/// Declares that a type has a [`Work<T>`] field.
+///
+/// # Safety
+///
+/// The [`OFFSET`] constant must be the offset of a field in Self of type [`Work<T>`]. The methods on
+/// this trait must have exactly the behavior that the definitions given below have.
+///
+/// [`Work<T>`]: Work
+/// [`OFFSET`]: HasWork::OFFSET
+pub unsafe trait HasWork<T> {
+    /// The offset of the [`Work<T>`] field.
+    ///
+    /// [`Work<T>`]: Work
+    const OFFSET: usize;
+
+    /// Returns the offset of the [`Work<T>`] field.
+    ///
+    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not Sized.
+    ///
+    /// [`Work<T>`]: Work
+    /// [`OFFSET`]: HasWork::OFFSET
+    #[inline]
+    fn get_work_offset(&self) -> usize {
+        Self::OFFSET
+    }
+
+    /// Returns a pointer to the [`Work<T>`] field.
+    ///
+    /// # Safety
+    ///
+    /// The pointer must not be dangling. (But the memory need not be initialized.)
+    ///
+    /// [`Work<T>`]: Work
+    #[inline]
+    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T>
+    where
+        Self: Sized,
+    {
+        // SAFETY: The caller promises that the pointer is not dangling.
+        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T> }
+    }
+
+    /// Returns a pointer to the struct containing the [`Work<T>`] field.
+    ///
+    /// # Safety
+    ///
+    /// The pointer must not be dangling. (But the memory need not be initialized.)
+    ///
+    /// [`Work<T>`]: Work
+    #[inline]
+    unsafe fn work_container_of(ptr: *mut Work<T>) -> *mut Self
+    where
+        Self: Sized,
+    {
+        // SAFETY: The caller promises that the pointer is not dangling.
+        unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
+    }
+}
+
+/// Used to safely implement the [`HasWork<T>`] trait.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::sync::Arc;
+///
+/// struct MyStruct {
+///     work_field: Work<Arc<MyStruct>>,
+/// }
+///
+/// impl_has_work! {
+///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
+/// }
+/// ```
+///
+/// [`HasWork<T>`]: HasWork
+#[macro_export]
+macro_rules! impl_has_work {
+    ($(impl$(<$($implarg:ident),*>)?
+       HasWork<$work_type:ty>
+       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> for $self $(<$($selfarg),*>)? {
+            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
+
+            #[inline]
+            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
+                // 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.40.1.606.ga4b1b128d6-goog

[PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Alice Ryhl]

This commit introduces `ArcWorkItem`, `BoxWorkItem`, and
`define_work_adapter_newtype!` that make it possible to use the
workqueue without any unsafe code whatsoever.

The `ArcWorkItem` and `BoxWorkItem` traits are used when a struct has a
single `work_struct` field.

The `define_work_adapter_newtype!` macro is used when a struct has
multiple `work_struct` fields. For each `work_struct` field, a newtype
struct is defined that wraps `Arc<TheStruct>`, and pushing an instance
of the newtype to a workqueue will enqueue it using the associated
`work_struct` field. The newtypes are matched with `work_struct` fields
by having the T in `Work<T>` be the newtype.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/workqueue.rs | 332 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 331 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 7509618af252..007005ddcaf0 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -4,8 +4,9 @@
 //!
 //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
 
-use crate::{bindings, prelude::*, types::Opaque};
+use crate::{bindings, prelude::*, sync::Arc, types::Opaque};
 use core::marker::{PhantomData, PhantomPinned};
+use core::result::Result;
 
 /// A kernel work queue.
 ///
@@ -279,6 +280,335 @@ macro_rules! impl_has_work {
     )*};
 }
 
+/// Declares that [`Arc<Self>`] should implement [`WorkItem`].
+///
+/// # Examples
+///
+/// The example below will make [`Arc<MyStruct>`] implement the [`WorkItem`] trait so that you can
+/// enqueue it in a workqueue.
+///
+/// ```
+/// use kernel::sync::Arc;
+///
+/// struct MyStruct {
+///     work_field: Work<Arc<MyStruct>>,
+/// }
+///
+/// kernel::impl_has_work! {
+///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
+/// }
+///
+/// impl ArcWorkItem for MyStruct {
+///     fn run(self: Arc<Self>) {
+///         pr_info!("Executing MyStruct on a workqueue.");
+///     }
+/// }
+/// ```
+///
+/// [`Arc<Self>`]: crate::sync::Arc
+/// [`Arc<MyStruct>`]: crate::sync::Arc
+pub trait ArcWorkItem {
+    /// Called when this work item is executed.
+    fn run(self: Arc<Self>);
+}
+
+unsafe impl<T> WorkItem for Arc<T>
+where
+    T: ArcWorkItem + HasWork<Self> + ?Sized,
+{
+    type EnqueueOutput = Result<(), Self>;
+
+    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
+    where
+        F: FnOnce(*mut bindings::work_struct) -> bool,
+    {
+        let ptr = Arc::into_raw(self);
+
+        // Using `get_work_offset` here for object-safety.
+        //
+        // SAFETY: The pointer is valid since we just got it from `into_raw`.
+        let off = unsafe { (&*ptr).get_work_offset() };
+
+        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
+        // `Work<Self>` in the same allocation.
+        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
+        // SAFETY: The pointer is not dangling.
+        let work_ptr = unsafe { Work::raw_get(work_ptr) };
+
+        match (queue_work_on)(work_ptr) {
+            true => Ok(()),
+            // SAFETY: The work queue has not taken ownership of the pointer.
+            false => Err(unsafe { Arc::from_raw(ptr) }),
+        }
+    }
+}
+
+// Let `Work<Arc<T>>` be usable with types that are `ArcWorkItem`.
+//
+// We do not allow unsized types here. The `Work<Arc<T>>` field should always specify the actual
+// concrete type stored in the `Arc`.
+//
+// SAFETY: The `Work<Arc<T>>` field must be initialized with this `run` method because the `Work`
+// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
+// same `Work<Arc<T>>` field because `HasWork` promises to always return the same field.
+unsafe impl<T> WorkItemAdapter for Arc<T>
+where
+    T: ArcWorkItem + HasWork<Self> + Sized,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
+        let ptr = ptr as *mut Work<Self>;
+        // 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) };
+
+        arc.run();
+    }
+}
+
+/// Declares that [`Pin`]`<`[`Box`]`<Self>>` should implement [`WorkItem`].
+///
+/// # Examples
+///
+/// The example below will make [`Pin`]`<`[`Box`]`<MyStruct>>` implement the [`WorkItem`] trait so
+/// that you can enqueue it in a workqueue.
+///
+/// ```
+/// struct MyStruct {
+///     work_field: Work<Pin<Box<MyStruct>>>,
+/// }
+///
+/// kernel::impl_has_work! {
+///     impl HasWork<Pin<Box<MyStruct>>> for MyStruct { self.work_field }
+/// }
+///
+/// impl BoxWorkItem for MyStruct {
+///     fn run(self: Pin<Box<MyStruct>>) {
+///         pr_info!("Executing MyStruct on a workqueue.");
+///     }
+/// }
+/// ```
+///
+/// [`Box`]: alloc::boxed::Box
+/// [`Pin`]: core::pin::Pin
+pub trait BoxWorkItem {
+    /// Called when this work item is executed.
+    fn run(self: Pin<Box<Self>>);
+}
+
+unsafe impl<T> WorkItem for Pin<Box<T>>
+where
+    T: BoxWorkItem + HasWork<Self> + ?Sized,
+{
+    // When a box is in a workqueue, the workqueue has exclusive ownership of the box. Therefore,
+    // it's not possible to enqueue a box while it is in a workqueue.
+    type EnqueueOutput = ();
+
+    unsafe fn __enqueue<F>(self, queue_work_on: F)
+    where
+        F: FnOnce(*mut bindings::work_struct) -> bool,
+    {
+        // SAFETY: We will not used the contents in an unpinned manner.
+        let ptr = unsafe { Box::into_raw(Pin::into_inner_unchecked(self)) };
+
+        // Using `get_work_offset` here for object-safety.
+        //
+        // SAFETY: The pointer is valid since we just got it from `into_raw`.
+        let off = unsafe { (&*ptr).get_work_offset() };
+
+        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
+        // `Work<Self>` in the same allocation.
+        let work_ptr = unsafe { (ptr as *mut u8).add(off) as *mut Work<Self> };
+        // SAFETY: The pointer is not dangling.
+        let work_ptr = unsafe { Work::raw_get(work_ptr) };
+
+        match (queue_work_on)(work_ptr) {
+            true => {}
+            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
+            // workqueue.
+            false => unsafe { core::hint::unreachable_unchecked() },
+        }
+    }
+}
+
+// Let `Work<Pin<Box<T>>>` be usable with types that are `BoxWorkItem`.
+//
+// We do not allow unsized types here. The `Work<Pin<Box<T>>>` field should always specify the actual
+// concrete type stored in the `Box`.
+//
+// SAFETY: The `Work<Pin<Box<T>>>` field must be initialized with this run method because the `Work`
+// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
+// same `Work<Pin<Box<T>>>` field because `HasWork` promises to always return the same field.
+unsafe impl<T> WorkItemAdapter for Pin<Box<T>>
+where
+    T: BoxWorkItem + HasWork<Self> + Sized,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
+        let ptr = ptr as *mut Work<Self>;
+        // 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 `Box::into_raw` and we've been given back ownership.
+        // The box was originally pinned, so pinning it again is ok.
+        let boxed = unsafe { Pin::new_unchecked(Box::from_raw(ptr)) };
+
+        boxed.run();
+    }
+}
+
+/// Helper macro for structs with several `Work` fields that can be in several queues at once.
+///
+/// For each `Work` field in your type `T`, a newtype struct that wraps an `Arc<T>` or
+/// `Pin<Box<T>>` should be defined.
+///
+/// # Examples
+///
+/// ```
+/// struct MyStruct {
+///     work1: Work<MyStructWork1>,
+///     work2: Work<MyStructWork2>,
+/// }
+///
+/// impl_has_work! {
+///     impl HasWork<MyStructWork1> for MyStruct { self.work1 }
+///     impl HasWork<MyStructWork2> for MyStruct { self.work2 }
+/// }
+///
+/// define_work_adapter_newtype! {
+///     struct MyStructWork1(Arc<MyStruct>);
+///     struct MyStructWork2(Arc<MyStruct>);
+/// }
+///
+/// impl MyStructWork1 {
+///     fn run(self) {
+///         // ...
+///     }
+/// }
+///
+/// impl MyStructWork2 {
+///     fn run(self) {
+///         // ...
+///     }
+/// }
+/// ```
+///
+/// This will let you push a `MyStructWork1(arc)` or `MyStructWork2(arc)` to a work queue. The [`Arc`]
+/// can be in two work queues at the same time, and the `run` method on the wrapper type is called
+/// when the work item is called.
+///
+/// [`Arc`]: crate::sync::Arc
+#[macro_export]
+macro_rules! define_work_adapter_newtype {
+    (
+        $(#[$outer:meta])*
+        $pub:vis struct $name:ident(
+            $(#[$innermeta:meta])*
+            $fpub:vis Arc<$inner:ty> $(,)?
+        );
+        $($rest:tt)*
+    ) => {
+        $(#[$outer])*
+        $pub struct $name($(#[$innermeta])* $fpub $crate::sync::Arc<$inner>);
+
+        unsafe impl $crate::workqueue::WorkItem for $name {
+            type EnqueueOutput = ::core::result::Result<(), $name>;
+
+            unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
+            where
+                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
+            {
+                let ptr = $crate::sync::Arc::into_raw(self.0);
+
+                // SAFETY: The pointer is not dangling since we just got it from Arc::into_raw.
+                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr.cast_mut()) };
+
+                // SAFETY: The pointer is not dangling.
+                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
+
+                match (queue_work_on)(work_ptr) {
+                    true => Ok(()),
+                    // SAFETY: The work queue has not taken ownership of the pointer.
+                    false => Err($name(unsafe { $crate::sync::Arc::from_raw(ptr) })),
+                }
+            }
+        }
+
+        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
+            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
+                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
+                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
+                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
+                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
+                // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
+                let arc = unsafe { $crate::sync::Arc::from_raw(ptr) };
+
+                $name::run($name(arc));
+            }
+        }
+
+        define_work_adapter_newtype! { $($rest)* }
+    };
+
+    (
+        $(#[$outer:meta])*
+        $pub:vis struct $name:ident(
+            $(#[$innermeta:meta])*
+            $fpub:vis Pin<Box<$inner:ty>> $(,)?
+        );
+        $($rest:tt)*
+    ) => {
+        $(#[$outer])*
+        $pub struct $name($(#[$innermeta])* $fpub ::core::pin::Pin<::alloc::boxed::Box<$inner>>);
+
+        unsafe impl $crate::workqueue::WorkItem for $name {
+            type EnqueueOutput = ();
+
+            unsafe fn __enqueue<F>(self, queue_work_on: F)
+            where
+                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
+            {
+                // SAFETY: We will not used the contents in an unpinned manner.
+                let boxed = unsafe { ::core::pin::Pin::into_inner_unchecked(self.0) };
+                let ptr = ::alloc::boxed::Box::into_raw(boxed);
+
+                // SAFETY: The pointer is not dangling since we just got it from Box::into_raw.
+                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr) };
+
+                // SAFETY: The pointer is not dangling.
+                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
+
+                match (queue_work_on)(work_ptr) {
+                    true => {},
+                    // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
+                    // workqueue.
+                    false => unsafe { ::core::hint::unreachable_unchecked() },
+                }
+            }
+        }
+
+        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
+            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
+                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
+                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
+                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
+                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
+                // SAFETY: This pointer comes from `Box::into_raw` and we've been given back ownership.
+                let boxed = unsafe { ::alloc::boxed::Box::from_raw(ptr) };
+                // SAFETY: The box was originally pinned, so pinning it again is ok.
+                let boxed = unsafe { ::core::pin::Pin::new_unchecked(boxed) };
+
+                $name::run($name(boxed));
+            }
+        }
+
+        define_work_adapter_newtype! { $($rest)* }
+    };
+
+    // After processing the last definition, we call ourselves with no input.
+    () => {};
+}
+
 /// Returns the system work queue (`system_wq`).
 ///
 /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
-- 
2.40.1.606.ga4b1b128d6-goog

[PATCH v1 7/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.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/workqueue.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 007005ddcaf0..303b72efd95f 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -57,6 +57,42 @@ impl Queue {
             })
         }
     }
+
+    /// 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 + Fn()>(&self, func: T) -> Result {
+        let init = pin_init!(ClosureWork {
+            work <- Work::new(),
+            func: Some(func),
+        });
+
+        self.enqueue(Box::pin_init(init)?);
+        Ok(())
+    }
+}
+
+/// A helper type used in `try_spawn`.
+#[pin_data]
+struct ClosureWork<T> {
+    #[pin]
+    work: Work<Pin<Box<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()> BoxWorkItem for ClosureWork<T> {
+    fn run(mut self: Pin<Box<Self>>) {
+        if let Some(func) = self.as_mut().project().take() {
+            (func)()
+        }
+    }
 }
 
 /// A work item.
@@ -280,6 +316,10 @@ macro_rules! impl_has_work {
     )*};
 }
 
+impl_has_work! {
+    impl<T> HasWork<Pin<Box<Self>>> for ClosureWork<T> { self.work }
+}
+
 /// Declares that [`Arc<Self>`] should implement [`WorkItem`].
 ///
 /// # Examples
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH v1 0/7] Bindings for the workqueue

[Tejun Heo]

Hello, Alice.

On Wed, May 17, 2023 at 08:31:12PM +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.
> 
> This patch could be significantly simplified if we already had the field
> projection bindings. However, we have decided to upstream the current
> version that does not depend on field projection first - the PR that
> introduces field projections will then include a commit that simplifies
> the workqueue implementation. (In particular, it would simplify the 5th
> patch in this series.)
> 
> The first 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`.

I tried to read the patches but am too dumb to understand much. Any chance
you can provide some examples so that I can at least imagine how workqueue
would be used from rust side?

Thanks.

-- 
tejun

Re: [PATCH v1 0/7] Bindings for the workqueue

[Alice Ryhl]

On Wed, 17 May 2023 11:48:19 -1000, Tejun Heo wrote:
> I tried to read the patches but am too dumb to understand much.

The patch is more complicated than I would have liked, unfortunately.
However, as I mentioned in the cover letter, simplifications should be
on their way.

Luckily, using the workqueue bindings is simpler than the bindings
themselves.

> Any chance you can provide some examples so that I can at least
> imagine how workqueue would be used from rust side?

Yes, of course!

The simplest way to use the workqueue is to use the `try_spawn` method
introduced by the last patch in the series. With this function, you just
pass a function pointer to the `try_spawn` method, and it schedules the
function for execution. Unfortunately this allocates memory, making it
a fallible operation.

To avoid allocation memory, we do something else. As an example, we can
look at the Rust binder driver that I am currently working on. Here is
how it will be used in the binder driver: First, the `Process` struct
will be given a `work_struct` field:

#[pin_data]
pub(crate) struct Process {
    // Work node for deferred work item.
    #[pin]
    defer_work: Work<Arc<Process>>,

    // Other fields follow...
}

Here, we use the type `Work<Arc<Process>>` for our field. This type is
the Rust wrapper for `work_struct`. The generic parameter to `Work`
should be the pointer type used to access `Process`, and in this case it
is `Arc<Process>`. The pointer type `Arc` is used for reference
counting, and its a pointer type that owns a ref-count to the inner
value. (So e.g., it decrements the ref-cout when the arc goes out of
scope.) Arc is an abbreviation of "atomic reference count". This means
that while it is enqueued in the workqueue, the workqueue owns a
ref-count to the process.

Next, binder will use the `impl_has_work!` macro to declare that it
wants to use `defer_work` as its `work_struct` field. That looks like
this:

kernel::impl_has_work! {
    impl HasWork<Arc<Process>> for Process { self.defer_work }
}

To define the code that should run when the work item is executed on the
workqueue, binder does the following:

impl workqueue::ArcWorkItem for Process {
    fn run(self: Arc<Process>) {
        // this runs when the work item is executed
    }
}

Finally to schedule it to the system workqueue, it does the following:

let _ = workqueue::system().enqueue(process);

Here, the `enqueue` call is fallible, since it might fail if the process
has already been enqueued to a work queue. However, binder just uses
`let _ =` to ignore the failure, since it doesn't need to do anything
special in that case.

I hope that helps, and let me know if you have any further questions.

Alice

Re: [PATCH v1 1/7] rust: workqueue: add low-level workqueue bindings

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> Define basic low-level bindings to a kernel workqueue. The API defined
> here can only be used unsafely. Later commits will provide safe
> wrappers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]
> +
> +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.

Wouldn't be worth to mention that, if not implied, the item it's going
to be worked on an unbound CPU?

> +    pub fn enqueue<T: WorkItem + Send + 'static>(&self, w: T) -> T::EnqueueOutput {
> +        let queue_ptr = self.0.get();
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v1 2/7] rust: add offset_of! macro

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This macro is used to compute the offset of a field in a struct.
> 
> This commit enables two unstable features that are necessary for using
> the macro in a constant. However, this is not a problem as the macro
> will become available from the Rust standard library soon [1]. The
> unstable features can be disabled again once that happens.
> 
> The macro in this patch does not support sub-fields. That is, you cannot
> write `offset_of!(MyStruct, field.sub_field)` to get the offset of
> `sub_field` with `field`'s type being a struct with a field called
> `sub_field`. This is because `field` might be a `Box<SubStruct>`, which
> means that you would be trying to compute the offset to something in an
> entirely different allocation. There's no easy way to fix the current
> macro to support subfields, but the version being added to the standard
> library should support it, so the limitation is temporary and not a big
> deal.
> 
> Link: https://github.com/rust-lang/rust/issues/106655 [1]
> 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>

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> 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: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v1 4/7] rust: workqueue: define built-in queues

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> 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>

Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work_struct fields

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, 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. This
> commit will use traits so that they know about each other according to
> the following cycle:
> 
>   * The pointer type. It knows the type of the work item struct.
>   * The work item struct. It knows the offset of its `work_struct` field.
>   * The `work_struct` field. It knows the pointer type.
> 
> There's nothing special about making the pointer type know the type of
> the struct it points at. Pointers generally always know that
> information.
> 
> However, making the `work_struct` field know about the pointer type is
> less commonly seen. This is done by using a generic parameter: the
> `work_struct` field will have the type `Work<T>`, where T will be the
> pointer type in use. The pointer type is required to implement the
> `WorkItemAdapter` trait, which defines the function pointer to store in
> the `work_struct` field. The `Work<T>` type guarantees that the
> `work_struct` inside it uses `<T as WorkItemAdapter>::run` as its
> function pointer.
> 
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T>`. 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<Arc<MyWorkItem>>,
> }
> 
> impl_has_work! {
>      impl HasWork<Arc<MyWorkItem>> for MyWorkItem { self.work_field }
> }
> ```
> 
> So to summarize, given a pointer to an allocation containing a work
> item, you can use the `HasWork<T>` trait to offset the pointer to the
> `work_struct` field. The function pointer in the `work_struct` field is
> guaranteed to be a function that knows what the original pointer type
> was, and using that information, it can undo the offset operation by
> looking up what the offset was via the `HasWork<T>` trait.
> 
> This design supports work items with multiple `work_struct` fields by
> using different pointer types. For example, you might define structs
> like these:
> 
> ```
> struct MyPointer1(Arc<MyWorkItem>);
> struct MyPointer2(Arc<MyWorkItem>);
> 
> struct MyWorkItem {
>      work1: Work<MyPointer1>,
>      work2: Work<MyPointer2>,
> }
> ```
> 
> Then, the wrapper structs `MyPointer1` and `MyPointer2` will take the
> role as the pointer type. By using one or the other, you tell the
> workqueue which `work_struct` field to use. This pattern is called the
> "newtype" pattern.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> This commit introduces `ArcWorkItem`, `BoxWorkItem`, and
> `define_work_adapter_newtype!` that make it possible to use the
> workqueue without any unsafe code whatsoever.
> 
> The `ArcWorkItem` and `BoxWorkItem` traits are used when a struct has a
> single `work_struct` field.
> 
> The `define_work_adapter_newtype!` macro is used when a struct has
> multiple `work_struct` fields. For each `work_struct` field, a newtype
> struct is defined that wraps `Arc<TheStruct>`, and pushing an instance
> of the newtype to a workqueue will enqueue it using the associated
> `work_struct` field. The newtypes are matched with `work_struct` fields
> by having the T in `Work<T>` be the newtype.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]
> +
> +unsafe impl<T> WorkItem for Arc<T>
> +where
> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
> +{
> +    type EnqueueOutput = Result<(), Self>;
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        let ptr = Arc::into_raw(self);
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {

Match for boolean is not a good pattern in my eyes, if-else should be
used instead. Also aren't the parens around the closure unnecessary?

> +            true => Ok(()),
> +            // SAFETY: The work queue has not taken ownership of the pointer.
> +            false => Err(unsafe { Arc::from_raw(ptr) }),
> +        }
> +    }
> +}
> +
> [...]
> +
> +unsafe impl<T> WorkItem for Pin<Box<T>>
> +where
> +    T: BoxWorkItem + HasWork<Self> + ?Sized,
> +{
> +    // When a box is in a workqueue, the workqueue has exclusive ownership of the box. Therefore,
> +    // it's not possible to enqueue a box while it is in a workqueue.
> +    type EnqueueOutput = ();
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F)
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        // SAFETY: We will not used the contents in an unpinned manner.
> +        let ptr = unsafe { Box::into_raw(Pin::into_inner_unchecked(self)) };
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *mut u8).add(off) as *mut Work<Self> };
> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {

Same as above.

> +            true => {}
> +            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +            // workqueue.
> +            false => unsafe { core::hint::unreachable_unchecked() },
> +        }
> +    }
> +}
> +
> [...]
> +
> +/// Helper macro for structs with several `Work` fields that can be in several queues at once.
> +///
> +/// For each `Work` field in your type `T`, a newtype struct that wraps an `Arc<T>` or
> +/// `Pin<Box<T>>` should be defined.
> +///
> +/// # Examples
> +///
> +/// ```

There must be those work macro and type imports here I think.

> +/// struct MyStruct {
> +///     work1: Work<MyStructWork1>,
> +///     work2: Work<MyStructWork2>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyStructWork1> for MyStruct { self.work1 }
> +///     impl HasWork<MyStructWork2> for MyStruct { self.work2 }
> +/// }
> +///
> +/// define_work_adapter_newtype! {
> +///     struct MyStructWork1(Arc<MyStruct>);
> +///     struct MyStructWork2(Arc<MyStruct>);
> +/// }
> +///
> +/// impl MyStructWork1 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +///
> +/// impl MyStructWork2 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +/// ```
> +///
> +/// This will let you push a `MyStructWork1(arc)` or `MyStructWork2(arc)` to a work queue. The [`Arc`]
> +/// can be in two work queues at the same time, and the `run` method on the wrapper type is called
> +/// when the work item is called.
> +///
> +/// [`Arc`]: crate::sync::Arc
> +#[macro_export]
> +macro_rules! define_work_adapter_newtype {
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Arc<$inner:ty> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub $crate::sync::Arc<$inner>);
> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ::core::result::Result<(), $name>;
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                let ptr = $crate::sync::Arc::into_raw(self.0);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Arc::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr.cast_mut()) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {

Same as what I've said on those `WorkItem` impls.

> +                    true => Ok(()),
> +                    // SAFETY: The work queue has not taken ownership of the pointer.
> +                    false => Err($name(unsafe { $crate::sync::Arc::from_raw(ptr) })),
> +                }
> +            }
> +        }
> [...]
> +    };
> +
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Pin<Box<$inner:ty>> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub ::core::pin::Pin<::alloc::boxed::Box<$inner>>);
> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ();
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F)
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                // SAFETY: We will not used the contents in an unpinned manner.
> +                let boxed = unsafe { ::core::pin::Pin::into_inner_unchecked(self.0) };
> +                let ptr = ::alloc::boxed::Box::into_raw(boxed);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Box::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {

Same as above.

> +                    true => {},
> +                    // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +                    // workqueue.
> +                    false => unsafe { ::core::hint::unreachable_unchecked() },
> +                }
> +            }
> +        }
> +
> [...]

Re: [PATCH v1 7/7] rust: workqueue: add `try_spawn` helper method

[Martin Rodriguez Reboredo]

On 5/17/23 17:31, Alice Ryhl wrote:
> 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v1 1/7] rust: workqueue: add low-level workqueue bindings

[Alice Ryhl]

On 5/18/23 16:51, Martin Rodriguez Reboredo wrote:
> On 5/17/23 17:31, Alice Ryhl wrote:
>> +    /// Enqueues a work item.
>> +    ///
>> +    /// This may fail if the work item is already enqueued in a workqueue.
>
> Wouldn't be worth to mention that, if not implied, the item it's going
> to be worked on an unbound CPU?

I'm not really sure what you mean. Can you elaborate?

Alice

Re: [PATCH v1 1/7] rust: workqueue: add low-level workqueue bindings

[Martin Rodriguez Reboredo]

On 5/19/23 06:40, Alice Ryhl wrote:
> On 5/18/23 16:51, Martin Rodriguez Reboredo wrote:
>> On 5/17/23 17:31, Alice Ryhl wrote:
>>> +    /// Enqueues a work item.
>>> +    ///
>>> +    /// This may fail if the work item is already enqueued in a workqueue.
>>
>> Wouldn't be worth to mention that, if not implied, the item it's going
>> to be worked on an unbound CPU?
> 
> I'm not really sure what you mean. Can you elaborate?
> 
> Alice

I've meant that if it's good to mention that `queue_work_on` is going
to be called with `WORK_CPU_UNBOUND` so that API users know about it.

Re: [PATCH v1 1/7] rust: workqueue: add low-level workqueue bindings

[Alice Ryhl]

On 5/19/23 09:04, Martin Rodriguez Reboredo wrote:
> On 5/19/23 06:40, Alice Ryhl wrote:
>> On 5/18/23 16:51, Martin Rodriguez Reboredo wrote:
>>> On 5/17/23 17:31, Alice Ryhl wrote:
>>>> +    /// Enqueues a work item.
>>>> +    ///
>>>> +    /// This may fail if the work item is already enqueued in a workqueue.
>>>
>>> Wouldn't be worth to mention that, if not implied, the item it's going
>>> to be worked on an unbound CPU?
>> 
>> I'm not really sure what you mean. Can you elaborate?
> 
> I've meant that if it's good to mention that `queue_work_on` is going
> to be called with `WORK_CPU_UNBOUND` so that API users know about it.

Ah, I misunderstood at first. I thought you were commenting on the "This
may fail if ..." sentence. I'll go ahead and add that to the
documentation. I will include it in the next patch set once I have
looked at your other reviews.

This part of the next version will look like this:

+    /// 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<T: WorkItem + Send + 'static>(&self, w: T) -> T::EnqueueOutput {

Alice

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Alice Ryhl]

On 5/18/23 21:17, Martin Rodriguez Reboredo wrote:
> On 5/17/23 17:31, Alice Ryhl wrote:
>> +unsafe impl<T> WorkItem for Arc<T>
>> +where
>> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
>> +{
>> +    type EnqueueOutput = Result<(), Self>;
>> +
>> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>> +    where
>> +        F: FnOnce(*mut bindings::work_struct) -> bool,
>> +    {
>> +        let ptr = Arc::into_raw(self);
>> +
>> +        // Using `get_work_offset` here for object-safety.
>> +        //
>> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
>> +        let off = unsafe { (&*ptr).get_work_offset() };
>> +
>> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
>> +        // `Work<Self>` in the same allocation.
>> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
>> +        // SAFETY: The pointer is not dangling.
>> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
>> +
>> +        match (queue_work_on)(work_ptr) {
> 
> Match for boolean is not a good pattern in my eyes, if-else should be
> used instead.

I think this is a question of style. For a comparison:

match (queue_work_on)(work_ptr) {
    true => Ok(()),
    // SAFETY: The work queue has not taken ownership of the pointer.
    false => Err(unsafe { Arc::from_raw(ptr) }),
}

vs

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) }),
}

I'm happy to change it if others disagree, but when the branches
evaluate to a short expression like they do here, I quite like the first
version.

> Also aren't the parens around the closure unnecessary?

Hmm, parenthesises are often required around closures, but it's possible
that it is only required for stuff like `self.closure(args)` to
disambiguate between a `closure` field (of pointer type) and a `closure`
method. I can check and remove them if they are not necessary.

Re: [PATCH v1 0/7] Bindings for the workqueue

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> On Wed, 17 May 2023 11:48:19 -1000, Tejun Heo wrote:
>> I tried to read the patches but am too dumb to understand much.
>
> The patch is more complicated than I would have liked, unfortunately.
> However, as I mentioned in the cover letter, simplifications should be
> on their way.
>
> Luckily, using the workqueue bindings is simpler than the bindings
> themselves.
>
>> Any chance you can provide some examples so that I can at least
>> imagine how workqueue would be used from rust side?
>
> Yes, of course!

If you have bandwidth for it, it would be awesome to see some examples
in the series as well (for /samples/rust).

BR Andreas

>
> The simplest way to use the workqueue is to use the `try_spawn` method
> introduced by the last patch in the series. With this function, you just
> pass a function pointer to the `try_spawn` method, and it schedules the
> function for execution. Unfortunately this allocates memory, making it
> a fallible operation.
>
> To avoid allocation memory, we do something else. As an example, we can
> look at the Rust binder driver that I am currently working on. Here is
> how it will be used in the binder driver: First, the `Process` struct
> will be given a `work_struct` field:
>
> #[pin_data]
> pub(crate) struct Process {
>     // Work node for deferred work item.
>     #[pin]
>     defer_work: Work<Arc<Process>>,
>
>     // Other fields follow...
> }
>
> Here, we use the type `Work<Arc<Process>>` for our field. This type is
> the Rust wrapper for `work_struct`. The generic parameter to `Work`
> should be the pointer type used to access `Process`, and in this case it
> is `Arc<Process>`. The pointer type `Arc` is used for reference
> counting, and its a pointer type that owns a ref-count to the inner
> value. (So e.g., it decrements the ref-cout when the arc goes out of
> scope.) Arc is an abbreviation of "atomic reference count". This means
> that while it is enqueued in the workqueue, the workqueue owns a
> ref-count to the process.
>
> Next, binder will use the `impl_has_work!` macro to declare that it
> wants to use `defer_work` as its `work_struct` field. That looks like
> this:
>
> kernel::impl_has_work! {
>     impl HasWork<Arc<Process>> for Process { self.defer_work }
> }
>
> To define the code that should run when the work item is executed on the
> workqueue, binder does the following:
>
> impl workqueue::ArcWorkItem for Process {
>     fn run(self: Arc<Process>) {
>         // this runs when the work item is executed
>     }
> }
>
> Finally to schedule it to the system workqueue, it does the following:
>
> let _ = workqueue::system().enqueue(process);
>
> Here, the `enqueue` call is fallible, since it might fail if the process
> has already been enqueued to a work queue. However, binder just uses
> `let _ =` to ignore the failure, since it doesn't need to do anything
> special in that case.
>
> I hope that helps, and let me know if you have any further questions.
>
> Alice

Re: [PATCH v1 0/7] Bindings for the workqueue

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> 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.
>
> This patch could be significantly simplified if we already had the field
> projection bindings. However, we have decided to upstream the current
> version that does not depend on field projection first - the PR that
> introduces field projections will then include a commit that simplifies
> the workqueue implementation. (In particular, it would simplify the 5th
> patch in this series.)
>
> The first 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`.
>
> Alice Ryhl (4):
>   rust: workqueue: add low-level workqueue bindings
>   rust: workqueue: add helper for defining work_struct fields
>   rust: workqueue: add safe API to workqueue
>   rust: workqueue: add `try_spawn` helper method
>
> Wedson Almeida Filho (3):
>   rust: add offset_of! macro
>   rust: sync: add `Arc::{from_raw, into_raw}`
>   rust: workqueue: define built-in queues
>
>  rust/helpers.c           |   8 +
>  rust/kernel/lib.rs       |  37 ++
>  rust/kernel/sync/arc.rs  |  44 +++
>  rust/kernel/workqueue.rs | 715 +++++++++++++++++++++++++++++++++++++++
>  scripts/Makefile.build   |   2 +-
>  5 files changed, 805 insertions(+), 1 deletion(-)
>  create mode 100644 rust/kernel/workqueue.rs
>
>
> base-commit: ac9a78681b921877518763ba0e89202254349d1b

This does not compile for me. Could you link dependencies to be applied
first?

Best regards,
Andreas

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Gary Guo]

On Wed, 17 May 2023 20:31:15 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> 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>
> ---
>  rust/kernel/lib.rs      |  1 +
>  rust/kernel/sync/arc.rs | 44 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index cdf9fe999328..82854c86e65d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -22,6 +22,7 @@
>  #![feature(generic_associated_types)]
>  #![feature(new_uninit)]
>  #![feature(pin_macro)]
> +#![feature(ptr_metadata)]
>  #![feature(receiver_trait)]
>  #![feature(unsize)]
>  
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index e6d206242465..7c55a9178dfb 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -210,6 +210,50 @@ impl<T: ?Sized> Arc<T> {
>          }
>      }
>  
> +    /// 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`].
> +    ///
> +    /// This code relies on the `repr(C)` layout of structs as described in
> +    /// <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
> +    /// can only be called once for each previous call to [`Arc::into_raw`].
> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        let val_align = core::mem::align_of_val(unsafe { &*ptr });
> +        let refcount_size = core::mem::size_of::<Opaque<bindings::refcount_t>>();
> +
> +        // Use the `repr(C)` algorithm to compute the offset of `data` in `ArcInner`.
> +        //
> +        // Pseudo-code for the `#[repr(C)]` algorithm can be found here:
> +        // <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>
> +        let mut val_offset = refcount_size;
> +        let val_misalign = val_offset % val_align;
> +        if val_misalign > 0 {
> +            val_offset += val_align - val_misalign;
> +        }

Given the layout of the whole ArcInner can be calculated as

	Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().0.pad_to_align()

The offset of `data` could be more intuitively calculated by

	Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().1

or

	Layout::new::<bindings::refcount_t>().align_to(val_align).unwrap_unchecked().pad_to_align().size()

Best,
Gary

Re: [PATCH v1 2/7] rust: add offset_of! macro

[Gary Guo]

On Wed, 17 May 2023 20:31:14 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This macro is used to compute the offset of a field in a struct.
> 
> This commit enables two unstable features that are necessary for using
> the macro in a constant. However, this is not a problem as the macro
> will become available from the Rust standard library soon [1]. The
> unstable features can be disabled again once that happens.
> 
> The macro in this patch does not support sub-fields. That is, you cannot
> write `offset_of!(MyStruct, field.sub_field)` to get the offset of
> `sub_field` with `field`'s type being a struct with a field called
> `sub_field`. This is because `field` might be a `Box<SubStruct>`, which
> means that you would be trying to compute the offset to something in an
> entirely different allocation. There's no easy way to fix the current
> macro to support subfields, but the version being added to the standard
> library should support it, so the limitation is temporary and not a big
> deal.
> 
> Link: https://github.com/rust-lang/rust/issues/106655 [1]
> 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>
> ---
>  rust/kernel/lib.rs     | 35 +++++++++++++++++++++++++++++++++++
>  scripts/Makefile.build |  2 +-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c718524056a6..cdf9fe999328 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,8 @@
>  #![no_std]
>  #![feature(allocator_api)]
>  #![feature(coerce_unsized)]
> +#![feature(const_ptr_offset_from)]
> +#![feature(const_refs_to_cell)]
>  #![feature(core_ffi_c)]
>  #![feature(dispatch_from_dyn)]
>  #![feature(explicit_generic_args_with_impl_trait)]
> @@ -102,3 +104,36 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>      // instead of `!`. See <https://github.com/rust-lang/rust-bindgen/issues/2094>.
>      loop {}
>  }
> +
> +/// Calculates the offset of a field from the beginning of the struct it belongs to.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// #[repr(C)]
> +/// struct Test {
> +///     a: u64,
> +///     b: u32,
> +/// }
> +///
> +/// assert_eq!(kernel::offset_of!(Test, b), 8);
> +/// ```
> +#[macro_export]
> +macro_rules! offset_of {
> +    ($type:ty, $field:ident) => {{
> +        let tmp = ::core::mem::MaybeUninit::<$type>::uninit();
> +        let outer = tmp.as_ptr();
> +        // To avoid warnings when nesting `unsafe` blocks.
> +        #[allow(unused_unsafe)]
> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
> +        // reference.
> +        let inner = unsafe { ::core::ptr::addr_of!((*outer).$field) } as *const u8;
> +        // To avoid warnings when nesting `unsafe` blocks.
> +        #[allow(unused_unsafe)]
> +        // SAFETY: The two pointers are within the same allocation block.
> +        unsafe {
> +            inner.offset_from(outer as *const u8) as usize
> +        }

This has no protection against using `Deref`. The memoffset crate has a 

let $type { $field: _, .. };

line to ensure that the field is a direct member of type and deref is
not happening.

> +    }};
> +}
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 9f94fc83f086..ec583d13dde2 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
>  
> -rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
> +rust_allowed_features := const_ptr_offset_from,const_refs_to_cell,core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro

Side note: once we bump our compiler to 1.71, we should switch to the
built-in `offset_of!` macro and get rid of these unstable features.

Best,
Gary

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> 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>
> ---
>  rust/kernel/lib.rs      |  1 +
>  rust/kernel/sync/arc.rs | 44 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index cdf9fe999328..82854c86e65d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -22,6 +22,7 @@
>  #![feature(generic_associated_types)]
>  #![feature(new_uninit)]
>  #![feature(pin_macro)]
> +#![feature(ptr_metadata)]
>  #![feature(receiver_trait)]
>  #![feature(unsize)]
>  
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index e6d206242465..7c55a9178dfb 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -210,6 +210,50 @@ impl<T: ?Sized> Arc<T> {
>          }
>      }
>  
> +    /// 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`].
> +    ///
> +    /// This code relies on the `repr(C)` layout of structs as described in
> +    /// <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
> +    /// can only be called once for each previous call to [`Arc::into_raw`].
> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> +        // SAFETY: The safety requirement ensures that the pointer is valid.
> +        let val_align = core::mem::align_of_val(unsafe { &*ptr });
> +        let refcount_size = core::mem::size_of::<Opaque<bindings::refcount_t>>();
> +
> +        // Use the `repr(C)` algorithm to compute the offset of `data` in `ArcInner`.
> +        //
> +        // Pseudo-code for the `#[repr(C)]` algorithm can be found here:
> +        // <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>
> +        let mut val_offset = refcount_size;
> +        let val_misalign = val_offset % val_align;
> +        if val_misalign > 0 {
> +            val_offset += val_align - val_misalign;
> +        }
> +
> +        // This preserves the metadata in the pointer, if any.
> +        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);

I can't follow this. How does this work? `ptr` was for field
`inner.data: T`, but we are casting to `ArcInner<T>`.

> +        let ptr = (ptr as *mut u8).wrapping_sub(val_offset) as *mut ();
> +        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);

Metadata was obtained from a pointer pointing to `inner.data`, we then
move it back to beginning of `ArcInner<T>` and then reconstruct the
potentially fat pointer with metadata from the pointer to `T`? How can
this be right?

BR Andreas

> +
> +        // 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

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Alice Ryhl]

Andreas Hindborg <nmi@metaspace.dk> writes:
> Alice Ryhl <aliceryhl@google.com> writes:
>> +        // This preserves the metadata in the pointer, if any.
>> +        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);
> 
> I can't follow this. How does this work? `ptr` was for field
> `inner.data: T`, but we are casting to `ArcInner<T>`.
> 
>> +        let ptr = (ptr as *mut u8).wrapping_sub(val_offset) as *mut ();
>> +        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);
> 
> Metadata was obtained from a pointer pointing to `inner.data`, we then
> move it back to beginning of `ArcInner<T>` and then reconstruct the
> potentially fat pointer with metadata from the pointer to `T`? How can
> this be right?

The metadata of a struct is always the metadata of its last field, so
both `*mut T` and `*mut ArcInner<T>` have the same metadata. Because of
that, moving the metadata over from one type to the other is ok.

The reason that I cast to an `ArcInner<T>` pointer before calling
`metadata` is because I get a type mismatch otherwise for the metadata,
since the compiler doesn't unify the metadata types when the type is
generic.

Alice

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Alice Ryhl]

Gary Guo <gary@garyguo.net> writes:
> On Wed, 17 May 2023 20:31:15 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> +    /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
>> +    ///
>> +    /// This code relies on the `repr(C)` layout of structs as described in
>> +    /// <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
>> +    /// can only be called once for each previous call to [`Arc::into_raw`].
>> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
>> +        // SAFETY: The safety requirement ensures that the pointer is valid.
>> +        let val_align = core::mem::align_of_val(unsafe { &*ptr });
>> +        let refcount_size = core::mem::size_of::<Opaque<bindings::refcount_t>>();
>> +
>> +        // Use the `repr(C)` algorithm to compute the offset of `data` in `ArcInner`.
>> +        //
>> +        // Pseudo-code for the `#[repr(C)]` algorithm can be found here:
>> +        // <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>
>> +        let mut val_offset = refcount_size;
>> +        let val_misalign = val_offset % val_align;
>> +        if val_misalign > 0 {
>> +            val_offset += val_align - val_misalign;
>> +        }
> 
> Given the layout of the whole ArcInner can be calculated as
> 
> 	Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().0.pad_to_align()
> 
> The offset of `data` could be more intuitively calculated by
> 
> 	Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().1
> 
> or
> 
> 	Layout::new::<bindings::refcount_t>().align_to(val_align).unwrap_unchecked().pad_to_align().size()

I'm not a big fan of the `pad_to_align` version (which is also what
the rust branch uses), but I like the version you posted with
`extend`, and I agree that it is clear and intuitive. I will use that
in the next version of the patchset. Thanks for the suggestion.

Alice

Re: [PATCH v1 2/7] rust: add offset_of! macro

[Alice Ryhl]

Gary Guo <gary@garyguo.net> writes:
> On Wed, 17 May 2023 20:31:14 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> +#[macro_export]
>> +macro_rules! offset_of {
>> +    ($type:ty, $field:ident) => {{
>> +        let tmp = ::core::mem::MaybeUninit::<$type>::uninit();
>> +        let outer = tmp.as_ptr();
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
>> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
>> +        // reference.
>> +        let inner = unsafe { ::core::ptr::addr_of!((*outer).$field) } as *const u8;
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The two pointers are within the same allocation block.
>> +        unsafe {
>> +            inner.offset_from(outer as *const u8) as usize
>> +        }
> 
> This has no protection against using `Deref`. The memoffset crate has a 
> 
> let $type { $field: _, .. };
> 
> line to ensure that the field is a direct member of type and deref is
> not happening.

Added. I had to change `$type:ty` to `$type:path` to get that to
compile, since otherwise you can't use the token in a pattern. However,
I think it's fine - this is temporary anyway until the standard library
gets the macro.
 
>> +    }};
>> +}
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 9f94fc83f086..ec583d13dde2 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>>  # Compile Rust sources (.rs)
>>  # ---------------------------------------------------------------------------
>>  
>> -rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
>> +rust_allowed_features := const_ptr_offset_from,const_refs_to_cell,core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
> 
> Side note: once we bump our compiler to 1.71, we should switch to the
> built-in `offset_of!` macro and get rid of these unstable features.

Agreed. I mentioned that in the commit message.

Alice

Re: [PATCH v1 0/7] Bindings for the workqueue

[Alice Ryhl]

Andreas Hindborg <nmi@metaspace.dk> writes:
> This does not compile for me. Could you link dependencies to be applied
> first?

I messed up something related to the patch for specifying error type on
`Result` [1]. This patch doesn't _need_ to depend on that though, so the
next version of this patchset should compile without it.

Alice

[1]: https://lore.kernel.org/all/20230502124015.356001-1-aliceryhl@google.com/

Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work_struct fields

[Benno Lossin]

On Wednesday, May 17th, 2023 at 22:31, 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. This
> commit will use traits so that they know about each other according to
> the following cycle:
> 
>  * The pointer type. It knows the type of the work item struct.
>  * The work item struct. It knows the offset of its `work_struct` field.
>  * The `work_struct` field. It knows the pointer type.
> 
> There's nothing special about making the pointer type know the type of
> the struct it points at. Pointers generally always know that
> information.
> 
> However, making the `work_struct` field know about the pointer type is
> less commonly seen. This is done by using a generic parameter: the
> `work_struct` field will have the type `Work<T>`, where T will be the
> pointer type in use. The pointer type is required to implement the
> `WorkItemAdapter` trait, which defines the function pointer to store in
> the `work_struct` field. The `Work<T>` type guarantees that the
> `work_struct` inside it uses `<T as WorkItemAdapter>::run` as its
> function pointer.
> 
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T>`. 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<Arc<MyWorkItem>>,
> }
> 
> impl_has_work! {
>     impl HasWork<Arc<MyWorkItem>> for MyWorkItem { self.work_field }
> }
> ```
> 
> So to summarize, given a pointer to an allocation containing a work
> item, you can use the `HasWork<T>` trait to offset the pointer to the
> `work_struct` field. The function pointer in the `work_struct` field is
> guaranteed to be a function that knows what the original pointer type
> was, and using that information, it can undo the offset operation by
> looking up what the offset was via the `HasWork<T>` trait.
> 
> This design supports work items with multiple `work_struct` fields by
> using different pointer types. For example, you might define structs
> like these:
> 
> ```
> struct MyPointer1(Arc<MyWorkItem>);
> struct MyPointer2(Arc<MyWorkItem>);
> 
> struct MyWorkItem {
>     work1: Work<MyPointer1>,
>     work2: Work<MyPointer2>,
> }
> ```
> 
> Then, the wrapper structs `MyPointer1` and `MyPointer2` will take the
> role as the pointer type. By using one or the other, you tell the
> workqueue which `work_struct` field to use. This pattern is called the
> "newtype" pattern.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/helpers.c           |   8 ++
>  rust/kernel/workqueue.rs | 183 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 190 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 81e80261d597..7f0c2fe2fbeb 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -26,6 +26,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/sched/signal.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
> 
>  __noreturn void rust_helper_BUG(void)
>  {
> @@ -128,6 +129,13 @@ void rust_helper_put_task_struct(struct task_struct *t)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
> 
> +void rust_helper___INIT_WORK(struct work_struct *work, work_func_t func,
> +			     bool on_stack)
> +{
> +	__INIT_WORK(work, func, on_stack);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper___INIT_WORK);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 22205d3bda72..7509618af252 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -4,7 +4,8 @@
>  //!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
> 
> -use crate::{bindings, types::Opaque};
> +use crate::{bindings, prelude::*, types::Opaque};
> +use core::marker::{PhantomData, PhantomPinned};
> 
>  /// A kernel work queue.
>  ///
> @@ -98,6 +99,186 @@ pub unsafe trait WorkItem {
>          F: FnOnce(*mut bindings::work_struct) -> bool;
>  }
> 
> +/// Defines the method that should be called when a work item is executed.
> +///
> +/// 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`]: WorkItem::__enqueue
> +/// [`run`]: WorkItemAdapter::run
> +pub unsafe trait WorkItemAdapter: WorkItem {
> +    /// Run this work item.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Must only be called via the function pointer that [`__enqueue`] provides to the
> +    /// `queue_work_on` closure, and only as described in the documentation of `queue_work_on`.
> +    ///
> +    /// [`__enqueue`]: WorkItem::__enqueue
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
> +}
> +
> +/// Links for a work item.
> +///
> +/// This struct contains a function pointer to the `T::run` function from the [`WorkItemAdapter`]
> +/// 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 [`WorkItemAdapter`] that uses
> +/// it.
> +#[repr(transparent)]
> +pub struct Work<T: ?Sized> {
> +    work: Opaque<bindings::work_struct>,
> +    _pin: PhantomPinned,
> +    _adapter: 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> Send for Work<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> Sync for Work<T> {}
> +
> +impl<T: ?Sized> Work<T> {
> +    /// Creates a new instance of [`Work`].
> +    #[inline]
> +    #[allow(clippy::new_ret_no_self)]
> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: WorkItemAdapter,
> +    {
> +        // SAFETY: The `WorkItemAdapter` implementation promises that `T::run` can be used as the
> +        // work item function.
> +        unsafe {
> +            kernel::init::pin_init_from_closure(move |slot| {
> +                bindings::__INIT_WORK(Self::raw_get(slot), Some(T::run), false);
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Get a pointer to the inner `work_struct`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided pointer must not be dangling. (But it 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 valid.
> +        //
> +        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
> +        // the compiler does not complain that `work` is unused.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
> +    }
> +}
> +
> +/// Declares that a type has a [`Work<T>`] field.
> +///
> +/// # Safety
> +///
> +/// The [`OFFSET`] constant must be the offset of a field in Self of type [`Work<T>`]. The methods on
> +/// this trait must have exactly the behavior that the definitions given below have.
> +///

I don't like this safety section, since the discharging safety comment
will just be "I implemented everything correctly" which for me is the same
as just writing no safety comment at all.

I am working on improving safety comments in general, so we can defer
improving this until I have come up with a good plan. (If you still want
to improve it, feel free to do so)

> +/// [`Work<T>`]: Work
> +/// [`OFFSET`]: HasWork::OFFSET
> +pub unsafe trait HasWork<T> {
> +    /// The offset of the [`Work<T>`] field.
> +    ///
> +    /// [`Work<T>`]: Work
> +    const OFFSET: usize;
> +
> +    /// Returns the offset of the [`Work<T>`] field.
> +    ///
> +    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not Sized.
> +    ///
> +    /// [`Work<T>`]: Work
> +    /// [`OFFSET`]: HasWork::OFFSET
> +    #[inline]
> +    fn get_work_offset(&self) -> usize {
> +        Self::OFFSET
> +    }
> +
> +    /// Returns a pointer to the [`Work<T>`] field.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must not be dangling. (But the memory need not be initialized.)
> +    ///
> +    /// [`Work<T>`]: Work
> +    #[inline]
> +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T>
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: The caller promises that the pointer is not dangling.
> +        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T> }
> +    }
> +
> +    /// Returns a pointer to the struct containing the [`Work<T>`] field.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must not be dangling. (But the memory need not be initialized.)

The pointer also must point to a `Work<T>` that is embedded at `OFFSET`
inside of `Self`.

> +    ///
> +    /// [`Work<T>`]: Work
> +    #[inline]
> +    unsafe fn work_container_of(ptr: *mut Work<T>) -> *mut Self
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: The caller promises that the pointer is not dangling.
> +        unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
> +    }
> +}
> +
> +/// Used to safely implement the [`HasWork<T>`] trait.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct MyStruct {
> +///     work_field: Work<Arc<MyStruct>>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
> +/// }
> +/// ```
> +///
> +/// [`HasWork<T>`]: HasWork
> +#[macro_export]
> +macro_rules! impl_has_work {
> +    ($(impl$(<$($implarg:ident),*>)?
> +       HasWork<$work_type:ty>
> +       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> for $self $(<$($selfarg),*>)? {
> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
> +
> +            #[inline]
> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
> +                }
> +            }
> +        }
> +    )*};
> +}

I don't really like the verbosity that this creates, but for the moment it
should be fine. When/if we get field projections, we can build a much
better proc-macro, so I think we can defer improving this until then.

> +
>  /// Returns the system work queue (`system_wq`).
>  ///
>  /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
> --
> 2.40.1.606.ga4b1b128d6-goog
>

--
Cheers,
Benno

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Benno Lossin]

On Wednesday, May 17th, 2023 at 22:31, Alice Ryhl <aliceryhl@google.com> wrote:
> This commit introduces `ArcWorkItem`, `BoxWorkItem`, and
> `define_work_adapter_newtype!` that make it possible to use the
> workqueue without any unsafe code whatsoever.
> 
> The `ArcWorkItem` and `BoxWorkItem` traits are used when a struct has a
> single `work_struct` field.
> 
> The `define_work_adapter_newtype!` macro is used when a struct has
> multiple `work_struct` fields. For each `work_struct` field, a newtype
> struct is defined that wraps `Arc<TheStruct>`, and pushing an instance
> of the newtype to a workqueue will enqueue it using the associated
> `work_struct` field. The newtypes are matched with `work_struct` fields
> by having the T in `Work<T>` be the newtype.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/workqueue.rs | 332 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 331 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 7509618af252..007005ddcaf0 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -4,8 +4,9 @@
>  //!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
> 
> -use crate::{bindings, prelude::*, types::Opaque};
> +use crate::{bindings, prelude::*, sync::Arc, types::Opaque};
>  use core::marker::{PhantomData, PhantomPinned};
> +use core::result::Result;
> 
>  /// A kernel work queue.
>  ///
> @@ -279,6 +280,335 @@ macro_rules! impl_has_work {
>      )*};
>  }
> 
> +/// Declares that [`Arc<Self>`] should implement [`WorkItem`].
> +///
> +/// # Examples
> +///
> +/// The example below will make [`Arc<MyStruct>`] implement the [`WorkItem`] trait so that you can
> +/// enqueue it in a workqueue.
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct MyStruct {
> +///     work_field: Work<Arc<MyStruct>>,
> +/// }
> +///
> +/// kernel::impl_has_work! {
> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
> +/// }
> +///
> +/// impl ArcWorkItem for MyStruct {
> +///     fn run(self: Arc<Self>) {
> +///         pr_info!("Executing MyStruct on a workqueue.");
> +///     }
> +/// }
> +/// ```
> +///
> +/// [`Arc<Self>`]: crate::sync::Arc
> +/// [`Arc<MyStruct>`]: crate::sync::Arc
> +pub trait ArcWorkItem {
> +    /// Called when this work item is executed.
> +    fn run(self: Arc<Self>);
> +}
> +
> +unsafe impl<T> WorkItem for Arc<T>
> +where
> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
> +{
> +    type EnqueueOutput = Result<(), Self>;
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        let ptr = Arc::into_raw(self);
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {
> +            true => Ok(()),
> +            // SAFETY: The work queue has not taken ownership of the pointer.
> +            false => Err(unsafe { Arc::from_raw(ptr) }),
> +        }
> +    }
> +}
> +
> +// Let `Work<Arc<T>>` be usable with types that are `ArcWorkItem`.
> +//
> +// We do not allow unsized types here. The `Work<Arc<T>>` field should always specify the actual
> +// concrete type stored in the `Arc`.
> +//
> +// SAFETY: The `Work<Arc<T>>` field must be initialized with this `run` method because the `Work`
> +// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
> +// same `Work<Arc<T>>` field because `HasWork` promises to always return the same field.
> +unsafe impl<T> WorkItemAdapter for Arc<T>
> +where
> +    T: ArcWorkItem + HasWork<Self> + Sized,
> +{
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
> +        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +        let ptr = ptr as *mut Work<Self>;
> +        // 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) };
> +
> +        arc.run();
> +    }
> +}
> +
> +/// Declares that [`Pin`]`<`[`Box`]`<Self>>` should implement [`WorkItem`].
> +///
> +/// # Examples
> +///
> +/// The example below will make [`Pin`]`<`[`Box`]`<MyStruct>>` implement the [`WorkItem`] trait so
> +/// that you can enqueue it in a workqueue.
> +///
> +/// ```
> +/// struct MyStruct {
> +///     work_field: Work<Pin<Box<MyStruct>>>,
> +/// }
> +///
> +/// kernel::impl_has_work! {
> +///     impl HasWork<Pin<Box<MyStruct>>> for MyStruct { self.work_field }
> +/// }
> +///
> +/// impl BoxWorkItem for MyStruct {
> +///     fn run(self: Pin<Box<MyStruct>>) {
> +///         pr_info!("Executing MyStruct on a workqueue.");
> +///     }
> +/// }
> +/// ```
> +///
> +/// [`Box`]: alloc::boxed::Box
> +/// [`Pin`]: core::pin::Pin
> +pub trait BoxWorkItem {
> +    /// Called when this work item is executed.
> +    fn run(self: Pin<Box<Self>>);
> +}
> +
> +unsafe impl<T> WorkItem for Pin<Box<T>>
> +where
> +    T: BoxWorkItem + HasWork<Self> + ?Sized,
> +{
> +    // When a box is in a workqueue, the workqueue has exclusive ownership of the box. Therefore,
> +    // it's not possible to enqueue a box while it is in a workqueue.
> +    type EnqueueOutput = ();
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F)
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        // SAFETY: We will not used the contents in an unpinned manner.
> +        let ptr = unsafe { Box::into_raw(Pin::into_inner_unchecked(self)) };
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *mut u8).add(off) as *mut Work<Self> };
> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {
> +            true => {}
> +            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +            // workqueue.
> +            false => unsafe { core::hint::unreachable_unchecked() },
> +        }
> +    }
> +}
> +
> +// Let `Work<Pin<Box<T>>>` be usable with types that are `BoxWorkItem`.
> +//
> +// We do not allow unsized types here. The `Work<Pin<Box<T>>>` field should always specify the actual
> +// concrete type stored in the `Box`.
> +//
> +// SAFETY: The `Work<Pin<Box<T>>>` field must be initialized with this run method because the `Work`
> +// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
> +// same `Work<Pin<Box<T>>>` field because `HasWork` promises to always return the same field.
> +unsafe impl<T> WorkItemAdapter for Pin<Box<T>>
> +where
> +    T: BoxWorkItem + HasWork<Self> + Sized,
> +{
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
> +        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +        let ptr = ptr as *mut Work<Self>;
> +        // 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 `Box::into_raw` and we've been given back ownership.
> +        // The box was originally pinned, so pinning it again is ok.
> +        let boxed = unsafe { Pin::new_unchecked(Box::from_raw(ptr)) };
> +
> +        boxed.run();
> +    }
> +}
> +
> +/// Helper macro for structs with several `Work` fields that can be in several queues at once.
> +///
> +/// For each `Work` field in your type `T`, a newtype struct that wraps an `Arc<T>` or
> +/// `Pin<Box<T>>` should be defined.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// struct MyStruct {
> +///     work1: Work<MyStructWork1>,
> +///     work2: Work<MyStructWork2>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyStructWork1> for MyStruct { self.work1 }
> +///     impl HasWork<MyStructWork2> for MyStruct { self.work2 }
> +/// }
> +///
> +/// define_work_adapter_newtype! {
> +///     struct MyStructWork1(Arc<MyStruct>);
> +///     struct MyStructWork2(Arc<MyStruct>);
> +/// }
> +///
> +/// impl MyStructWork1 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +///
> +/// impl MyStructWork2 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +/// ```
> +///
> +/// This will let you push a `MyStructWork1(arc)` or `MyStructWork2(arc)` to a work queue. The [`Arc`]
> +/// can be in two work queues at the same time, and the `run` method on the wrapper type is called
> +/// when the work item is called.
> +///
> +/// [`Arc`]: crate::sync::Arc
> +#[macro_export]
> +macro_rules! define_work_adapter_newtype {
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Arc<$inner:ty> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub $crate::sync::Arc<$inner>);

I am a bit confused as to why these types *contain* a pointer. Shouldn't
these be exactly the same `Work<$inner>`, except they allow multiple `run`
functions? So IMO they should embed a `Work<$inner>` and the
manually defined `run` function would take a `$inner`.

> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ::core::result::Result<(), $name>;
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                let ptr = $crate::sync::Arc::into_raw(self.0);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Arc::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr.cast_mut()) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {
> +                    true => Ok(()),
> +                    // SAFETY: The work queue has not taken ownership of the pointer.
> +                    false => Err($name(unsafe { $crate::sync::Arc::from_raw(ptr) })),
> +                }
> +            }
> +        }
> +
> +        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
> +            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
> +                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
> +                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
> +                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
> +                // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
> +                let arc = unsafe { $crate::sync::Arc::from_raw(ptr) };
> +
> +                $name::run($name(arc));
> +            }
> +        }
> +
> +        define_work_adapter_newtype! { $($rest)* }
> +    };
> +
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Pin<Box<$inner:ty>> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub ::core::pin::Pin<::alloc::boxed::Box<$inner>>);
> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ();
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F)
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                // SAFETY: We will not used the contents in an unpinned manner.
> +                let boxed = unsafe { ::core::pin::Pin::into_inner_unchecked(self.0) };
> +                let ptr = ::alloc::boxed::Box::into_raw(boxed);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Box::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {
> +                    true => {},
> +                    // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +                    // workqueue.
> +                    false => unsafe { ::core::hint::unreachable_unchecked() },
> +                }
> +            }
> +        }
> +
> +        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
> +            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
> +                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
> +                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
> +                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
> +                // SAFETY: This pointer comes from `Box::into_raw` and we've been given back ownership.
> +                let boxed = unsafe { ::alloc::boxed::Box::from_raw(ptr) };
> +                // SAFETY: The box was originally pinned, so pinning it again is ok.
> +                let boxed = unsafe { ::core::pin::Pin::new_unchecked(boxed) };
> +
> +                $name::run($name(boxed));
> +            }
> +        }
> +
> +        define_work_adapter_newtype! { $($rest)* }
> +    };
> +
> +    // After processing the last definition, we call ourselves with no input.
> +    () => {};
> +}
> +
>  /// Returns the system work queue (`system_wq`).
>  ///
>  /// It is the one used by `schedule[_delayed]_work[_on]()`. Multi-CPU multi-threaded. There are
> --
> 2.40.1.606.ga4b1b128d6-goog
> 

--
Cheers,
Benno

Re: [PATCH v1 7/7] rust: workqueue: add `try_spawn` helper method

[Benno Lossin]

On Wednesday, May 17th, 2023 at 22:31, Alice Ryhl <aliceryhl@google.com> wrote:
> 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/workqueue.rs | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 007005ddcaf0..303b72efd95f 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -57,6 +57,42 @@ impl Queue {
>              })
>          }
>      }
> +
> +    /// 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 + Fn()>(&self, func: T) -> Result {

Why is this `Fn()` instead of `FnOnce()`?

> +        let init = pin_init!(ClosureWork {
> +            work <- Work::new(),
> +            func: Some(func),
> +        });
> +
> +        self.enqueue(Box::pin_init(init)?);
> +        Ok(())
> +    }
> +}
> +
> +/// A helper type used in `try_spawn`.
> +#[pin_data]
> +struct ClosureWork<T> {
> +    #[pin]
> +    work: Work<Pin<Box<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()> BoxWorkItem for ClosureWork<T> {
> +    fn run(mut self: Pin<Box<Self>>) {
> +        if let Some(func) = self.as_mut().project().take() {
> +            (func)()
> +        }
> +    }
>  }
> 
>  /// A work item.
> @@ -280,6 +316,10 @@ macro_rules! impl_has_work {
>      )*};
>  }
> 
> +impl_has_work! {
> +    impl<T> HasWork<Pin<Box<Self>>> for ClosureWork<T> { self.work }
> +}
> +
>  /// Declares that [`Arc<Self>`] should implement [`WorkItem`].
>  ///
>  /// # Examples
> --
> 2.40.1.606.ga4b1b128d6-goog
> 

--
Cheers,
Benno

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> Andreas Hindborg <nmi@metaspace.dk> writes:
>> Alice Ryhl <aliceryhl@google.com> writes:
>>> +        // This preserves the metadata in the pointer, if any.
>>> +        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);
>> 
>> I can't follow this. How does this work? `ptr` was for field
>> `inner.data: T`, but we are casting to `ArcInner<T>`.
>> 
>>> +        let ptr = (ptr as *mut u8).wrapping_sub(val_offset) as *mut ();
>>> +        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);
>> 
>> Metadata was obtained from a pointer pointing to `inner.data`, we then
>> move it back to beginning of `ArcInner<T>` and then reconstruct the
>> potentially fat pointer with metadata from the pointer to `T`? How can
>> this be right?
>
> The metadata of a struct is always the metadata of its last field, so
> both `*mut T` and `*mut ArcInner<T>` have the same metadata. Because of
> that, moving the metadata over from one type to the other is ok.
>
> The reason that I cast to an `ArcInner<T>` pointer before calling
> `metadata` is because I get a type mismatch otherwise for the metadata,
> since the compiler doesn't unify the metadata types when the type is
> generic.

OK, cool. In that case, since this is common knowledge (is it?),
could you maybe include a link to the relevant documentation, or a
comment indicating why this is OK?

BR Andreas

Re: [PATCH v1 4/7] rust: workqueue: define built-in queues

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> 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>
> ---
>  rust/kernel/workqueue.rs | 65 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index e66b6b50dfae..22205d3bda72 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -97,3 +97,68 @@ pub unsafe trait WorkItem {
>      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.

Can we add a short definition of what "freezable" means?

BR Andreas

> +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.
> +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) }
> +}

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Gary Guo]

On Thu, 25 May 2023 09:45:29 +0200
Andreas Hindborg <nmi@metaspace.dk> wrote:

> Alice Ryhl <aliceryhl@google.com> writes:
> 
> > Andreas Hindborg <nmi@metaspace.dk> writes:  
> >> Alice Ryhl <aliceryhl@google.com> writes:  
> >>> +        // This preserves the metadata in the pointer, if any.
> >>> +        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);  
> >> 
> >> I can't follow this. How does this work? `ptr` was for field
> >> `inner.data: T`, but we are casting to `ArcInner<T>`.
> >>   
> >>> +        let ptr = (ptr as *mut u8).wrapping_sub(val_offset) as *mut ();
> >>> +        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);  
> >> 
> >> Metadata was obtained from a pointer pointing to `inner.data`, we then
> >> move it back to beginning of `ArcInner<T>` and then reconstruct the
> >> potentially fat pointer with metadata from the pointer to `T`? How can
> >> this be right?  
> >
> > The metadata of a struct is always the metadata of its last field, so
> > both `*mut T` and `*mut ArcInner<T>` have the same metadata. Because of
> > that, moving the metadata over from one type to the other is ok.
> >
> > The reason that I cast to an `ArcInner<T>` pointer before calling
> > `metadata` is because I get a type mismatch otherwise for the metadata,
> > since the compiler doesn't unify the metadata types when the type is
> > generic.  
> 
> OK, cool. In that case, since this is common knowledge (is it?),
> could you maybe include a link to the relevant documentation, or a
> comment indicating why this is OK?
> 
> BR Andreas

This is documented in the doc of Pointee trait:

https://doc.rust-lang.org/std/ptr/trait.Pointee.html

> For structs whose last field is a DST, metadata is the metadata for the last field

Best,
Gary

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> On 5/18/23 21:17, Martin Rodriguez Reboredo wrote:
>> On 5/17/23 17:31, Alice Ryhl wrote:
>>> +unsafe impl<T> WorkItem for Arc<T>
>>> +where
>>> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
>>> +{
>>> +    type EnqueueOutput = Result<(), Self>;
>>> +
>>> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>>> +    where
>>> +        F: FnOnce(*mut bindings::work_struct) -> bool,
>>> +    {
>>> +        let ptr = Arc::into_raw(self);
>>> +
>>> +        // Using `get_work_offset` here for object-safety.
>>> +        //
>>> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
>>> +        let off = unsafe { (&*ptr).get_work_offset() };
>>> +
>>> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
>>> +        // `Work<Self>` in the same allocation.
>>> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
>>> +        // SAFETY: The pointer is not dangling.
>>> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
>>> +
>>> +        match (queue_work_on)(work_ptr) {
>> 
>> Match for boolean is not a good pattern in my eyes, if-else should be
>> used instead.
>
> I think this is a question of style. For a comparison:
>
> match (queue_work_on)(work_ptr) {
>     true => Ok(()),
>     // SAFETY: The work queue has not taken ownership of the pointer.
>     false => Err(unsafe { Arc::from_raw(ptr) }),
> }
>
> vs
>
> 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) }),
> }
>
> I'm happy to change it if others disagree, but when the branches
> evaluate to a short expression like they do here, I quite like the first
> version.

I prefer the first one, but both look OK to me. Is one more idiomatic
than the other, or is it just a matter of personal preference?

BR Andreas

>
>> Also aren't the parens around the closure unnecessary?
>
> Hmm, parenthesises are often required around closures, but it's possible
> that it is only required for stuff like `self.closure(args)` to
> disambiguate between a `closure` field (of pointer type) and a `closure`
> method. I can check and remove them if they are not necessary.

Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Andreas Hindborg]

Gary Guo <gary@garyguo.net> writes:

> On Thu, 25 May 2023 09:45:29 +0200
> Andreas Hindborg <nmi@metaspace.dk> wrote:
>
>> Alice Ryhl <aliceryhl@google.com> writes:
>> 
>> > Andreas Hindborg <nmi@metaspace.dk> writes:  
>> >> Alice Ryhl <aliceryhl@google.com> writes:  
>> >>> +        // This preserves the metadata in the pointer, if any.
>> >>> +        let metadata = core::ptr::metadata(ptr as *const ArcInner<T>);  
>> >> 
>> >> I can't follow this. How does this work? `ptr` was for field
>> >> `inner.data: T`, but we are casting to `ArcInner<T>`.
>> >>   
>> >>> +        let ptr = (ptr as *mut u8).wrapping_sub(val_offset) as *mut ();
>> >>> +        let ptr = core::ptr::from_raw_parts_mut(ptr, metadata);  
>> >> 
>> >> Metadata was obtained from a pointer pointing to `inner.data`, we then
>> >> move it back to beginning of `ArcInner<T>` and then reconstruct the
>> >> potentially fat pointer with metadata from the pointer to `T`? How can
>> >> this be right?  
>> >
>> > The metadata of a struct is always the metadata of its last field, so
>> > both `*mut T` and `*mut ArcInner<T>` have the same metadata. Because of
>> > that, moving the metadata over from one type to the other is ok.
>> >
>> > The reason that I cast to an `ArcInner<T>` pointer before calling
>> > `metadata` is because I get a type mismatch otherwise for the metadata,
>> > since the compiler doesn't unify the metadata types when the type is
>> > generic.  
>> 
>> OK, cool. In that case, since this is common knowledge (is it?),
>> could you maybe include a link to the relevant documentation, or a
>> comment indicating why this is OK?
>> 
>> BR Andreas
>
> This is documented in the doc of Pointee trait:
>
> https://doc.rust-lang.org/std/ptr/trait.Pointee.html

Nice. I think I forgot a _not_ in my last message. I think it would be
nice to add a comment with a link to this documentation and perhaps a
note as to why this works.

BR Andreas

Re: [PATCH v1 1/7] rust: workqueue: add low-level workqueue bindings

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> Define basic low-level bindings to a kernel workqueue. The API defined
> here can only be used unsafely. Later commits will provide safe
> wrappers.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/lib.rs       |  1 +
>  rust/kernel/workqueue.rs | 99 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 rust/kernel/workqueue.rs
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 676995d4e460..c718524056a6 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -47,6 +47,7 @@ pub mod str;
>  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..e66b6b50dfae
> --- /dev/null
> +++ b/rust/kernel/workqueue.rs
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Work queues.
> +//!
> +//! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)

I think we need to add workqueue.h to rust/bindings/bindings_helper.h
with this commit. It is probably transitively included as is, but it
would be the right thing to explicitly include it.

BR Andreas

> +
> +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: Kernel workqueues are usable from any thread.
> +unsafe impl Send for Queue {}
> +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.
> +    pub fn enqueue<T: WorkItem + Send + 'static>(&self, w: T) -> T::EnqueueOutput {
> +        let queue_ptr = self.0.get();
> +
> +        // SAFETY: There are two cases.
> +        //
> +        //  1. If `queue_work_on` returns false, then we failed to push the work item to the queue.
> +        //     In this case, we don't touch the work item again.
> +        //
> +        //  2. If `queue_work_on` returns true, then we pushed the work item to the queue. The work
> +        //     queue will call the function pointer in the `work_struct` at some point in the
> +        //     future. We require `T` to be static, so the type has no lifetimes annotated on it.
> +        //     We require `T` to be send, so there are no thread-safety issues to take care of.
> +        //
> +        // In either case we follow the safety requirements of `__enqueue`.
> +        unsafe {
> +            w.__enqueue(move |work_ptr| {
> +                bindings::queue_work_on(bindings::WORK_CPU_UNBOUND as _, queue_ptr, work_ptr)
> +            })
> +        }
> +    }
> +}
> +
> +/// A work item.
> +///
> +/// This is the low-level trait that is designed for being as general as possible.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that `__enqueue` behaves as documented.
> +pub unsafe trait WorkItem {
> +    /// The return type of [`Queue::enqueue`].
> +    type EnqueueOutput;
> +
> +    /// Enqueues this work item on a queue using the provided `queue_work_on` method.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Calling this method guarantees that the provided closure will be called with a raw pointer
> +    /// to a `struct work_struct`. The closure should behave in the following way:
> +    ///
> +    ///  1. If the `struct work_struct` cannot be pushed to a workqueue because its already in one,
> +    ///     then the closure should return `false`. It may not access the pointer after returning
> +    ///     `false`.
> +    ///  2. If the `struct work_struct` is successfully added to a workqueue, then the closure
> +    ///     should return `true`. When the workqueue executes the work item, it will do so by
> +    ///     calling the function pointer stored in the `struct work_struct`. The work item ensures
> +    ///     that the raw pointer remains valid until that happens.
> +    ///
> +    /// This method may not have any other failure cases than the closure returning `false`. The
> +    /// output type should reflect this, but it may also be an infallible type if the work item
> +    /// statically ensures that pushing the `struct work_struct` will succeed.
> +    ///
> +    /// If the work item type is annotated with any lifetimes, then the workqueue must call the
> +    /// function pointer before any such lifetime expires. (Or it may forget the work item and
> +    /// never call the function pointer at all.)
> +    ///
> +    /// If the work item type is not [`Send`], then the work item must be executed 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;
> +}

Re: [PATCH v1 2/7] rust: add offset_of! macro

[Andreas Hindborg]

Gary Guo <gary@garyguo.net> writes:

> On Wed, 17 May 2023 20:31:14 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>> 
>> This macro is used to compute the offset of a field in a struct.
>> 
>> This commit enables two unstable features that are necessary for using
>> the macro in a constant. However, this is not a problem as the macro
>> will become available from the Rust standard library soon [1]. The
>> unstable features can be disabled again once that happens.
>> 
>> The macro in this patch does not support sub-fields. That is, you cannot
>> write `offset_of!(MyStruct, field.sub_field)` to get the offset of
>> `sub_field` with `field`'s type being a struct with a field called
>> `sub_field`. This is because `field` might be a `Box<SubStruct>`, which
>> means that you would be trying to compute the offset to something in an
>> entirely different allocation. There's no easy way to fix the current
>> macro to support subfields, but the version being added to the standard
>> library should support it, so the limitation is temporary and not a big
>> deal.
>> 
>> Link: https://github.com/rust-lang/rust/issues/106655 [1]
>> 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>
>> ---
>>  rust/kernel/lib.rs     | 35 +++++++++++++++++++++++++++++++++++
>>  scripts/Makefile.build |  2 +-
>>  2 files changed, 36 insertions(+), 1 deletion(-)
>> 
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index c718524056a6..cdf9fe999328 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -14,6 +14,8 @@
>>  #![no_std]
>>  #![feature(allocator_api)]
>>  #![feature(coerce_unsized)]
>> +#![feature(const_ptr_offset_from)]
>> +#![feature(const_refs_to_cell)]
>>  #![feature(core_ffi_c)]
>>  #![feature(dispatch_from_dyn)]
>>  #![feature(explicit_generic_args_with_impl_trait)]
>> @@ -102,3 +104,36 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>>      // instead of `!`. See <https://github.com/rust-lang/rust-bindgen/issues/2094>.
>>      loop {}
>>  }
>> +
>> +/// Calculates the offset of a field from the beginning of the struct it belongs to.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// #[repr(C)]
>> +/// struct Test {
>> +///     a: u64,
>> +///     b: u32,
>> +/// }
>> +///
>> +/// assert_eq!(kernel::offset_of!(Test, b), 8);
>> +/// ```
>> +#[macro_export]
>> +macro_rules! offset_of {
>> +    ($type:ty, $field:ident) => {{
>> +        let tmp = ::core::mem::MaybeUninit::<$type>::uninit();
>> +        let outer = tmp.as_ptr();
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The pointer is valid and aligned, just not initialised; `addr_of` ensures that
>> +        // we don't actually read from `outer` (which would be UB) nor create an intermediate
>> +        // reference.
>> +        let inner = unsafe { ::core::ptr::addr_of!((*outer).$field) } as *const u8;
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The two pointers are within the same allocation block.
>> +        unsafe {
>> +            inner.offset_from(outer as *const u8) as usize
>> +        }
>
> This has no protection against using `Deref`. The memoffset crate has a 
>
> let $type { $field: _, .. };
>
> line to ensure that the field is a direct member of type and deref is
> not happening.

Good catch 👍

>
>> +    }};
>> +}
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 9f94fc83f086..ec583d13dde2 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>>  # Compile Rust sources (.rs)
>>  # ---------------------------------------------------------------------------
>>  
>> -rust_allowed_features := core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
>> +rust_allowed_features := const_ptr_offset_from,const_refs_to_cell,core_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
>
> Side note: once we bump our compiler to 1.71, we should switch to the
> built-in `offset_of!` macro and get rid of these unstable features.
>
> Best,
> Gary

Re: [PATCH v1 5/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. This
> commit will use traits so that they know about each other according to
> the following cycle:
>
>  * The pointer type. It knows the type of the work item struct.
>  * The work item struct. It knows the offset of its `work_struct` field.
>  * The `work_struct` field. It knows the pointer type.
>
> There's nothing special about making the pointer type know the type of
> the struct it points at. Pointers generally always know that
> information.
>
> However, making the `work_struct` field know about the pointer type is
> less commonly seen. This is done by using a generic parameter: the
> `work_struct` field will have the type `Work<T>`, where T will be the
> pointer type in use. The pointer type is required to implement the
> `WorkItemAdapter` trait, which defines the function pointer to store in
> the `work_struct` field. The `Work<T>` type guarantees that the
> `work_struct` inside it uses `<T as WorkItemAdapter>::run` as its
> function pointer.
>
> Finally, to make the work item struct know the offset of its
> `work_struct` field, we use a trait called `HasWork<T>`. If a type
> implements this trait, then the type declares that, at the given offset,
> there is a field of type `Work<T>`. 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<Arc<MyWorkItem>>,
> }
>
> impl_has_work! {
>     impl HasWork<Arc<MyWorkItem>> for MyWorkItem { self.work_field }
> }
> ```
>
> So to summarize, given a pointer to an allocation containing a work
> item, you can use the `HasWork<T>` trait to offset the pointer to the
> `work_struct` field. The function pointer in the `work_struct` field is
> guaranteed to be a function that knows what the original pointer type
> was, and using that information, it can undo the offset operation by
> looking up what the offset was via the `HasWork<T>` trait.
>
> This design supports work items with multiple `work_struct` fields by
> using different pointer types. For example, you might define structs
> like these:
>
> ```
> struct MyPointer1(Arc<MyWorkItem>);
> struct MyPointer2(Arc<MyWorkItem>);
>
> struct MyWorkItem {
>     work1: Work<MyPointer1>,
>     work2: Work<MyPointer2>,
> }
> ```
>
> Then, the wrapper structs `MyPointer1` and `MyPointer2` will take the
> role as the pointer type. By using one or the other, you tell the
> workqueue which `work_struct` field to use. This pattern is called the
> "newtype" pattern.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/helpers.c           |   8 ++
>  rust/kernel/workqueue.rs | 183 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 190 insertions(+), 1 deletion(-)
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 81e80261d597..7f0c2fe2fbeb 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -26,6 +26,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/sched/signal.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  
>  __noreturn void rust_helper_BUG(void)
>  {
> @@ -128,6 +129,13 @@ void rust_helper_put_task_struct(struct task_struct *t)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
>  
> +void rust_helper___INIT_WORK(struct work_struct *work, work_func_t func,
> +			     bool on_stack)
> +{
> +	__INIT_WORK(work, func, on_stack);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper___INIT_WORK);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 22205d3bda72..7509618af252 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -4,7 +4,8 @@
>  //!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
>  
> -use crate::{bindings, types::Opaque};
> +use crate::{bindings, prelude::*, types::Opaque};
> +use core::marker::{PhantomData, PhantomPinned};
>  
>  /// A kernel work queue.
>  ///
> @@ -98,6 +99,186 @@ pub unsafe trait WorkItem {
>          F: FnOnce(*mut bindings::work_struct) -> bool;
>  }
>  
> +/// Defines the method that should be called when a work item is executed.
> +///
> +/// 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`]: WorkItem::__enqueue
> +/// [`run`]: WorkItemAdapter::run
> +pub unsafe trait WorkItemAdapter: WorkItem {
> +    /// Run this work item.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Must only be called via the function pointer that [`__enqueue`] provides to the
> +    /// `queue_work_on` closure, and only as described in the documentation of `queue_work_on`.
> +    ///
> +    /// [`__enqueue`]: WorkItem::__enqueue
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
> +}
> +
> +/// Links for a work item.
> +///
> +/// This struct contains a function pointer to the `T::run` function from the [`WorkItemAdapter`]
> +/// 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 [`WorkItemAdapter`] that uses
> +/// it.
> +#[repr(transparent)]
> +pub struct Work<T: ?Sized> {
> +    work: Opaque<bindings::work_struct>,
> +    _pin: PhantomPinned,
> +    _adapter: 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> Send for Work<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> Sync for Work<T> {}
> +
> +impl<T: ?Sized> Work<T> {
> +    /// Creates a new instance of [`Work`].
> +    #[inline]
> +    #[allow(clippy::new_ret_no_self)]
> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: WorkItemAdapter,
> +    {
> +        // SAFETY: The `WorkItemAdapter` implementation promises that `T::run` can be used as the
> +        // work item function.
> +        unsafe {
> +            kernel::init::pin_init_from_closure(move |slot| {
> +                bindings::__INIT_WORK(Self::raw_get(slot), Some(T::run), false);
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Get a pointer to the inner `work_struct`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The provided pointer must not be dangling. (But it 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 valid.
> +        //
> +        // A pointer cast would also be ok due to `#[repr(transparent)]`. We use `addr_of!` so that
> +        // the compiler does not complain that `work` is unused.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).work)) }
> +    }
> +}
> +
> +/// Declares that a type has a [`Work<T>`] field.
> +///
> +/// # Safety
> +///
> +/// The [`OFFSET`] constant must be the offset of a field in Self of type [`Work<T>`]. The methods on
> +/// this trait must have exactly the behavior that the definitions given below have.
> +///
> +/// [`Work<T>`]: Work
> +/// [`OFFSET`]: HasWork::OFFSET
> +pub unsafe trait HasWork<T> {
> +    /// The offset of the [`Work<T>`] field.
> +    ///
> +    /// [`Work<T>`]: Work
> +    const OFFSET: usize;
> +
> +    /// Returns the offset of the [`Work<T>`] field.
> +    ///
> +    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not Sized.
> +    ///
> +    /// [`Work<T>`]: Work
> +    /// [`OFFSET`]: HasWork::OFFSET
> +    #[inline]
> +    fn get_work_offset(&self) -> usize {
> +        Self::OFFSET
> +    }
> +
> +    /// Returns a pointer to the [`Work<T>`] field.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must not be dangling. (But the memory need not be initialized.)
> +    ///
> +    /// [`Work<T>`]: Work
> +    #[inline]
> +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T>
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: The caller promises that the pointer is not dangling.
> +        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T> }
> +    }
> +
> +    /// Returns a pointer to the struct containing the [`Work<T>`] field.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must not be dangling. (But the memory need not be initialized.)
> +    ///
> +    /// [`Work<T>`]: Work
> +    #[inline]
> +    unsafe fn work_container_of(ptr: *mut Work<T>) -> *mut Self
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: The caller promises that the pointer is not dangling.
> +        unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
> +    }
> +}
> +
> +/// Used to safely implement the [`HasWork<T>`] trait.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct MyStruct {
> +///     work_field: Work<Arc<MyStruct>>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
> +/// }
> +/// ```
> +///
> +/// [`HasWork<T>`]: HasWork
> +#[macro_export]
> +macro_rules! impl_has_work {
> +    ($(impl$(<$($implarg:ident),*>)?
> +       HasWork<$work_type:ty>
> +       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> for $self $(<$($selfarg),*>)? {
> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
> +
> +            #[inline]
> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
> +                }
> +            }

What is the reason for overriding the default implementation of `raw_get_work()`?

BR Andreas

> +        }
> +    )*};
> +}
> +
>  /// 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 v1 6/7] rust: workqueue: add safe API to workqueue

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> This commit introduces `ArcWorkItem`, `BoxWorkItem`, and
> `define_work_adapter_newtype!` that make it possible to use the
> workqueue without any unsafe code whatsoever.
>
> The `ArcWorkItem` and `BoxWorkItem` traits are used when a struct has a
> single `work_struct` field.
>
> The `define_work_adapter_newtype!` macro is used when a struct has
> multiple `work_struct` fields. For each `work_struct` field, a newtype
> struct is defined that wraps `Arc<TheStruct>`, and pushing an instance
> of the newtype to a workqueue will enqueue it using the associated
> `work_struct` field. The newtypes are matched with `work_struct` fields
> by having the T in `Work<T>` be the newtype.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/workqueue.rs | 332 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 331 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 7509618af252..007005ddcaf0 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -4,8 +4,9 @@
>  //!
>  //! C header: [`include/linux/workqueue.h`](../../../../include/linux/workqueue.h)
>  
> -use crate::{bindings, prelude::*, types::Opaque};
> +use crate::{bindings, prelude::*, sync::Arc, types::Opaque};
>  use core::marker::{PhantomData, PhantomPinned};
> +use core::result::Result;
>  
>  /// A kernel work queue.
>  ///
> @@ -279,6 +280,335 @@ macro_rules! impl_has_work {
>      )*};
>  }
>  
> +/// Declares that [`Arc<Self>`] should implement [`WorkItem`].
> +///
> +/// # Examples
> +///
> +/// The example below will make [`Arc<MyStruct>`] implement the [`WorkItem`] trait so that you can
> +/// enqueue it in a workqueue.
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct MyStruct {
> +///     work_field: Work<Arc<MyStruct>>,
> +/// }
> +///
> +/// kernel::impl_has_work! {
> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
> +/// }
> +///
> +/// impl ArcWorkItem for MyStruct {
> +///     fn run(self: Arc<Self>) {
> +///         pr_info!("Executing MyStruct on a workqueue.");
> +///     }
> +/// }
> +/// ```
> +///
> +/// [`Arc<Self>`]: crate::sync::Arc
> +/// [`Arc<MyStruct>`]: crate::sync::Arc
> +pub trait ArcWorkItem {
> +    /// Called when this work item is executed.
> +    fn run(self: Arc<Self>);
> +}
> +
> +unsafe impl<T> WorkItem for Arc<T>
> +where
> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
> +{
> +    type EnqueueOutput = Result<(), Self>;
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        let ptr = Arc::into_raw(self);
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };

We have this functionality in the default impl of
`HasWork<T>::raw_get_work() where Self: Sized`. I am uncertain about the
`Sized` bound. If it is sound to do the offset calculation here where
`T: ?Sized`, it should also be sound in the default implementation of
`HasWork<T>`. Should we not be able to change the bound on
`HasWork<T>::raw_get_work()` to `Self: ?Sized` and call into that from
here?

        let work_ptr = unsafe { <T as HasWork<Self>>::raw_get_work(ptr as _) };

Same for Box.

BR Andreas

> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {
> +            true => Ok(()),
> +            // SAFETY: The work queue has not taken ownership of the pointer.
> +            false => Err(unsafe { Arc::from_raw(ptr) }),
> +        }
> +    }
> +}
> +
> +// Let `Work<Arc<T>>` be usable with types that are `ArcWorkItem`.
> +//
> +// We do not allow unsized types here. The `Work<Arc<T>>` field should always specify the actual
> +// concrete type stored in the `Arc`.
> +//
> +// SAFETY: The `Work<Arc<T>>` field must be initialized with this `run` method because the `Work`
> +// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
> +// same `Work<Arc<T>>` field because `HasWork` promises to always return the same field.
> +unsafe impl<T> WorkItemAdapter for Arc<T>
> +where
> +    T: ArcWorkItem + HasWork<Self> + Sized,
> +{
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
> +        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +        let ptr = ptr as *mut Work<Self>;
> +        // 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) };
> +
> +        arc.run();
> +    }
> +}
> +
> +/// Declares that [`Pin`]`<`[`Box`]`<Self>>` should implement [`WorkItem`].
> +///
> +/// # Examples
> +///
> +/// The example below will make [`Pin`]`<`[`Box`]`<MyStruct>>` implement the [`WorkItem`] trait so
> +/// that you can enqueue it in a workqueue.
> +///
> +/// ```
> +/// struct MyStruct {
> +///     work_field: Work<Pin<Box<MyStruct>>>,
> +/// }
> +///
> +/// kernel::impl_has_work! {
> +///     impl HasWork<Pin<Box<MyStruct>>> for MyStruct { self.work_field }
> +/// }
> +///
> +/// impl BoxWorkItem for MyStruct {
> +///     fn run(self: Pin<Box<MyStruct>>) {
> +///         pr_info!("Executing MyStruct on a workqueue.");
> +///     }
> +/// }
> +/// ```
> +///
> +/// [`Box`]: alloc::boxed::Box
> +/// [`Pin`]: core::pin::Pin
> +pub trait BoxWorkItem {
> +    /// Called when this work item is executed.
> +    fn run(self: Pin<Box<Self>>);
> +}
> +
> +unsafe impl<T> WorkItem for Pin<Box<T>>
> +where
> +    T: BoxWorkItem + HasWork<Self> + ?Sized,
> +{
> +    // When a box is in a workqueue, the workqueue has exclusive ownership of the box. Therefore,
> +    // it's not possible to enqueue a box while it is in a workqueue.
> +    type EnqueueOutput = ();
> +
> +    unsafe fn __enqueue<F>(self, queue_work_on: F)
> +    where
> +        F: FnOnce(*mut bindings::work_struct) -> bool,
> +    {
> +        // SAFETY: We will not used the contents in an unpinned manner.
> +        let ptr = unsafe { Box::into_raw(Pin::into_inner_unchecked(self)) };
> +
> +        // Using `get_work_offset` here for object-safety.
> +        //
> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
> +        let off = unsafe { (&*ptr).get_work_offset() };
> +
> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
> +        // `Work<Self>` in the same allocation.
> +        let work_ptr = unsafe { (ptr as *mut u8).add(off) as *mut Work<Self> };
> +        // SAFETY: The pointer is not dangling.
> +        let work_ptr = unsafe { Work::raw_get(work_ptr) };
> +
> +        match (queue_work_on)(work_ptr) {
> +            true => {}
> +            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +            // workqueue.
> +            false => unsafe { core::hint::unreachable_unchecked() },
> +        }
> +    }
> +}
> +
> +// Let `Work<Pin<Box<T>>>` be usable with types that are `BoxWorkItem`.
> +//
> +// We do not allow unsized types here. The `Work<Pin<Box<T>>>` field should always specify the actual
> +// concrete type stored in the `Box`.
> +//
> +// SAFETY: The `Work<Pin<Box<T>>>` field must be initialized with this run method because the `Work`
> +// struct prevents you from initializing it in any other way. The `__enqueue` trait uses the
> +// same `Work<Pin<Box<T>>>` field because `HasWork` promises to always return the same field.
> +unsafe impl<T> WorkItemAdapter for Pin<Box<T>>
> +where
> +    T: BoxWorkItem + HasWork<Self> + Sized,
> +{
> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
> +        // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +        let ptr = ptr as *mut Work<Self>;
> +        // 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 `Box::into_raw` and we've been given back ownership.
> +        // The box was originally pinned, so pinning it again is ok.
> +        let boxed = unsafe { Pin::new_unchecked(Box::from_raw(ptr)) };
> +
> +        boxed.run();
> +    }
> +}
> +
> +/// Helper macro for structs with several `Work` fields that can be in several queues at once.
> +///
> +/// For each `Work` field in your type `T`, a newtype struct that wraps an `Arc<T>` or
> +/// `Pin<Box<T>>` should be defined.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// struct MyStruct {
> +///     work1: Work<MyStructWork1>,
> +///     work2: Work<MyStructWork2>,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<MyStructWork1> for MyStruct { self.work1 }
> +///     impl HasWork<MyStructWork2> for MyStruct { self.work2 }
> +/// }
> +///
> +/// define_work_adapter_newtype! {
> +///     struct MyStructWork1(Arc<MyStruct>);
> +///     struct MyStructWork2(Arc<MyStruct>);
> +/// }
> +///
> +/// impl MyStructWork1 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +///
> +/// impl MyStructWork2 {
> +///     fn run(self) {
> +///         // ...
> +///     }
> +/// }
> +/// ```
> +///
> +/// This will let you push a `MyStructWork1(arc)` or `MyStructWork2(arc)` to a work queue. The [`Arc`]
> +/// can be in two work queues at the same time, and the `run` method on the wrapper type is called
> +/// when the work item is called.
> +///
> +/// [`Arc`]: crate::sync::Arc
> +#[macro_export]
> +macro_rules! define_work_adapter_newtype {
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Arc<$inner:ty> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub $crate::sync::Arc<$inner>);
> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ::core::result::Result<(), $name>;
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                let ptr = $crate::sync::Arc::into_raw(self.0);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Arc::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr.cast_mut()) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {
> +                    true => Ok(()),
> +                    // SAFETY: The work queue has not taken ownership of the pointer.
> +                    false => Err($name(unsafe { $crate::sync::Arc::from_raw(ptr) })),
> +                }
> +            }
> +        }
> +
> +        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
> +            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
> +                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
> +                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
> +                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
> +                // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
> +                let arc = unsafe { $crate::sync::Arc::from_raw(ptr) };
> +
> +                $name::run($name(arc));
> +            }
> +        }
> +
> +        define_work_adapter_newtype! { $($rest)* }
> +    };
> +
> +    (
> +        $(#[$outer:meta])*
> +        $pub:vis struct $name:ident(
> +            $(#[$innermeta:meta])*
> +            $fpub:vis Pin<Box<$inner:ty>> $(,)?
> +        );
> +        $($rest:tt)*
> +    ) => {
> +        $(#[$outer])*
> +        $pub struct $name($(#[$innermeta])* $fpub ::core::pin::Pin<::alloc::boxed::Box<$inner>>);
> +
> +        unsafe impl $crate::workqueue::WorkItem for $name {
> +            type EnqueueOutput = ();
> +
> +            unsafe fn __enqueue<F>(self, queue_work_on: F)
> +            where
> +                F: ::core::ops::FnOnce(*mut $crate::bindings::work_struct) -> bool,
> +            {
> +                // SAFETY: We will not used the contents in an unpinned manner.
> +                let boxed = unsafe { ::core::pin::Pin::into_inner_unchecked(self.0) };
> +                let ptr = ::alloc::boxed::Box::into_raw(boxed);
> +
> +                // SAFETY: The pointer is not dangling since we just got it from Box::into_raw.
> +                let work_ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::raw_get_work(ptr) };
> +
> +                // SAFETY: The pointer is not dangling.
> +                let work_ptr = unsafe { $crate::workqueue::Work::raw_get(work_ptr) };
> +
> +                match (queue_work_on)(work_ptr) {
> +                    true => {},
> +                    // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
> +                    // workqueue.
> +                    false => unsafe { ::core::hint::unreachable_unchecked() },
> +                }
> +            }
> +        }
> +
> +        unsafe impl $crate::workqueue::WorkItemAdapter for $name {
> +            unsafe extern "C" fn run(ptr: *mut $crate::bindings::work_struct) {
> +                // SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<Self>`.
> +                let ptr = ptr as *mut $crate::workqueue::Work<Self>;
> +                // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
> +                let ptr = unsafe { <$inner as $crate::workqueue::HasWork::<$name>>::work_container_of(ptr) };
> +                // SAFETY: This pointer comes from `Box::into_raw` and we've been given back ownership.
> +                let boxed = unsafe { ::alloc::boxed::Box::from_raw(ptr) };
> +                // SAFETY: The box was originally pinned, so pinning it again is ok.
> +                let boxed = unsafe { ::core::pin::Pin::new_unchecked(boxed) };
> +
> +                $name::run($name(boxed));
> +            }
> +        }
> +
> +        define_work_adapter_newtype! { $($rest)* }
> +    };
> +
> +    // After processing the last definition, we call ourselves with no input.
> +    () => {};
> +}
> +
>  /// 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 v1 6/7] rust: workqueue: add safe API to workqueue

[Martin Rodriguez Reboredo]

On 5/30/23 04:19, Andreas Hindborg wrote:> I prefer the first one, but both look OK to me. Is one more idiomatic
> than the other, or is it just a matter of personal preference?

Pedantic Clippy will deny a boolean match and I prefer if-else over it
because the latter it's explicitly purposed for boolean control flow.

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Miguel Ojeda]

On Tue, May 23, 2023 at 1:07 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> I think this is a question of style. For a comparison:
>
> match (queue_work_on)(work_ptr) {
>     true => Ok(()),
>     // SAFETY: The work queue has not taken ownership of the pointer.
>     false => Err(unsafe { Arc::from_raw(ptr) }),
> }
>
> vs
>
> 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) }),
> }

There is also the possibility of using the early return style:

    if ... {
        return Err(...);
    }

    Ok(())

This one makes it consistent with other early exits (i.e. whether at
the end of the function or not) and closer to the C side.

It is particularly nice when we are talking about errors, instead of
two "equal", non-error outcomes.

Cheers,
Miguel

Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work_struct fields

[Alice Ryhl]

Andreas Hindborg <nmi@metaspace.dk> writes:
> Alice Ryhl <aliceryhl@google.com> writes:
>> +/// Used to safely implement the [`HasWork<T>`] trait.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// use kernel::sync::Arc;
>> +///
>> +/// struct MyStruct {
>> +///     work_field: Work<Arc<MyStruct>>,
>> +/// }
>> +///
>> +/// impl_has_work! {
>> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
>> +/// }
>> +/// ```
>> +///
>> +/// [`HasWork<T>`]: HasWork
>> +#[macro_export]
>> +macro_rules! impl_has_work {
>> +    ($(impl$(<$($implarg:ident),*>)?
>> +       HasWork<$work_type:ty>
>> +       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> for $self $(<$($selfarg),*>)? {
>> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
>> +
>> +            #[inline]
>> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
>> +                // SAFETY: The caller promises that the pointer is not dangling.
>> +                unsafe {
>> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
>> +                }
>> +            }
> 
> What is the reason for overriding the default implementation of `raw_get_work()`?
> 
> BR Andreas

That's how the macro checks that the field actually has the type you
claim it has. If you lie about the type, then `raw_get_work` will not
compile. (See the safety comment on the impl block.)

Alice

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Alice Ryhl]

Benno Lossin <benno.lossin@proton.me> writes:
> On Wednesday, May 17th, 2023 at 22:31, Alice Ryhl <aliceryhl@google.com> wrote:
>> +macro_rules! define_work_adapter_newtype {
>> +    (
>> +        $(#[$outer:meta])*
>> +        $pub:vis struct $name:ident(
>> +            $(#[$innermeta:meta])*
>> +            $fpub:vis Arc<$inner:ty> $(,)?
>> +        );
>> +        $($rest:tt)*
>> +    ) => {
>> +        $(#[$outer])*
>> +        $pub struct $name($(#[$innermeta])* $fpub $crate::sync::Arc<$inner>);
> 
> I am a bit confused as to why these types *contain* a pointer. Shouldn't
> these be exactly the same `Work<$inner>`, except they allow multiple `run`
> functions? So IMO they should embed a `Work<$inner>` and the
> manually defined `run` function would take a `$inner`.

No, that's not how this particular patch was designed. With the design I
used, the way you tell `enqueue` which `work_struct` field you want to
use is by using a different pointer type for each `work_struct` field.
This macro defines those pointer types.

So, for example, if you have only one `work_struct` field, then you just
use `Arc<MyStruct>` as your pointer type, and the field has type
`Work<Arc<MyStruct>>`.

On the other hand, if you have two `work_struct` fields, then you
instead use the macro to define `MyPointerType1` and `MyPoinerType2`
that both wrap an `Arc<MyStruct>`, and the fields then have types
`Work<MyPointerType1>` and `Work<MyPointerType2>`.

Alice

Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work_struct fields

[Andreas Hindborg]

Alice Ryhl <aliceryhl@google.com> writes:

> Andreas Hindborg <nmi@metaspace.dk> writes:
>> Alice Ryhl <aliceryhl@google.com> writes:
>>> +/// Used to safely implement the [`HasWork<T>`] trait.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```
>>> +/// use kernel::sync::Arc;
>>> +///
>>> +/// struct MyStruct {
>>> +///     work_field: Work<Arc<MyStruct>>,
>>> +/// }
>>> +///
>>> +/// impl_has_work! {
>>> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
>>> +/// }
>>> +/// ```
>>> +///
>>> +/// [`HasWork<T>`]: HasWork
>>> +#[macro_export]
>>> +macro_rules! impl_has_work {
>>> +    ($(impl$(<$($implarg:ident),*>)?
>>> +       HasWork<$work_type:ty>
>>> +       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> for $self $(<$($selfarg),*>)? {
>>> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
>>> +
>>> +            #[inline]
>>> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
>>> +                // SAFETY: The caller promises that the pointer is not dangling.
>>> +                unsafe {
>>> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
>>> +                }
>>> +            }
>> 
>> What is the reason for overriding the default implementation of `raw_get_work()`?
>> 
>> BR Andreas
>
> That's how the macro checks that the field actually has the type you
> claim it has. If you lie about the type, then `raw_get_work` will not
> compile. (See the safety comment on the impl block.)

Got it 👍

I was thinking we could do the type check without redefining the method,
but that blows up complexity wise fast, since we need a trait to do it
to support `Self` in `$work_type`. It strikes me as a bit of a hack to
overwrite an otherwise fine implementation, but I guess it is the least
complex way.

Also I am a bit annoyed that we need to state the `$work_type` type at
all, since it is available in `work_field`. But I can see no way around
that.

BR Andreas

Re: [PATCH v1 4/7] rust: workqueue: define built-in queues

[Alice Ryhl]

Andreas Hindborg <nmi@metaspace.dk> writes:
> Alice Ryhl <aliceryhl@google.com> writes:
>> +/// Returns the system freezable work queue (`system_freezable_wq`).
>> +///
>> +/// It is equivalent to the one returned by [`system`] except that it's freezable.
> 
> Can we add a short definition of what "freezable" means?

I don't know what it means, but I would be happy to add an explanation
if you have one.

Alice

Re: [PATCH v1 7/7] rust: workqueue: add `try_spawn` helper method

[Alice Ryhl]

Benno Lossin <benno.lossin@proton.me> writes:
>> +    /// 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 + Fn()>(&self, func: T) -> Result {
> 
> Why is this `Fn()` instead of `FnOnce()`?

That's a mistake. Good catch. It will be fixed in the next version.

Alice

Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue

[Alice Ryhl]

Andreas Hindborg <nmi@metaspace.dk> writes:
> Alice Ryhl <aliceryhl@google.com> writes:
>> +unsafe impl<T> WorkItem for Arc<T>
>> +where
>> +    T: ArcWorkItem + HasWork<Self> + ?Sized,
>> +{
>> +    type EnqueueOutput = Result<(), Self>;
>> +
>> +    unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>> +    where
>> +        F: FnOnce(*mut bindings::work_struct) -> bool,
>> +    {
>> +        let ptr = Arc::into_raw(self);
>> +
>> +        // Using `get_work_offset` here for object-safety.
>> +        //
>> +        // SAFETY: The pointer is valid since we just got it from `into_raw`.
>> +        let off = unsafe { (&*ptr).get_work_offset() };
>> +
>> +        // SAFETY: The `HasWork` impl promises that this offset gives us a field of type
>> +        // `Work<Self>` in the same allocation.
>> +        let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> };
> 
> We have this functionality in the default impl of
> `HasWork<T>::raw_get_work() where Self: Sized`. I am uncertain about the
> `Sized` bound. If it is sound to do the offset calculation here where
> `T: ?Sized`, it should also be sound in the default implementation of
> `HasWork<T>`. Should we not be able to change the bound on
> `HasWork<T>::raw_get_work()` to `Self: ?Sized` and call into that from
> here?
> 
>         let work_ptr = unsafe { <T as HasWork<Self>>::raw_get_work(ptr as _) };
> 
> Same for Box.
> 
> BR Andreas

I looked into this, and it seems like we can remove `Self: Sized` bound
from `raw_get_work`, so I am able to simplify this code somewhat.
However, it cannot be removed from `container_of`.

Thanks,
Alice

Re: [PATCH v1 4/7] rust: workqueue: define built-in queues

[Andreas Hindborg (Samsung)]

Alice Ryhl <aliceryhl@google.com> writes:

> Andreas Hindborg <nmi@metaspace.dk> writes:
>> Alice Ryhl <aliceryhl@google.com> writes:
>>> +/// Returns the system freezable work queue (`system_freezable_wq`).
>>> +///
>>> +/// It is equivalent to the one returned by [`system`] except that it's freezable.
>> 
>> Can we add a short definition of what "freezable" means?
>
> I don't know what it means, but I would be happy to add an explanation
> if you have one.

From https://docs.kernel.org/core-api/workqueue.html:

"A freezable wq participates in the freeze phase of the system suspend
operations. Work items on the wq are drained and no new work item starts
execution until thawed."

BR Andreas