Message-ID: <CAH5fLgicAeLOnNUm-gMytYe5NmmMvwbWtXBUhWM5w7vY_BUnYw@mail.gmail.com>
Date: Thu, 12 Jun 2025 13:23:29 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <lossin@...nel.org>
Cc: 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, tmgross@...ch.edu, chrisi.schrefl@...il.com, 
	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 Thu, Jun 12, 2025 at 9:58 AM Benno Lossin <lossin@...nel.org> wrote:
>
> 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)?

If there's something incorrect in the docs, sure, let's fix it. But
since this is supposed to land as a part of a fix, perhaps it would be
best to have the work of making these docs perfect be a follow-up that
can land in the usual merge window?

Alice

Powered by blists - more mailing lists