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

[Waiman Long <longman@redhat.com>]

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