* [PATCH v2] rust: add trylock method support for lock backend
@ 2024-09-15 15:23 Filipe Xavier
2024-09-24 7:02 ` Boqun Feng
0 siblings, 1 reply; 7+ messages in thread
From: Filipe Xavier @ 2024-09-15 15:23 UTC (permalink / raw)
To: aliceryhl, ojeda, boqun.feng, gary, benno.lossin, will, longman
Cc: rust-for-linux, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-15 15:23 [PATCH v2] rust: add trylock method support for lock backend Filipe Xavier
@ 2024-09-24 7:02 ` Boqun Feng
2024-09-24 18:31 ` Waiman Long
2024-09-24 20:24 ` Gary Guo
0 siblings, 2 replies; 7+ messages in thread
From: Boqun Feng @ 2024-09-24 7:02 UTC (permalink / raw)
To: Filipe Xavier
Cc: aliceryhl, ojeda, gary, benno.lossin, will, longman,
rust-for-linux
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-24 7:02 ` Boqun Feng
@ 2024-09-24 18:31 ` Waiman Long
2024-09-24 22:36 ` Boqun Feng
2024-09-24 20:24 ` Gary Guo
1 sibling, 1 reply; 7+ messages in thread
From: Waiman Long @ 2024-09-24 18:31 UTC (permalink / raw)
To: Boqun Feng, Filipe Xavier
Cc: aliceryhl, ojeda, gary, benno.lossin, will, rust-for-linux
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-24 7:02 ` Boqun Feng
2024-09-24 18:31 ` Waiman Long
@ 2024-09-24 20:24 ` Gary Guo
2024-09-24 23:33 ` Boqun Feng
1 sibling, 1 reply; 7+ messages in thread
From: Gary Guo @ 2024-09-24 20:24 UTC (permalink / raw)
To: Boqun Feng
Cc: Filipe Xavier, aliceryhl, ojeda, benno.lossin, will, longman,
rust-for-linux
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-24 18:31 ` Waiman Long
@ 2024-09-24 22:36 ` Boqun Feng
2024-09-25 1:27 ` Waiman Long
0 siblings, 1 reply; 7+ messages in thread
From: Boqun Feng @ 2024-09-24 22:36 UTC (permalink / raw)
To: Waiman Long
Cc: Filipe Xavier, aliceryhl, ojeda, gary, benno.lossin, will,
rust-for-linux
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-24 20:24 ` Gary Guo
@ 2024-09-24 23:33 ` Boqun Feng
0 siblings, 0 replies; 7+ messages in thread
From: Boqun Feng @ 2024-09-24 23:33 UTC (permalink / raw)
To: Gary Guo
Cc: Filipe Xavier, aliceryhl, ojeda, benno.lossin, will, longman,
rust-for-linux
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] rust: add trylock method support for lock backend
2024-09-24 22:36 ` Boqun Feng
@ 2024-09-25 1:27 ` Waiman Long
0 siblings, 0 replies; 7+ messages in thread
From: Waiman Long @ 2024-09-25 1:27 UTC (permalink / raw)
To: Boqun Feng
Cc: Filipe Xavier, aliceryhl, ojeda, gary, benno.lossin, will,
rust-for-linux
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-25 1:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-15 15:23 [PATCH v2] rust: add trylock method support for lock backend Filipe Xavier
2024-09-24 7:02 ` Boqun Feng
2024-09-24 18:31 ` Waiman Long
2024-09-24 22:36 ` Boqun Feng
2024-09-25 1:27 ` Waiman Long
2024-09-24 20:24 ` Gary Guo
2024-09-24 23:33 ` Boqun Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox