[PATCH] rust: add trylock method support for lock backend

[PATCH] rust: add trylock method support for lock backend

[Filipe Xavier]

Add a non-blocking trylock method to lock backend interface, mutex
and spinlock implementations. It includes a C helper for spin_trylock.


Signed-off-by: Filipe Xavier <felipe_life@live.com>
---
 rust/helpers/spinlock.c           |  5 +++++
 rust/kernel/sync/lock.rs          | 15 +++++++++++++++
 rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
 rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
 4 files changed, 42 insertions(+)

diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
index acc1376b833c..775ed4d549ae 100644
--- a/rust/helpers/spinlock.c
+++ b/rust/helpers/spinlock.c
@@ -22,3 +22,8 @@ void rust_helper_spin_unlock(spinlock_t *lock)
 {
 	spin_unlock(lock);
 }
+
+int rust_helper_spin_trylock(spinlock_t *lock)
+{
+	return spin_trylock(lock);
+}
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f6c34ca4d819..c13595a735ae 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -27,6 +27,7 @@
 ///
 /// [`lock`]: Backend::lock
 /// [`unlock`]: Backend::unlock
+/// [`try_lock`]: Backend::try_lock
 /// [`relock`]: Backend::relock
 pub unsafe trait Backend {
     /// The state required by the lock.
@@ -65,6 +66,13 @@ unsafe fn init(
     /// It must only be called by the current owner of the lock.
     unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
 
+    /// Tries to acquire the mutex without blocking.
+    ///
+    /// # Safety
+    ///
+    /// Returns `Some(state)` if successful, `None` if already locked.
+    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState>;
+
     /// Reacquires the lock, making the caller its owner.
     ///
     /// # Safety
@@ -128,6 +136,13 @@ pub fn lock(&self) -> Guard<'_, T, B> {
         // SAFETY: The lock was just acquired.
         unsafe { Guard::new(self, state) }
     }
+
+    /// Tries to acquire the lock without blocking and returns a guard that can be used
+    /// to access the data protected by the lock if successful.
+    pub fn try_lock(&self) -> Option<Guard<'_, T, B>> {
+        // SAFETY: The backend `try_lock` method has been implemented securely.
+        unsafe { B::try_lock(self.state.get()).map(|state| Guard::new(self, state)) }
+    }
 }
 
 /// A lock guard.
diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs
index 30632070ee67..c4f3b6cbfe48 100644
--- a/rust/kernel/sync/lock/mutex.rs
+++ b/rust/kernel/sync/lock/mutex.rs
@@ -115,4 +115,15 @@ unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
         // caller is the owner of the mutex.
         unsafe { bindings::mutex_unlock(ptr) };
     }
+
+    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState> {
+        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
+        let result = unsafe { bindings::mutex_trylock(ptr) };
+
+        if result != 0 {
+            Some(())
+        } else {
+            None
+        }
+    }
 }
diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
index ea5c5bc1ce12..c900ae23db76 100644
--- a/rust/kernel/sync/lock/spinlock.rs
+++ b/rust/kernel/sync/lock/spinlock.rs
@@ -114,4 +114,15 @@ unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
         // caller is the owner of the spinlock.
         unsafe { bindings::spin_unlock(ptr) }
     }
+
+    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState> {
+        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
+        let result = unsafe { bindings::spin_trylock(ptr) };
+
+        if result != 0 {
+            Some(())
+        } else {
+            None
+        }
+    }
 }
-- 
2.46.0

Re: [PATCH] rust: add trylock method support for lock backend

[Boqun Feng]

Hi Filipe,

Thanks for the patch.

On Sat, Sep 14, 2024 at 11:00:33AM -0300, Filipe Xavier wrote:
> 

^ unnecessary empty line here.

> Add a non-blocking trylock method to lock backend interface, mutex
> and spinlock implementations. It includes a C helper for spin_trylock.

The commit log needs to explain why the change is needed rather than
what the change is. Do you have a (potential) usage of these trylock
APIs? I know that trylock concept has been there for quite a while, but
still an usage will help us understand why this is needed in Rust code.

Also could you Cc lock maintainers next time, you can find the list of
their email address at MAINTAINERS file (there is a "LOCKING PRIMITIVES"
section).

v unnecessary empty line here as well.
> 
> 
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
> ---
>  rust/helpers/spinlock.c           |  5 +++++
>  rust/kernel/sync/lock.rs          | 15 +++++++++++++++
>  rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
>  rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> index acc1376b833c..775ed4d549ae 100644
> --- a/rust/helpers/spinlock.c
> +++ b/rust/helpers/spinlock.c
> @@ -22,3 +22,8 @@ void rust_helper_spin_unlock(spinlock_t *lock)
>  {
>  	spin_unlock(lock);
>  }
> +
> +int rust_helper_spin_trylock(spinlock_t *lock)

I'd just change this to `boolean`, but I don't whether it affects
LTO-inlining, Gary?

> +{
> +	return spin_trylock(lock);
> +}
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index f6c34ca4d819..c13595a735ae 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -27,6 +27,7 @@
>  ///
>  /// [`lock`]: Backend::lock
>  /// [`unlock`]: Backend::unlock
> +/// [`try_lock`]: Backend::try_lock

Other links are added because they are used in previous document
section, but this patch doesn't add the corresponding document for
`try_lock`.

>  /// [`relock`]: Backend::relock
>  pub unsafe trait Backend {
>      /// The state required by the lock.
> @@ -65,6 +66,13 @@ unsafe fn init(
>      /// It must only be called by the current owner of the lock.
>      unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState);
>  
> +    /// Tries to acquire the mutex without blocking.

Why use word "mutex" in a generic trait for all locks?

> +    ///
> +    /// # Safety
> +    ///
> +    /// Returns `Some(state)` if successful, `None` if already locked.

This is not a safety require for this function.

> +    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState>;
> +
>      /// Reacquires the lock, making the caller its owner.
>      ///
>      /// # Safety
> @@ -128,6 +136,13 @@ pub fn lock(&self) -> Guard<'_, T, B> {
>          // SAFETY: The lock was just acquired.
>          unsafe { Guard::new(self, state) }
>      }
> +
> +    /// Tries to acquire the lock without blocking and returns a guard that can be used

I'd make a new paragraph for the second part of the second (i..e "and
returns ..."). "Tries to acquire the lock without blocking" is good
enough to describe what the function is (although maybe "without
blocking" can be also dropped, because "blocking" can mean exactly
"sleep blocking" but that's not the case for spinlock), and the rest of
the sentence is really further describing the return behavior which
usually needs a separate paragraph.

> +    /// to access the data protected by the lock if successful.
> +    pub fn try_lock(&self) -> Option<Guard<'_, T, B>> {
> +        // SAFETY: The backend `try_lock` method has been implemented securely.

I didn't find any description on how `try_lock` should be implemented
securely, but yet you mention it here. Could you take the `lock()`
functions in `Backend` and `Lock` as an example, and see what you are
missing? You probably need a "safety" section for trait Backend.

Regards,
Boqun

> +        unsafe { B::try_lock(self.state.get()).map(|state| Guard::new(self, state)) }
> +    }
>  }
>  
>  /// A lock guard.
> diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs
> index 30632070ee67..c4f3b6cbfe48 100644
> --- a/rust/kernel/sync/lock/mutex.rs
> +++ b/rust/kernel/sync/lock/mutex.rs
> @@ -115,4 +115,15 @@ unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
>          // caller is the owner of the mutex.
>          unsafe { bindings::mutex_unlock(ptr) };
>      }
> +
> +    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState> {
> +        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
> +        let result = unsafe { bindings::mutex_trylock(ptr) };
> +
> +        if result != 0 {
> +            Some(())
> +        } else {
> +            None
> +        }
> +    }
>  }
> diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> index ea5c5bc1ce12..c900ae23db76 100644
> --- a/rust/kernel/sync/lock/spinlock.rs
> +++ b/rust/kernel/sync/lock/spinlock.rs
> @@ -114,4 +114,15 @@ unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
>          // caller is the owner of the spinlock.
>          unsafe { bindings::spin_unlock(ptr) }
>      }
> +
> +    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState> {
> +        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
> +        let result = unsafe { bindings::spin_trylock(ptr) };
> +
> +        if result != 0 {
> +            Some(())
> +        } else {
> +            None
> +        }
> +    }
>  }
> -- 
> 2.46.0
> 

Re: [PATCH] rust: add trylock method support for lock backend

[Alice Ryhl]

On Sun, Sep 15, 2024 at 7:35 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hi Filipe,
>
> Thanks for the patch.
>
> On Sat, Sep 14, 2024 at 11:00:33AM -0300, Filipe Xavier wrote:
> >
>
> ^ unnecessary empty line here.
>
> > Add a non-blocking trylock method to lock backend interface, mutex
> > and spinlock implementations. It includes a C helper for spin_trylock.
>
> The commit log needs to explain why the change is needed rather than
> what the change is. Do you have a (potential) usage of these trylock
> APIs? I know that trylock concept has been there for quite a while, but
> still an usage will help us understand why this is needed in Rust code.

Filipe, you can explain in your commit message that Rust Binder will
use this together with the new shrinker abstractions [1] to avoid
deadlocks in the memory shrinker.

Alice

[1]: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com/

Re: [PATCH] rust: add trylock method support for lock backend

[Gary Guo]

On Sat, 14 Sep 2024 22:35:29 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> > diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> > index acc1376b833c..775ed4d549ae 100644
> > --- a/rust/helpers/spinlock.c
> > +++ b/rust/helpers/spinlock.c
> > @@ -22,3 +22,8 @@ void rust_helper_spin_unlock(spinlock_t *lock)
> >  {
> >  	spin_unlock(lock);
> >  }
> > +
> > +int rust_helper_spin_trylock(spinlock_t *lock)  
> 
> I'd just change this to `boolean`, but I don't whether it affects
> LTO-inlining, Gary?
> 

I don't see why it would affect LTO inlining. The additional cast
should be easily optimized away.

However I'd be wary of the signature going out-of-sync compared to rest
of C. If we later use bindgen's automatic helper generation or if the C
side made spin_trylock an externed function, then we would have issue
(compilation issue, but still...)

I think we should stick with `int` unless we change the signature of the
static inline functionn to also return `bool`.

Best,
Gary


> > +{
> > +	return spin_trylock(lock);
> > +}

Re: [PATCH] rust: add trylock method support for lock backend

[Miguel Ojeda]

On Sun, Sep 15, 2024 at 2:16 PM Gary Guo <gary@garyguo.net> wrote:
>
> I think we should stick with `int` unless we change the signature of the
> static inline functionn to also return `bool`.

Agreed, I think it is best to keep them in sync, either not changing
it or changing it in both sides.

Cheers,
Miguel