On 15.02.2022 14:13, Julien Grall wrote: > On 15/02/2022 09:39, Roger Pau Monne wrote: >> There's no need to subtract _QR_BIAS from the lock value for storing >> in the local cnts variable in the read lock slow path: the users of >> the value in cnts only care about the writer-related bits and use a >> mask to get the value. >> >> Note that further setting of cnts in rspin_until_writer_unlock already >> do not subtract _QR_BIAS. > > The rwlock is a copy of the Linux implementation. So I looked at the > history to find out why _QR_BIAS was substracted. > > It looks like this was done to get better assembly on x86: > > commit f9852b74bec0117b888da39d070c323ea1cb7f4c > Author: Peter Zijlstra <pet...@infradead.org> > Date: Mon Apr 18 01:27:03 2016 +0200 > > locking/atomic, arch/qrwlock: Employ atomic_fetch_add_acquire() > > The only reason for the current code is to make GCC emit only the > "LOCK XADD" instruction on x86 (and not do a pointless extra ADD on > the result), do so nicer. > > Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org> > Acked-by: Waiman Long <waiman.l...@hpe.com> > Cc: Andrew Morton <a...@linux-foundation.org> > Cc: Linus Torvalds <torva...@linux-foundation.org> > Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com> > Cc: Peter Zijlstra <pet...@infradead.org> > Cc: Thomas Gleixner <t...@linutronix.de> > Cc: linux-a...@vger.kernel.org > Cc: linux-ker...@vger.kernel.org > Signed-off-by: Ingo Molnar <mi...@kernel.org> > > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c > index fec082338668..19248ddf37ce 100644 > --- a/kernel/locking/qrwlock.c > +++ b/kernel/locking/qrwlock.c > @@ -93,7 +93,7 @@ void queued_read_lock_slowpath(struct qrwlock *lock, > u32 cnts) > * that accesses can't leak upwards out of our subsequent critical > * section in the case that the lock is currently held for write. > */ > - cnts = atomic_add_return_acquire(_QR_BIAS, &lock->cnts) - _QR_BIAS; > + cnts = atomic_fetch_add_acquire(_QR_BIAS, &lock->cnts); > rspin_until_writer_unlock(lock, cnts); > > /* > > This is a slowpath, so probably not a concern. But I thought I would > double check whether the x86 folks are still happy to proceed with that > in mind.
Hmm, that's an interesting observation. Roger - did you inspect the generated code? At the very least the description may want amending. Jan
