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

[PATCH v2] 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.
Rust Binder will use this method together with the new shrinker abstractions
to avoid deadlocks in the memory shrinker.

Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
Signed-off-by: Filipe Xavier <felipe_life@live.com>
---
 rust/helpers/spinlock.c           |  5 +++++
 rust/kernel/sync/lock.rs          | 16 ++++++++++++++++
 rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
 rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
 4 files changed, 43 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..f4e51a5a1f23 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -58,6 +58,13 @@ unsafe fn init(
     #[must_use]
     unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
 
+    /// Tries to acquire the lock without blocking.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that [`Backend::init`] has been previously called.
+    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState>;
+
     /// Releases the lock, giving up its ownership.
     ///
     /// # Safety
@@ -128,6 +135,15 @@ pub fn lock(&self) -> Guard<'_, T, B> {
         // SAFETY: The lock was just acquired.
         unsafe { Guard::new(self, state) }
     }
+
+    /// Tries to acquire the lock and returns immediately
+    ///
+    /// 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 constructor of the type calls `init`, so the existence of the object proves
+        // that `init` was called.
+        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 v2] rust: add trylock method support for lock backend

[Boqun Feng]

On Sun, Sep 15, 2024 at 12:23:39PM -0300, Filipe Xavier wrote:
> Add a non-blocking trylock method to lock backend interface, mutex
> and spinlock implementations. It includes a C helper for spin_trylock.
> Rust Binder will use this method together with the new shrinker abstractions
> to avoid deadlocks in the memory shrinker.
> 

Getting better, thanks! Please see below:

> Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
> ---
>  rust/helpers/spinlock.c           |  5 +++++
>  rust/kernel/sync/lock.rs          | 16 ++++++++++++++++
>  rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
>  rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
>  4 files changed, 43 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..f4e51a5a1f23 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -58,6 +58,13 @@ unsafe fn init(
>      #[must_use]
>      unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
>  
> +    /// Tries to acquire the lock without blocking.

As I suggested in v1, "without blocking" is not accurate here because
a lock can be a spinlock. So you can just remove it. I think the word
"Tries" itself implies "neither busy waiting nor blocking".

> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that [`Backend::init`] has been previously called.
> +    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState>;
> +
>      /// Releases the lock, giving up its ownership.
>      ///
>      /// # Safety
> @@ -128,6 +135,15 @@ pub fn lock(&self) -> Guard<'_, T, B> {
>          // SAFETY: The lock was just acquired.
>          unsafe { Guard::new(self, state) }
>      }
> +
> +    /// Tries to acquire the lock and returns immediately

Please remove the "and returns immediately", as the meaning is vague: a
normal lock function also "returns immediately" after acquiring the
lock.

Otherwise, the patch looks good to me, thanks!

Regards,
Boqun

> +    ///
> +    /// 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 constructor of the type calls `init`, so the existence of the object proves
> +        // that `init` was called.
> +        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 v2] rust: add trylock method support for lock backend

[Waiman Long]

On 9/24/24 03:02, Boqun Feng wrote:
>> +
>> +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..f4e51a5a1f23 100644
>> --- a/rust/kernel/sync/lock.rs
>> +++ b/rust/kernel/sync/lock.rs
>> @@ -58,6 +58,13 @@ unsafe fn init(
>>       #[must_use]
>>       unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
>>   
>> +    /// Tries to acquire the lock without blocking.
> As I suggested in v1, "without blocking" is not accurate here because
> a lock can be a spinlock. So you can just remove it. I think the word
> "Tries" itself implies "neither busy waiting nor blocking".

Actually a spinlock in a PREEMPT_RT kernel is a sleeping lock. Not all 
people will associate "Tries" with not blocking. Anyway, I don't think 
it is a problem with the "without blocking" phrase.

Cheers,
Longman

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

[Boqun Feng]

On Tue, Sep 24, 2024 at 02:31:43PM -0400, Waiman Long wrote:
> On 9/24/24 03:02, Boqun Feng wrote:
> > > +
> > > +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..f4e51a5a1f23 100644
> > > --- a/rust/kernel/sync/lock.rs
> > > +++ b/rust/kernel/sync/lock.rs
> > > @@ -58,6 +58,13 @@ unsafe fn init(
> > >       #[must_use]
> > >       unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
> > > +    /// Tries to acquire the lock without blocking.
> > As I suggested in v1, "without blocking" is not accurate here because
> > a lock can be a spinlock. So you can just remove it. I think the word
> > "Tries" itself implies "neither busy waiting nor blocking".
> 
> Actually a spinlock in a PREEMPT_RT kernel is a sleeping lock. Not all

I don't see why a spinlock sometimes being a sleeping lock can justify
the inaccurate phrase "without blocking", it's still a spinning lock
when PREEMPT_RT=n, and function document should cover all the cases.

> people will associate "Tries" with not blocking. Anyway, I don't think it is
> a problem with the "without blocking" phrase.
> 

The problem is that for a generic Lock which includes spinlock and
mutex, "without blocking" is really inaccurate or redundant. For
spinlock, raw_spin_lock() also doesn't block, but it's not a valid
try_lock implementaiton. The phrase "without blocking" doesn't provide
useful or correct information to the users IMO. It should really be
something like "without waiting for the owner infinitely when
contention" if "Tries" is not an enough sign.

Regards,
Boqun

> Cheers,
> Longman
> 

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

[Waiman Long]

On 9/24/24 18:36, Boqun Feng wrote:
> On Tue, Sep 24, 2024 at 02:31:43PM -0400, Waiman Long wrote:
>> On 9/24/24 03:02, Boqun Feng wrote:
>>>> +
>>>> +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..f4e51a5a1f23 100644
>>>> --- a/rust/kernel/sync/lock.rs
>>>> +++ b/rust/kernel/sync/lock.rs
>>>> @@ -58,6 +58,13 @@ unsafe fn init(
>>>>        #[must_use]
>>>>        unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
>>>> +    /// Tries to acquire the lock without blocking.
>>> As I suggested in v1, "without blocking" is not accurate here because
>>> a lock can be a spinlock. So you can just remove it. I think the word
>>> "Tries" itself implies "neither busy waiting nor blocking".
>> Actually a spinlock in a PREEMPT_RT kernel is a sleeping lock. Not all
> I don't see why a spinlock sometimes being a sleeping lock can justify
> the inaccurate phrase "without blocking", it's still a spinning lock
> when PREEMPT_RT=n, and function document should cover all the cases.
>
>> people will associate "Tries" with not blocking. Anyway, I don't think it is
>> a problem with the "without blocking" phrase.
>>
> The problem is that for a generic Lock which includes spinlock and
> mutex, "without blocking" is really inaccurate or redundant. For
> spinlock, raw_spin_lock() also doesn't block, but it's not a valid
> try_lock implementaiton. The phrase "without blocking" doesn't provide
> useful or correct information to the users IMO. It should really be
> something like "without waiting for the owner infinitely when
> contention" if "Tries" is not an enough sign.

Right, if your focus is about no waiting, the phrase "without blocking" 
is insufficient. It should be "without blocking/waiting" or something 
similar. In that sense, just saying "without blocking" can be misleading.

Cheers,
Longman

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

[Gary Guo]

On Tue, 24 Sep 2024 00:02:46 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Sun, Sep 15, 2024 at 12:23:39PM -0300, Filipe Xavier wrote:
> > Add a non-blocking trylock method to lock backend interface, mutex
> > and spinlock implementations. It includes a C helper for spin_trylock.
> > Rust Binder will use this method together with the new shrinker abstractions
> > to avoid deadlocks in the memory shrinker.
> >   
> 
> Getting better, thanks! Please see below:
> 
> > Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> > Signed-off-by: Filipe Xavier <felipe_life@live.com>
> > ---
> >  rust/helpers/spinlock.c           |  5 +++++
> >  rust/kernel/sync/lock.rs          | 16 ++++++++++++++++
> >  rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
> >  rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
> >  4 files changed, 43 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..f4e51a5a1f23 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -58,6 +58,13 @@ unsafe fn init(
> >      #[must_use]
> >      unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
> >  
> > +    /// Tries to acquire the lock without blocking.  
> 
> As I suggested in v1, "without blocking" is not accurate here because
> a lock can be a spinlock. So you can just remove it. I think the word
> "Tries" itself implies "neither busy waiting nor blocking".

It seems that mutex_trylock contains a loop, so it's not
exactly wait-free.

Maybe `blocking` is indeed the correct word here?

Best,
Gary

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

[Boqun Feng]

On Tue, Sep 24, 2024 at 09:24:15PM +0100, Gary Guo wrote:
> On Tue, 24 Sep 2024 00:02:46 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Sun, Sep 15, 2024 at 12:23:39PM -0300, Filipe Xavier wrote:
> > > Add a non-blocking trylock method to lock backend interface, mutex
> > > and spinlock implementations. It includes a C helper for spin_trylock.
> > > Rust Binder will use this method together with the new shrinker abstractions
> > > to avoid deadlocks in the memory shrinker.
> > >   
> > 
> > Getting better, thanks! Please see below:
> > 
> > > Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> > > Signed-off-by: Filipe Xavier <felipe_life@live.com>
> > > ---
> > >  rust/helpers/spinlock.c           |  5 +++++
> > >  rust/kernel/sync/lock.rs          | 16 ++++++++++++++++
> > >  rust/kernel/sync/lock/mutex.rs    | 11 +++++++++++
> > >  rust/kernel/sync/lock/spinlock.rs | 11 +++++++++++
> > >  4 files changed, 43 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..f4e51a5a1f23 100644
> > > --- a/rust/kernel/sync/lock.rs
> > > +++ b/rust/kernel/sync/lock.rs
> > > @@ -58,6 +58,13 @@ unsafe fn init(
> > >      #[must_use]
> > >      unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState;
> > >  
> > > +    /// Tries to acquire the lock without blocking.  
> > 
> > As I suggested in v1, "without blocking" is not accurate here because
> > a lock can be a spinlock. So you can just remove it. I think the word
> > "Tries" itself implies "neither busy waiting nor blocking".
> 
> It seems that mutex_trylock contains a loop, so it's not
> exactly wait-free.
> 
> Maybe `blocking` is indeed the correct word here?
> 

Function document should be helpful and general as much as possible.
The cases where try_lock() is needed are mostly (also according to the
commit log) avoiding waiting for the owners infinitely. So when users
call this function, instead of "blocking", the thing they care most is
whether this function would wait for the exising owner (if any)
infinitely. And I think mention the behavior around "blocking" is at
least unuseful maybe also misleading.

Perhaps I made a mistake by calling "without blocking" "not accurate",
but my read of "without blocking" is "otherwise (when using the other
API, e.g. lock()) it might block", which is not the case for real
spinlock at least.

Does this make sense?

Regards,
Boqun

> Best,
> Gary