On Tue, Jul 07, 2015 at 05:29:50PM -0400, Waiman Long wrote:
> On 07/07/2015 02:10 PM, Will Deacon wrote:
> >diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> >index deb9e8b0eb9e..be8dc5c6fdbd 100644
> >--- a/include/asm-generic/qrwlock.h
> >+++ b/include/asm-generic/qrwlock.h
> >@@ -27,7 +27,6 @@
> > /*
> > * Writer states& reader shift and bias
> > */
> >-#define _QW_WAITING 1 /* A writer is waiting */
> > #define _QW_LOCKED 0xff /* A writer holds the lock */
> > #define _QW_WMASK 0xff /* Writer mask */
> > #define _QR_SHIFT 8 /* Reader count shift */
> >diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> >index 9f644933f6d4..4006aa1fbd0b 100644
> >--- a/kernel/locking/qrwlock.c
> >+++ b/kernel/locking/qrwlock.c
> >@@ -127,28 +127,23 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
> > }
> >
> > /*
> >- * Set the waiting flag to notify readers that a writer is pending,
> >- * or wait for a previous writer to go away.
> >+ * Wait for a previous writer to go away, then set the locked
> >+ * flag to notify future readers/writers that we are pending.
> > */
> > for (;;) {
> > struct __qrwlock *l = (struct __qrwlock *)lock;
> >
> > if (!READ_ONCE(l->wmode)&&
> >- (cmpxchg(&l->wmode, 0, _QW_WAITING) == 0))
> >+ (cmpxchg(&l->wmode, 0, _QW_LOCKED) == 0))
> > break;
> >
> > cpu_relax_lowlatency();
> > }
> >
> >- /* When no more readers, set the locked flag */
> >- for (;;) {
> >- if ((atomic_read(&lock->cnts) == _QW_WAITING)&&
> >- (atomic_cmpxchg(&lock->cnts, _QW_WAITING,
> >- _QW_LOCKED) == _QW_WAITING))
> >- break;
> >-
> >+ /* Wait for the readers to drain */
> >+ while (smp_load_acquire((u32 *)&lock->cnts)& ~_QW_WMASK)
> > cpu_relax_lowlatency();
> >- }
> >+
> > unlock:
> > arch_spin_unlock(&lock->lock);
> > }
>
> That changes the handshaking protocol. In this case, the readers will have
> to decrement its reader count to enable the writer to continue.
It already needs to, no?
> The interrupt context reader code has to be changed.
Agreed.
> This gives preference to writer and reader will be in a disadvantage.
I don't see that, everybody is still ordered by the wait queue / lock.
> I prefer the current setting as you won't know if the writer has the
> lock or not when you take a snapshot of the value of the lock. You
> need the whole time sequence in this case to figure it out and so will
> be more prone to error.
I still need to wake up, but I suspect we need to change
queue_read_{try,}lock() to use cmpxchg/inc_not_zero like things, which
is fine for ARM, but not so much for x86.
So I think I agree with Waiman, but am willing to be shown differently.
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
