On Wed, May 03, 2017 at 06:59:19PM +0000, pan xinhui wrote:
> 在 2017/5/3 22:51, Yury Norov 写道:> The patch 3 adds implementation for
> queued-based locking on
> > ARM64, and the option in kernel config to enable it. Patches
> > 1 and 2 fix some mess in header files to apply patch 3 smoothly.
> >
> > Tested on QDF2400 with huge improvements with these patches on
> > the torture tests, by Adam Wallis.
> >
> > Tested on ThunderX, by Andrew Pinski:
> > 120 thread (30 core - 4 thread/core) CN99xx (single socket):
> >
> > benchmark Units qspinlocks vs ticket locks
> > sched/messaging s 73.91%
> > sched/pipe ops/s 104.18%
> > futex/hash ops/s 103.87%
> > futex/wake ms 71.04%
> > futex/wake-parallel ms 93.88%
> > futex/requeue ms 96.47%
> > futex/lock-pi ops/s 118.33%
> >
> > Notice, there's the queued locks implementation for the Power PC introduced
> > by Pan Xinhui. He largely tested it and also found significant performance
> > gain. In arch part it is very similar to this patch though.
> > https://lwn.net/Articles/701137/Hi, Yury
> Glad to know you will join locking development :)
> I have left IBM. However I still care about the queued-spinlock anyway.
>
> > RFC: https://www.spinics.net/lists/arm-kernel/msg575575.htmlI notice you
> > raised one question about the performance degradation in the acquisition of
> > rw-lock for read on qemu.
> This is strange indeed. I once enabled qrwlock on ppc too.
>
> I paste your test reseults below. Is this a result of
> qspinlock + qrwlock VS qspinlock + normal rwlock or
> qspinlock + qrwlock VS normal spinlock + normal rwlock?
Initially it was VS normal spinlock + normal rwlock. But now I checked
it vs qspinlock + normal rwlock, and results are the same. I don't think
it's a real use case to have ticket spinlocks and queued rwlocks, or
vice versa.
> I am not sure how that should happen.
Either me. If I understand it correctly, qemu is not suitable for measuring
performance, so I don't understand why slowing in qemu is important at all,
if real hardware works better. If it matters, my host CPU is Core i7-2630QM
> I make one RFC patch below(not based on latest kernel), you could apply it to
> check if ther is any performance improvement.
> The idea is that.
> In queued_write_lock_slowpath(), we did not unlock the ->wait_lock.
> Because the writer hold the rwlock, all readers are still waiting anyway.
> And in queued_read_lock_slowpath(), calling rspin_until_writer_unlock() looks
> like introduce a little overhead, say, spinning at the rwlock.
>
> But in the end, queued_read_lock_slowpath() is too heavy, compared with the
> normal rwlock. such result maybe is somehow reasonable?
I tried this path, but kernel hangs on boot with it, in
queued_write_lock_slowpath().
> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> index 54a8e65..28ee01d 100644
> --- a/include/asm-generic/qrwlock.h
> +++ b/include/asm-generic/qrwlock.h
> @@ -28,8 +28,9 @@
> * 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 _QW_KICK 0x80 /* need unlock the spinlock*/
> +#define _QW_LOCKED 0x7f /* A writer holds the lock */
> +#define _QW_WMASK 0x7f /* Writer mask */
> #define _QR_SHIFT 8 /* Reader count shift */
> #define _QR_BIAS (1U << _QR_SHIFT)
>
> @@ -139,7 +140,10 @@ static inline void queued_read_unlock(struct qrwlock
> *lock)
> */
> static inline void queued_write_unlock(struct qrwlock *lock)
> {
> - smp_store_release((u8 *)&lock->cnts, 0);
> + u32 v = atomic_read(&lock->cnts) & (_QW_WMASK | _QW_KICK);
> + if (v & _QW_KICK)
> + arch_spin_unlock(&lock->wait_lock);
> + (void)atomic_sub_return_release(v, &lock->cnts);
> }
>
> /*
> diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> index fec0823..1f0ea02 100644
> --- a/kernel/locking/qrwlock.c
> +++ b/kernel/locking/qrwlock.c
> @@ -116,7 +116,7 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
>
> /* Try to acquire the lock directly if no reader is present */
> if (!atomic_read(&lock->cnts) &&
> - (atomic_cmpxchg_acquire(&lock->cnts, 0, _QW_LOCKED) == 0))
> + (atomic_cmpxchg_acquire(&lock->cnts, 0, _QW_LOCKED|_QW_KICK) == 0))
> goto unlock;
>
> /*
> @@ -138,12 +138,13 @@ void queued_write_lock_slowpath(struct qrwlock *lock)
> cnts = atomic_read(&lock->cnts);
> if ((cnts == _QW_WAITING) &&
> (atomic_cmpxchg_acquire(&lock->cnts, _QW_WAITING,
> - _QW_LOCKED) == _QW_WAITING))
> + _QW_LOCKED|_QW_KICK) ==
> _QW_WAITING))
> break;
>
> cpu_relax_lowlatency();
It hangs in this in this loop. It's because lock->cnts may now contain
_QW_WAITING or _QW_WAITING | _QW_KICK. So the if() condition may never
meet in 2nd case. To handle it, I changed it like this:
for (;;) {
cnts = atomic_read(&lock->cnts);
if (((cnts & _QW_WMASK) == _QW_WAITING) &&
(atomic_cmpxchg_acquire(&lock->cnts, cnts,
_QW_LOCKED|_QW_KICK) == cnts))
break;
cpu_relax();
}
But after that it hanged in queued_spin_lock_slowpath() at the line
478 smp_cond_load_acquire(&lock->val.counter, !(VAL &
_Q_LOCKED_MASK));
Backtrace is below.
Yury
> }
> unlock:
> - arch_spin_unlock(&lock->wait_lock);
> + return;
> }
> EXPORT_SYMBOL(queued_write_lock_slowpath);
> --
> 2.4.11
#0 queued_spin_lock_slowpath (lock=0xffff000008cb051c <proc_subdir_lock+4>,
val=<optimized out>)
at kernel/locking/qspinlock.c:478
#1 0xffff0000080ff158 in queued_spin_lock (lock=<optimized out>)
at ./include/asm-generic/qspinlock.h:104
#2 queued_write_lock_slowpath (lock=0xffff000008cb0518 <proc_subdir_lock>)
at kernel/locking/qrwlock.c:116
#3 0xffff000008815fc4 in queued_write_lock (lock=<optimized out>)
at ./include/asm-generic/qrwlock.h:135
#4 __raw_write_lock (lock=<optimized out>) at
./include/linux/rwlock_api_smp.h:211
#5 _raw_write_lock (lock=<optimized out>) at kernel/locking/spinlock.c:295
#6 0xffff00000824c4c0 in proc_register (dir=0xffff000008bff2d0 <proc_root>,
dp=0xffff80003d807300) at fs/proc/generic.c:342
#7 0xffff00000824c628 in proc_symlink (name=<optimized out>,
parent=0xffff000008b28e40 <proc_root_init+72>, dest=0xffff000008a331a8
"self/net")
at fs/proc/generic.c:413
#8 0xffff000008b2927c in proc_net_init () at fs/proc/proc_net.c:244
#9 0xffff000008b28e40 in proc_root_init () at fs/proc/root.c:137
#10 0xffff000008b10b10 in start_kernel () at init/main.c:661
#11 0xffff000008b101e0 in __primary_switched () at arch/arm64/kernel/head.S:347
