Re: [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv_unhash

[Waiman Long <llong@redhat.com>]

On 12/24/24 10:15 PM, Akira Yokosawa wrote:
> Hi,
>
> [With my LKMM reviewer hat on]
>
> On Mon, 23 Dec 2024 15:47:04 +0800, Bibo Mao wrote:
>> We ported pv spinlock to old linux kernel on LoongArch platform, there is
>> error with some stress tests. The error report is something like this for
>> short:
>>   kernel BUG at kernel/locking/qspinlock_paravirt.h:261!
>>   Oops - BUG[#1]:
>>   CPU: 1 PID: 6613 Comm: pidof Not tainted 4.19.190+ #43
>>   Hardware name: Loongson KVM, BIOS 0.0.0 02/06/2015
>>      ra: 9000000000509cfc do_task_stat+0x29c/0xaf0
>>     ERA: 9000000000291308 __pv_queued_spin_unlock_slowpath+0xf8/0x100
>>    CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
>>    PRMD: 00000000 (PPLV0 -PIE -PWE)
>>           ...
>>   Call Trace:
>>   [<9000000000291308>] __pv_queued_spin_unlock_slowpath+0xf8/0x100
>>   [<9000000000509cf8>] do_task_stat+0x298/0xaf0
>>   [<9000000000502570>] proc_single_show+0x60/0xe0
>>
>> The problem is that memory accessing is out of order on LoongArch
>> platform, there is contension between pv_unhash() and pv_hash().
>>
>> CPU0 pv_unhash:                                CPU1 pv_hash:
>>
>>    for_each_hash_entry(he, offset, hash) {        for_each_hash_entry(he, offset, hash) {
>>      if (READ_ONCE(he->lock) == lock) {             struct qspinlock *old = NULL;
>>        node = READ_ONCE(he->node);
>>        WRITE_ONCE(he->lock, NULL);
>>
>> On LoongArch platform which is out of order, the execution order may be
>> switched like this:
>>>       WRITE_ONCE(he->lock, NULL);
>>                                                      if (try_cmpxchg(&he->lock, &old, lock)) {
>>                                                        WRITE_ONCE(he->node, node);
>>                                                        return &he->lock;
>>
>> CPU1 pv_hash() is executing and watch that lock is set with NULL. Write
>> he->node with node of new lock.
>>>       node = READ_ONCE(he->node);
>> READ_ONCE(he->node) on CPU0 will return node of new lock rather than itself.
>>
>> Here READ_ONCE/WRITE_ONCE is replaced with try_cmpxchg().
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   kernel/locking/qspinlock_paravirt.h | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
>> index dc1cb90e3644..cc4eabce092d 100644
>> --- a/kernel/locking/qspinlock_paravirt.h
>> +++ b/kernel/locking/qspinlock_paravirt.h
>> @@ -240,9 +240,10 @@ static struct pv_node *pv_unhash(struct qspinlock *lock)
>>   	struct pv_node *node;
>>   
>>   	for_each_hash_entry(he, offset, hash) {
>> -		if (READ_ONCE(he->lock) == lock) {
>> +		struct qspinlock *old = lock;
>> +
>> +		if (try_cmpxchg(&he->lock, &old, NULL))
>>   			node = READ_ONCE(he->node);
>> -			WRITE_ONCE(he->lock, NULL);
>>   			return node;
>>   		}
>>   	}
> But this change might delay load of he->node *after* he->lock returns to NULL.
>
> Let's get back to the current code (plus labeling ONCE accesses):
>
> 	for_each_hash_entry(he, offset, hash) {
> 		if (READ_ONCE(he->lock) == lock) {   /* A */
> 			node = READ_ONCE(he->node);  /* B */
> 			WRITE_ONCE(he->lock, NULL);  /* C */
> 			return node;
> 		}
> 	}
>
> It looks to me you want to guarantee the ordering of A -> B -> C.
>
> Your change effectively provides ordering of [A C] -> B.
> [A C] is done in an atomic RMW op.
>
> For the ordering of A -> B -> C, I'd change the code to
>
> 	for_each_hash_entry(he, offset, hash) {
> 		if (smp_load_acquire(&he->lock) == lock) {   /* A */
> 			node = READ_ONCE(he->node);          /* B */
> 			smp_store_release(&he->lock, NULL);  /* C */
> 			return node;
> 		}
> 	}
>
> Note: A -> B is load-to-load ordering, and it is not provided by
> control dependency.  Upgrading A to ACQUIRE is a lightweight option.

If Loongson CPU, like PowerPC, does not provide load-to-load ordering in 
a control dependency, the proper way is to provide its version of  
smp_acquire__after_ctrl_dep() helper and use it to provide the acquire 
barrier instead of using smp_load_acquire() here.

Cheers,
Longman