Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 24.02.25 09:08, Andreas Hindborg wrote:
>> Boqun Feng <boqun.feng@gmail.com> writes:
>>
>>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>>> test, an "assert a lock is held by a guard" example is added.
>>>
>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>> ---
>>> This depends on Alice's patch:
>>>
>>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>>
>>> I'm also OK to fold this in if Alice thinks it's fine.
>>>
>>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>> index 3701fac6ebf6..6d868e35b0a3 100644
>>> --- a/rust/kernel/sync/lock.rs
>>> +++ b/rust/kernel/sync/lock.rs
>>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>>
>>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>>      /// Returns the lock that this guard originates from.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>>> +    /// lock is held.
>>> +    ///
>>> +    /// ```
>>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>>> +    ///
>>> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
>>> +    ///     // Address-equal means the same lock.
>>> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
>>> +    /// }
>>
>> This seems super useful. Perhaps add this method as part of the lock api
>> instead of just having it in the example?
>
> I don't think it should be an assert. Instead make it return a
> `Result<(), ()>`. (or create better named unit error types)

No, this should not be part of usual control flow, and developers should
not make control flow decisions based on this. It would always be an
assertion. But you are right that `assert!` is probably not what we
want. `debug_assert!` might be fine though.


Best regards,
Andreas Hindborg