| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DAKE8OYKXUWH.1NRVGV5IKW7I9@kernel.org>
Date: Thu, 12 Jun 2025 09:58:30 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
<rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
<boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
<benno.lossin@...ton.me>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
<tmgross@...ch.edu>, <chrisi.schrefl@...il.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Ingo
Molnar" <mingo@...hat.com>, "Peter Zijlstra" <peterz@...radead.org>, "Juri
Lelli" <juri.lelli@...hat.com>, "Vincent Guittot"
<vincent.guittot@...aro.org>, "Dietmar Eggemann"
<dietmar.eggemann@....com>, "Steven Rostedt" <rostedt@...dmis.org>, "Ben
Segall" <bsegall@...gle.com>, "Mel Gorman" <mgorman@...e.de>, "Valentin
Schneider" <vschneid@...hat.com>
Subject: Re: [PATCH 1/3] rust: completion: implement initial abstraction
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@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Juri Lelli <juri.lelli@...hat.com>
> Cc: Vincent Guittot <vincent.guittot@...aro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@....com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Ben Segall <bsegall@...gle.com>
> Cc: Mel Gorman <mgorman@...e.de>
> Cc: Valentin Schneider <vschneid@...hat.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
I have a couple comments on the documentation, but the rest seems good.
So with those fixed:
Reviewed-by: Benno Lossin <lossin@...nel.org>
> ---
> 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
Powered by blists - more mailing lists