Re: [PATCH 1/3] rust: completion: implement initial abstraction

[Benno Lossin]

On Tue Jun 3, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
> Implement a minimal abstraction for the completion synchronization
> primitive.
>
> This initial abstraction only adds complete_all() and
> wait_for_completion(), since that is what is required for the subsequent
> Devres patch.
>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> Cc: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Ben Segall <bsegall@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: Valentin Schneider <vschneid@xxxxxxxxxx>
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>

I have a couple comments on the documentation, but the rest seems good.
So with those fixed:

Reviewed-by: Benno Lossin <lossin@xxxxxxxxxx>

> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/completion.c       |   8 +++
>  rust/helpers/helpers.c          |   1 +
>  rust/kernel/sync.rs             |   2 +
>  rust/kernel/sync/completion.rs  | 111 ++++++++++++++++++++++++++++++++
>  5 files changed, 123 insertions(+)
>  create mode 100644 rust/helpers/completion.c
>  create mode 100644 rust/kernel/sync/completion.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a5a6fb45d405..9da3fe89295c 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -17,6 +17,7 @@
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
>  #include <linux/clk.h>
> +#include <linux/completion.h>
>  #include <linux/configfs.h>
>  #include <linux/cpu.h>
>  #include <linux/cpufreq.h>
> diff --git a/rust/helpers/completion.c b/rust/helpers/completion.c
> new file mode 100644
> index 000000000000..b2443262a2ae
> --- /dev/null
> +++ b/rust/helpers/completion.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/completion.h>
> +
> +void rust_helper_init_completion(struct completion *x)
> +{
> +	init_completion(x);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 805307018f0e..7a5c520be8cb 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_assert.c"
>  #include "build_bug.c"
>  #include "clk.c"
> +#include "completion.c"
>  #include "cpufreq.c"
>  #include "cpumask.c"
>  #include "cred.c"
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 36a719015583..c23a12639924 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -10,6 +10,7 @@
>  use pin_init;
>  
>  mod arc;
> +pub mod completion;
>  mod condvar;
>  pub mod lock;
>  mod locked_by;
> @@ -17,6 +18,7 @@
>  pub mod rcu;
>  
>  pub use arc::{Arc, ArcBorrow, UniqueArc};
> +pub use completion::Completion;
>  pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
>  pub use lock::global::{global_lock, GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy};
>  pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
> diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs
> new file mode 100644
> index 000000000000..4ec4c2aa73a5
> --- /dev/null
> +++ b/rust/kernel/sync/completion.rs
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Completion support.
> +//!
> +//! Reference: <https://docs.kernel.org/scheduler/completion.html>
> +//!
> +//! C header: [`include/linux/completion.h`](srctree/include/linux/completion.h)
> +
> +use crate::{bindings, prelude::*, types::Opaque};
> +
> +/// Synchronization primitive to signal when a certain task has been completed.
> +///
> +/// The [`Completion`] synchronization primitive signales when a certain task has been completed by
> +/// waking up other tasks that can queue themselves up to wait for the [`Completion`] to be

s/can queue themselves/have been queued/

> +/// completed.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::{Arc, Completion};
> +/// use kernel::workqueue::{self, impl_has_work, new_work, Work, WorkItem};
> +///
> +/// #[pin_data]
> +/// struct MyTask {
> +///     #[pin]
> +///     work: Work<MyTask>,

Can we maybe add a dummy value like `Mutex<i32>` here that the task
changes, so we can print the value of it below (after waiting for the
task)?

> +///     #[pin]
> +///     done: Completion,
> +/// }
> +///
> +/// impl_has_work! {
> +///     impl HasWork<Self> for MyTask { self.work }
> +/// }
> +///
> +/// impl MyTask {
> +///     fn new() -> Result<Arc<Self>> {
> +///         let this = Arc::pin_init(pin_init!(MyTask {
> +///             work <- new_work!("MyTask::work"),
> +///             done <- Completion::new(),
> +///         }), GFP_KERNEL)?;
> +///
> +///         let _ = workqueue::system().enqueue(this.clone());
> +///
> +///         Ok(this)
> +///     }
> +///
> +///     fn wait_for_completion(&self) {
> +///         self.done.wait_for_completion();
> +///
> +///         pr_info!("Completion: task complete\n");
> +///     }
> +/// }
> +///
> +/// impl WorkItem for MyTask {
> +///     type Pointer = Arc<MyTask>;
> +///
> +///     fn run(this: Arc<MyTask>) {
> +///         // process this task
> +///         this.done.complete_all();
> +///     }
> +/// }
> +///
> +/// let task = MyTask::new()?;
> +/// task.wait_for_completion();
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[pin_data]
> +pub struct Completion {
> +    #[pin]
> +    inner: Opaque<bindings::completion>,
> +}
> +
> +impl Completion {
> +    /// Create an initializer for a new [`Completion`].
> +    pub fn new() -> impl PinInit<Self> {
> +        pin_init!(Self {
> +            inner <- Opaque::ffi_init(|slot: *mut bindings::completion| {
> +                // SAFETY: `slot` is a valid pointer to an uninitialized `struct completion`.
> +                unsafe { bindings::init_completion(slot) };
> +            }),
> +        })
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::completion {
> +        self.inner.get()
> +    }
> +
> +    /// Signal all tasks waiting on this completion.
> +    ///
> +    /// This method wakes up all tasks waiting on this completion; after this operation the
> +    /// completion is permanently done.
> +    pub fn complete_all(&self) {
> +        // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`.
> +        unsafe { bindings::complete_all(self.as_raw()) };
> +    }
> +
> +    /// Wait for completion of a task.
> +    ///
> +    /// This method waits for the completion of a task; it is not interruptible and there is no

I personally would write:

s/waits for/blocks on/

But if `wait` is the more common kernel term then let's go with your
version instead.

> +    /// timeout.

I would mention the `complete_all` method above.

> +    pub fn wait_for_completion(&self) {
> +        // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`.
> +        unsafe { bindings::wait_for_completion(self.as_raw()) };
> +    }
> +}
> +
> +// SAFETY: `Completion` is safe to be send to any task.
> +unsafe impl Send for Completion {}
> +
> +// SAFETY: `Completion` is safe to be accessed concurrently.
> +unsafe impl Sync for Completion {}

Please move these to the struct definition, that way one can more easily
see them when looking at the code.

---
Cheers,
Benno