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

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

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