Message-ID: <59515c1e-d1f4-47c3-a201-d2b0824f948b@proton.me>
Date: Fri, 26 Jul 2024 07:40:59 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org
Cc: Danilo Krummrich <dakr@...hat.com>, airlied@...hat.com, Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, Valentin Obst <kernel@...entinobst.de>, Trevor Gross <tmgross@...ch.edu>, Ben Gooding <ben.gooding.dev@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: sync: Introduce LockContainer trait

On 26.07.24 00:27, Lyude Paul wrote:
> We want to be able to use spinlocks in no-interrupt contexts, but our
> current `Lock` infrastructure doesn't allow for the ability to pass
> arguments when acquiring a lock - meaning that there would be no way for us
> to verify interrupts are disabled before granting a lock since we have
> nowhere to pass an `IrqGuard`.
> 
> It doesn't particularly made sense for us to add the ability to pass such
> an argument either: this would technically work, but then we would have to
> pass empty units as arguments on all of the many locks that are not grabbed
> under interrupts. As a result, we go with a slightly nicer solution:

I think there is a solution that would allow us to have both[1]:
1. Add a new associated type to `Backend` called `Context`.
2. Add a new parameter to `Backend::lock`: `ctx: Self::Context`.
3. Add a new function to `Lock<T: ?Sized, B: Backend>`:
   `lock_with(&self, ctx: B::Context)` that delegates to `B::lock`.
4. Reimplement `Lock::lock` in terms of `Lock::lock_with`, by
   constraining the function to only be callable if
   `B::Context: Default` holds (and then using `Default::default()` as
   the value).

This way people can still use `lock()` as usual, but we can also have
`lock_with(irq)` for locks that require it.

[1]: I think I saw this kind of a pattern first from Wedson in the
context of passing default allocation flags.

> introducing a trait for types which can contain a lock of a specific type:
> LockContainer. This means we can still use locks implemented on top of
> other lock types in types such as `LockedBy` - as we convert `LockedBy` to
> begin using `LockContainer` internally and implement the trait for all
> existing lock types.


> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  rust/kernel/sync.rs           |  1 +
>  rust/kernel/sync/lock.rs      | 20 ++++++++++++++++++++
>  rust/kernel/sync/locked_by.rs | 11 +++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 0ab20975a3b5d..14a79ebbb42d5 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -16,6 +16,7 @@
>  pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
>  pub use lock::mutex::{new_mutex, Mutex};
>  pub use lock::spinlock::{new_spinlock, SpinLock};
> +pub use lock::LockContainer;
>  pub use locked_by::LockedBy;
> 
>  /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index f6c34ca4d819f..bbd0a7465cae3 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -195,3 +195,23 @@ pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self {
>          }
>      }
>  }
> +
> +/// A trait implemented by any type which contains a [`Lock`] with a specific [`Backend`].
> +pub trait LockContainer<T: ?Sized, B: Backend> {
> +    /// Returns an immutable reference to the lock
> +    ///
> +    /// # Safety
> +    ///
> +    /// Since this returns a reference to the contained [`Lock`] without going through the
> +    /// [`LockContainer`] implementor, it cannot be guaranteed that it is safe to acquire
> +    /// this lock. Thus the caller must promise not to attempt to use the returned immutable
> +    /// reference to attempt to grab the underlying lock without ensuring whatever guarantees the
> +    /// [`LockContainer`] implementor's interface enforces.

This safety requirement is rather unclear to me, there isn't really a
good place to put the `LockContainer` requirements when implementing
this trait.
I also don't understand the use-case where a lock can only be acquired
in certain circumstances, do you have an example?

---
Cheers,
Benno

> +    unsafe fn get_lock_ref(&self) -> &Lock<T, B>;
> +}
> +
> +impl<T: ?Sized, B: Backend> LockContainer<T, B> for Lock<T, B> {
> +    unsafe fn get_lock_ref(&self) -> &Lock<T, B> {
> +        &self
> +    }
> +}
> diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
> index babc731bd5f62..d16d89fe74e0b 100644
> --- a/rust/kernel/sync/locked_by.rs
> +++ b/rust/kernel/sync/locked_by.rs
> @@ -95,13 +95,20 @@ impl<T, U> LockedBy<T, U> {
>      /// data becomes inaccessible; if another instance of the owner is allocated *on the same
>      /// memory location*, the data becomes accessible again: none of this affects memory safety
>      /// because in any case at most one thread (or CPU) can access the protected data at a time.
> -    pub fn new<B: Backend>(owner: &Lock<U, B>, data: T) -> Self {
> +    pub fn new<B, L>(owner: &L, data: T) -> Self
> +    where
> +        B: Backend,
> +        L: super::LockContainer<U, B>,
> +    {
>          build_assert!(
>              size_of::<Lock<U, B>>() > 0,
>              "The lock type cannot be a ZST because it may be impossible to distinguish instances"
>          );
>          Self {
> -            owner: owner.data.get(),
> +            // SAFETY: We never directly acquire the lock through this reference, we simply use it
> +            // to ensure that a `Guard` the user provides us to access this container's contents
> +            // belongs to the same lock that owns this data
> +            owner: unsafe { owner.get_lock_ref() }.data.get(),
>              data: UnsafeCell::new(data),
>          }
>      }
> --
> 2.45.2
> 

Powered by blists - more mailing lists