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

[Waiman Long]

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@xxxxxxxxxxx>
> > ---
> >   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