From: Waiman Long <longman@redhat.com>
To: Nicholas Piggin <npiggin@gmail.com>,
Peter Zijlstra <peterz@infradead.org>
Cc: Anton Blanchard <anton@ozlabs.org>,
Boqun Feng <boqun.feng@gmail.com>,
kvm-ppc@vger.kernel.org, linux-arch@vger.kernel.org,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Ingo Molnar <mingo@redhat.com>,
virtualization@lists.linux-foundation.org,
Will Deacon <will@kernel.org>
Subject: Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
Date: Tue, 21 Jul 2020 10:36:16 -0400 [thread overview]
Message-ID: <eaabf501-80fe-dd15-c03c-f75ce4f75877@redhat.com> (raw)
In-Reply-To: <1595327263.lk78cqolxm.astroid@bobo.none>
On 7/21/20 7:08 AM, Nicholas Piggin wrote:
> diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h
> index b752d34517b3..26d8766a1106 100644
> --- a/arch/powerpc/include/asm/qspinlock.h
> +++ b/arch/powerpc/include/asm/qspinlock.h
> @@ -31,16 +31,57 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>
> #else
> extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> +extern void queued_spin_lock_slowpath_queue(struct qspinlock *lock);
> #endif
>
> static __always_inline void queued_spin_lock(struct qspinlock *lock)
> {
> - u32 val = 0;
> -
> - if (likely(atomic_try_cmpxchg_lock(&lock->val, &val, _Q_LOCKED_VAL)))
> + atomic_t *a = &lock->val;
> + u32 val;
> +
> +again:
> + asm volatile(
> +"1:\t" PPC_LWARX(%0,0,%1,1) " # queued_spin_lock \n"
> + : "=&r" (val)
> + : "r" (&a->counter)
> + : "memory");
> +
> + if (likely(val == 0)) {
> + asm_volatile_goto(
> + " stwcx. %0,0,%1 \n"
> + " bne- %l[again] \n"
> + "\t" PPC_ACQUIRE_BARRIER " \n"
> + :
> + : "r"(_Q_LOCKED_VAL), "r" (&a->counter)
> + : "cr0", "memory"
> + : again );
> return;
> -
> - queued_spin_lock_slowpath(lock, val);
> + }
> +
> + if (likely(val == _Q_LOCKED_VAL)) {
> + asm_volatile_goto(
> + " stwcx. %0,0,%1 \n"
> + " bne- %l[again] \n"
> + :
> + : "r"(_Q_LOCKED_VAL | _Q_PENDING_VAL), "r" (&a->counter)
> + : "cr0", "memory"
> + : again );
> +
> + atomic_cond_read_acquire(a, !(VAL & _Q_LOCKED_MASK));
> +// clear_pending_set_locked(lock);
> + WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
> +// lockevent_inc(lock_pending);
> + return;
> + }
> +
> + if (val == _Q_PENDING_VAL) {
> + int cnt = _Q_PENDING_LOOPS;
> + val = atomic_cond_read_relaxed(a,
> + (VAL != _Q_PENDING_VAL) || !cnt--);
> + if (!(val & ~_Q_LOCKED_MASK))
> + goto again;
> + }
> + queued_spin_lock_slowpath_queue(lock);
> }
> #define queued_spin_lock queued_spin_lock
>
I am fine with the arch code override some part of the generic code.
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index b9515fcc9b29..ebcc6f5d99d5 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -287,10 +287,14 @@ static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock,
>
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> #define queued_spin_lock_slowpath native_queued_spin_lock_slowpath
> +#define queued_spin_lock_slowpath_queue native_queued_spin_lock_slowpath_queue
> #endif
>
> #endif /* _GEN_PV_LOCK_SLOWPATH */
>
> +void queued_spin_lock_slowpath_queue(struct qspinlock *lock);
> +static void __queued_spin_lock_slowpath_queue(struct qspinlock *lock);
> +
> /**
> * queued_spin_lock_slowpath - acquire the queued spinlock
> * @lock: Pointer to queued spinlock structure
> @@ -314,12 +318,6 @@ static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock,
> */
> void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> {
> - struct mcs_spinlock *prev, *next, *node;
> - u32 old, tail;
> - int idx;
> -
> - BUILD_BUG_ON(CONFIG_NR_CPUS >= (1U << _Q_TAIL_CPU_BITS));
> -
> if (pv_enabled())
> goto pv_queue;
>
> @@ -397,6 +395,26 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> queue:
> lockevent_inc(lock_slowpath);
> pv_queue:
> + __queued_spin_lock_slowpath_queue(lock);
> +}
> +EXPORT_SYMBOL(queued_spin_lock_slowpath);
> +
> +void queued_spin_lock_slowpath_queue(struct qspinlock *lock)
> +{
> + lockevent_inc(lock_slowpath);
> + __queued_spin_lock_slowpath_queue(lock);
> +}
> +EXPORT_SYMBOL(queued_spin_lock_slowpath_queue);
> +
> +static void __queued_spin_lock_slowpath_queue(struct qspinlock *lock)
> +{
> + struct mcs_spinlock *prev, *next, *node;
> + u32 old, tail;
> + u32 val;
> + int idx;
> +
> + BUILD_BUG_ON(CONFIG_NR_CPUS >= (1U << _Q_TAIL_CPU_BITS));
> +
> node = this_cpu_ptr(&qnodes[0].mcs);
> idx = node->count++;
> tail = encode_tail(smp_processor_id(), idx);
> @@ -559,7 +577,6 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> */
> __this_cpu_dec(qnodes[0].mcs.count);
> }
> -EXPORT_SYMBOL(queued_spin_lock_slowpath);
>
> /*
> * Generate the paravirt code for queued_spin_unlock_slowpath().
>
I would prefer to extract out the pending bit handling code out into a
separate helper function which can be overridden by the arch code
instead of breaking the slowpath into 2 pieces.
Cheers,
Longman
next prev parent reply other threads:[~2020-07-21 14:36 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-06 4:35 [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks Nicholas Piggin
2020-07-06 4:35 ` [PATCH v3 1/6] powerpc/powernv: must include hvcall.h to get PAPR defines Nicholas Piggin
2020-07-09 10:05 ` Michael Ellerman
2020-07-06 4:35 ` [PATCH v3 2/6] powerpc/pseries: move some PAPR paravirt functions to their own file Nicholas Piggin
2020-07-09 10:11 ` Michael Ellerman
2020-07-06 4:35 ` [PATCH v3 3/6] powerpc: move spinlock implementation to simple_spinlock Nicholas Piggin
2020-07-09 10:15 ` Michael Ellerman
2020-07-06 4:35 ` [PATCH v3 4/6] powerpc/64s: implement queued spinlocks and rwlocks Nicholas Piggin
2020-07-09 10:20 ` Michael Ellerman
2020-07-09 10:33 ` Peter Zijlstra
2020-07-23 14:37 ` Michal Suchánek
2020-07-06 4:35 ` [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR Nicholas Piggin
2020-07-09 10:53 ` Michael Ellerman
2020-07-09 11:03 ` Peter Zijlstra
2020-07-09 16:06 ` Waiman Long
2020-07-23 14:00 ` Peter Zijlstra
2020-07-23 18:32 ` Waiman Long
2020-07-23 18:47 ` peterz
2020-07-23 19:04 ` Waiman Long
2020-07-23 19:58 ` peterz
2020-07-23 20:30 ` Segher Boessenkool
2020-07-23 21:58 ` Waiman Long
2020-07-24 8:16 ` Will Deacon
2020-07-24 19:10 ` Waiman Long
2020-07-25 3:02 ` Waiman Long
2020-07-25 17:26 ` Peter Zijlstra
2020-07-25 17:36 ` Waiman Long
2020-07-23 14:09 ` Nicholas Piggin
2020-07-06 4:35 ` [PATCH v3 6/6] powerpc/qspinlock: optimised atomic_try_cmpxchg_lock that adds the lock hint Nicholas Piggin
2020-07-06 18:39 ` [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks Waiman Long
2020-07-07 5:57 ` Nicholas Piggin
2020-07-08 3:33 ` Waiman Long
2020-07-08 5:10 ` Nicholas Piggin
2020-07-08 23:50 ` Waiman Long
2020-07-08 23:58 ` Waiman Long
2020-07-08 8:32 ` Peter Zijlstra
2020-07-08 23:53 ` Waiman Long
2020-07-08 8:41 ` Peter Zijlstra
2020-07-08 23:54 ` Waiman Long
2020-07-09 8:31 ` Peter Zijlstra
2020-07-21 11:20 ` Nicholas Piggin
2020-07-21 11:08 ` Nicholas Piggin
2020-07-21 14:36 ` Waiman Long [this message]
2020-07-23 13:30 ` Nicholas Piggin
2020-07-23 14:29 ` Waiman Long
2020-07-23 16:12 ` Nicholas Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eaabf501-80fe-dd15-c03c-f75ce4f75877@redhat.com \
--to=longman@redhat.com \
--cc=anton@ozlabs.org \
--cc=boqun.feng@gmail.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mingo@redhat.com \
--cc=npiggin@gmail.com \
--cc=peterz@infradead.org \
--cc=virtualization@lists.linux-foundation.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).