* Re: [PATCH v3] rust: add trylock method support for lock backend
2024-09-26 20:50 [PATCH v3] rust: add trylock method support for lock backend Filipe Xavier
@ 2024-09-27 8:03 ` Alice Ryhl
2024-09-27 8:24 ` Boqun Feng
2024-09-27 8:28 ` Boqun Feng
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Alice Ryhl @ 2024-09-27 8:03 UTC (permalink / raw)
To: Filipe Xavier
Cc: ojeda, boqun.feng, gary, benno.lossin, will, longman,
rust-for-linux
On Thu, Sep 26, 2024 at 10:50 PM Filipe Xavier <felipe_life@live.com> 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.
>
> Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
I guess there is the question of whether it should be called trylock
or try_lock. One matches the kernel, the other matches the Rust
standard library.
Either way:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] rust: add trylock method support for lock backend
2024-09-27 8:03 ` Alice Ryhl
@ 2024-09-27 8:24 ` Boqun Feng
0 siblings, 0 replies; 6+ messages in thread
From: Boqun Feng @ 2024-09-27 8:24 UTC (permalink / raw)
To: Alice Ryhl
Cc: Filipe Xavier, ojeda, gary, benno.lossin, will, longman,
rust-for-linux
On Fri, Sep 27, 2024 at 10:03:43AM +0200, Alice Ryhl wrote:
> On Thu, Sep 26, 2024 at 10:50 PM Filipe Xavier <felipe_life@live.com> 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.
> >
> > Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> > Signed-off-by: Filipe Xavier <felipe_life@live.com>
>
> I guess there is the question of whether it should be called trylock
> or try_lock. One matches the kernel, the other matches the Rust
> standard library.
>
I prefer try_lock, not because it matches the Rust standard library, but
because it matches with other try-like APIs.
Regards,
Boqun
> Either way:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rust: add trylock method support for lock backend
2024-09-26 20:50 [PATCH v3] rust: add trylock method support for lock backend Filipe Xavier
2024-09-27 8:03 ` Alice Ryhl
@ 2024-09-27 8:28 ` Boqun Feng
2024-09-27 9:07 ` Fiona Behrens
2024-10-05 22:05 ` Miguel Ojeda
3 siblings, 0 replies; 6+ messages in thread
From: Boqun Feng @ 2024-09-27 8:28 UTC (permalink / raw)
To: Filipe Xavier
Cc: aliceryhl, ojeda, gary, benno.lossin, will, longman,
rust-for-linux
On Thu, Sep 26, 2024 at 05:50:37PM -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.
>
> Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Regards,
Boqun
> ---
> 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.
> + ///
> + /// # 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.
> + ///
> + /// 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] 6+ messages in thread* Re: [PATCH v3] rust: add trylock method support for lock backend
2024-09-26 20:50 [PATCH v3] rust: add trylock method support for lock backend Filipe Xavier
2024-09-27 8:03 ` Alice Ryhl
2024-09-27 8:28 ` Boqun Feng
@ 2024-09-27 9:07 ` Fiona Behrens
2024-10-05 22:05 ` Miguel Ojeda
3 siblings, 0 replies; 6+ messages in thread
From: Fiona Behrens @ 2024-09-27 9:07 UTC (permalink / raw)
To: Filipe Xavier
Cc: aliceryhl, ojeda, boqun.feng, gary, benno.lossin, will, longman,
rust-for-linux
On 26 Sep 2024, at 22:50, 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.
>
> Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
> ---
> 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.
> + ///
> + /// # 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.
> + ///
> + /// 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] 6+ messages in thread* Re: [PATCH v3] rust: add trylock method support for lock backend
2024-09-26 20:50 [PATCH v3] rust: add trylock method support for lock backend Filipe Xavier
` (2 preceding siblings ...)
2024-09-27 9:07 ` Fiona Behrens
@ 2024-10-05 22:05 ` Miguel Ojeda
3 siblings, 0 replies; 6+ messages in thread
From: Miguel Ojeda @ 2024-10-05 22:05 UTC (permalink / raw)
To: Filipe Xavier
Cc: aliceryhl, ojeda, boqun.feng, gary, benno.lossin, will, longman,
rust-for-linux
On Thu, Sep 26, 2024 at 10:50 PM Filipe Xavier <felipe_life@live.com> 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.
>
> Link: https://lore.kernel.org/all/20240912-shrinker-v1-1-18b7f1253553@google.com
> Signed-off-by: Filipe Xavier <felipe_life@live.com>
Applied to `rust-next` -- thanks everyone!
On the docs issue discussed in v2, it may still make sense in the
future to add a clarification note, i.e. rather than saying "without
...", we could give examples of potential/possible differing behavior
there and/or link to other docs or types.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 6+ messages in thread