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

[Boqun Feng <boqun.feng@gmail.com>]

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
>