2022-07-04 14:54:08

by Nicholas Piggin

Subject: [PATCH 12/13] locking/qspinlock: separate pv_wait_node from the non-paravirt path

pv_wait_node waits until node->locked is non-zero, no need for the
pv case to wait again by also executing the !pv code path.

Signed-off-by: Nicholas Piggin <[email protected]>
---
kernel/locking/qspinlock.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 9db168753124..19e2f286be0a 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -862,10 +862,11 @@ static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, bool parav
/* Link @node into the waitqueue. */
WRITE_ONCE(prev->next, node);

+ /* Wait for mcs node lock to be released */
if (paravirt)
pv_wait_node(node, prev);
- /* Wait for mcs node lock to be released */
- smp_cond_load_acquire(&node->locked, VAL);
+ else
+ smp_cond_load_acquire(&node->locked, VAL);

/*
* While waiting for the MCS lock, the next pointer may have
--
2.35.1

2022-07-05 18:17:24

by Peter Zijlstra

Subject: Re: [PATCH 12/13] locking/qspinlock: separate pv_wait_node from the non-paravirt path

On Tue, Jul 05, 2022 at 12:38:19AM +1000, Nicholas Piggin wrote:
> pv_wait_node waits until node->locked is non-zero, no need for the
> pv case to wait again by also executing the !pv code path.
>
> Signed-off-by: Nicholas Piggin <[email protected]>
> ---
> kernel/locking/qspinlock.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 9db168753124..19e2f286be0a 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -862,10 +862,11 @@ static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, bool parav
> /* Link @node into the waitqueue. */
> WRITE_ONCE(prev->next, node);
>
> + /* Wait for mcs node lock to be released */
> if (paravirt)
> pv_wait_node(node, prev);
> - /* Wait for mcs node lock to be released */
> - smp_cond_load_acquire(&node->locked, VAL);
> + else
> + smp_cond_load_acquire(&node->locked, VAL);
>

(from patch #6):

+static void pv_wait_node(struct qnode *node, struct qnode *prev)
+{
+ int loop;
+ bool wait_early;
+
...
+
+ /*
+ * By now our node->locked should be 1 and our caller will not actually
+ * spin-wait for it. We do however rely on our caller to do a
+ * load-acquire for us.
+ */
+}

2022-07-12 01:14:00

by Nicholas Piggin

Subject: Re: [PATCH 12/13] locking/qspinlock: separate pv_wait_node from the non-paravirt path

Excerpts from Peter Zijlstra's message of July 6, 2022 3:34 am:
> On Tue, Jul 05, 2022 at 12:38:19AM +1000, Nicholas Piggin wrote:
>> pv_wait_node waits until node->locked is non-zero, no need for the
>> pv case to wait again by also executing the !pv code path.
>>
>> Signed-off-by: Nicholas Piggin <[email protected]>
>> ---
>> kernel/locking/qspinlock.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index 9db168753124..19e2f286be0a 100644
>> --- a/kernel/locking/qspinlock.c
>> +++ b/kernel/locking/qspinlock.c
>> @@ -862,10 +862,11 @@ static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, bool parav
>> /* Link @node into the waitqueue. */
>> WRITE_ONCE(prev->next, node);
>>
>> + /* Wait for mcs node lock to be released */
>> if (paravirt)
>> pv_wait_node(node, prev);
>> - /* Wait for mcs node lock to be released */
>> - smp_cond_load_acquire(&node->locked, VAL);
>> + else
>> + smp_cond_load_acquire(&node->locked, VAL);
>>
>
> (from patch #6):
>
> +static void pv_wait_node(struct qnode *node, struct qnode *prev)
> +{
> + int loop;
> + bool wait_early;
> +
> ...
> +
> + /*
> + * By now our node->locked should be 1 and our caller will not actually
> + * spin-wait for it. We do however rely on our caller to do a
> + * load-acquire for us.
> + */
> +}
>
>

Oh good catch, thanks so much that's a dumb bug. I'll add a
smp_load_acquire at the end of pv_wait_node where that comment
is.

Thanks,
Nick