diff mbox series

[next,v2,3/5] locking/osq_lock: Use node->prev_cpu instead of saving node->prev.

Message ID 7906aaa73f93493c873e6286c1f96645@AcuMS.aculab.com
State New
Headers
Series locking/osq_lock: Optimisations to osq_lock code. |

Commit Message

David Laight Dec. 31, 2023, 9:54 p.m. UTC 
  node->prev is only used to update 'prev' in the unlikely case
of concurrent unqueues.
This can be replaced by a check for node->prev_cpu changing
and then calling decode_cpu() to get the changed 'prev' pointer.

node->cpu (or more particularly) prev->cpu is only used for the
osq_wait_next() call in the unqueue path.
Normally this is exactly the value that the initial xchg() read
from lock->tail (used to obtain 'prev'), but can get updated
by concurrent unqueues.

Both the 'prev' and 'cpu' members of optimistic_spin_node are
now unused and can be deleted.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 kernel/locking/osq_lock.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Waiman Long Jan. 1, 2024, 4:09 a.m. UTC | #1 
On 12/31/23 16:54, David Laight wrote:
> node->prev is only used to update 'prev' in the unlikely case
> of concurrent unqueues.
> This can be replaced by a check for node->prev_cpu changing
> and then calling decode_cpu() to get the changed 'prev' pointer.
>
> node->cpu (or more particularly) prev->cpu is only used for the
> osq_wait_next() call in the unqueue path.
> Normally this is exactly the value that the initial xchg() read
> from lock->tail (used to obtain 'prev'), but can get updated
> by concurrent unqueues.
>
> Both the 'prev' and 'cpu' members of optimistic_spin_node are
> now unused and can be deleted.
>
> Signed-off-by: David Laight <david.laight@aculab.com>
> ---
>   kernel/locking/osq_lock.c | 31 +++++++++++++++++--------------
>   1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> index eb8a6dfdb79d..27324b509f68 100644
> --- a/kernel/locking/osq_lock.c
> +++ b/kernel/locking/osq_lock.c
> @@ -13,9 +13,8 @@
>    */
>   
>   struct optimistic_spin_node {
> -	struct optimistic_spin_node *next, *prev;
> +	struct optimistic_spin_node *next;
>   	int locked;    /* 1 if lock acquired */
> -	int cpu;       /* encoded CPU # + 1 value */
>   	int prev_cpu;  /* encoded CPU # + 1 value */
>   };
>   
> @@ -91,10 +90,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>   	struct optimistic_spin_node *node = this_cpu_ptr(&osq_node);
>   	struct optimistic_spin_node *prev, *next;
>   	int curr = encode_cpu(smp_processor_id());
> -	int old;
> +	int prev_cpu;
>   
>   	node->next = NULL;
> -	node->cpu = curr;
>   
>   	/*
>   	 * We need both ACQUIRE (pairs with corresponding RELEASE in
> @@ -102,13 +100,12 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>   	 * the node fields we just initialised) semantics when updating
>   	 * the lock tail.
>   	 */
> -	old = atomic_xchg(&lock->tail, curr);
> -	if (old == OSQ_UNLOCKED_VAL)
> +	prev_cpu = atomic_xchg(&lock->tail, curr);
> +	if (prev_cpu == OSQ_UNLOCKED_VAL)
>   		return true;
>   
> -	node->prev_cpu = old;
> -	prev = decode_cpu(old);
> -	node->prev = prev;
> +	node->prev_cpu = prev_cpu;
> +	prev = decode_cpu(prev_cpu);
>   	node->locked = 0;
>   
>   	/*
> @@ -174,9 +171,16 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>   
>   		/*
>   		 * Or we race against a concurrent unqueue()'s step-B, in which
> -		 * case its step-C will write us a new @node->prev pointer.
> +		 * case its step-C will write us a new @node->prev_cpu value.
>   		 */
> -		prev = READ_ONCE(node->prev);
> +		{
> +			int new_prev_cpu = READ_ONCE(node->prev_cpu);
> +
> +			if (new_prev_cpu == prev_cpu)
> +				continue;
> +			prev_cpu = new_prev_cpu;
> +			prev = decode_cpu(prev_cpu);
> +		}

Just a minor nit. It is not that common in the kernel to add another 
nesting level just to reduce the scope of  new_prev_cpu auto variable.

Anyway,

Reviewed-by: Waiman Long <longman@redhat.com>
diff mbox series

Patch

diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index eb8a6dfdb79d..27324b509f68 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -13,9 +13,8 @@ 
  */
 
 struct optimistic_spin_node {
-	struct optimistic_spin_node *next, *prev;
+	struct optimistic_spin_node *next;
 	int locked;    /* 1 if lock acquired */
-	int cpu;       /* encoded CPU # + 1 value */
 	int prev_cpu;  /* encoded CPU # + 1 value */
 };
 
@@ -91,10 +90,9 @@  bool osq_lock(struct optimistic_spin_queue *lock)
 	struct optimistic_spin_node *node = this_cpu_ptr(&osq_node);
 	struct optimistic_spin_node *prev, *next;
 	int curr = encode_cpu(smp_processor_id());
-	int old;
+	int prev_cpu;
 
 	node->next = NULL;
-	node->cpu = curr;
 
 	/*
 	 * We need both ACQUIRE (pairs with corresponding RELEASE in
@@ -102,13 +100,12 @@  bool osq_lock(struct optimistic_spin_queue *lock)
 	 * the node fields we just initialised) semantics when updating
 	 * the lock tail.
 	 */
-	old = atomic_xchg(&lock->tail, curr);
-	if (old == OSQ_UNLOCKED_VAL)
+	prev_cpu = atomic_xchg(&lock->tail, curr);
+	if (prev_cpu == OSQ_UNLOCKED_VAL)
 		return true;
 
-	node->prev_cpu = old;
-	prev = decode_cpu(old);
-	node->prev = prev;
+	node->prev_cpu = prev_cpu;
+	prev = decode_cpu(prev_cpu);
 	node->locked = 0;
 
 	/*
@@ -174,9 +171,16 @@  bool osq_lock(struct optimistic_spin_queue *lock)
 
 		/*
 		 * Or we race against a concurrent unqueue()'s step-B, in which
-		 * case its step-C will write us a new @node->prev pointer.
+		 * case its step-C will write us a new @node->prev_cpu value.
 		 */
-		prev = READ_ONCE(node->prev);
+		{
+			int new_prev_cpu = READ_ONCE(node->prev_cpu);
+
+			if (new_prev_cpu == prev_cpu)
+				continue;
+			prev_cpu = new_prev_cpu;
+			prev = decode_cpu(prev_cpu);
+		}
 	}
 
 	/*
@@ -186,7 +190,7 @@  bool osq_lock(struct optimistic_spin_queue *lock)
 	 * back to @prev.
 	 */
 
-	next = osq_wait_next(lock, node, prev->cpu);
+	next = osq_wait_next(lock, node, prev_cpu);
 	if (!next)
 		return false;
 
@@ -198,8 +202,7 @@  bool osq_lock(struct optimistic_spin_queue *lock)
 	 * it will wait in Step-A.
 	 */
 
-	WRITE_ONCE(next->prev_cpu, prev->cpu);
-	WRITE_ONCE(next->prev, prev);
+	WRITE_ONCE(next->prev_cpu, prev_cpu);
 	WRITE_ONCE(prev->next, next);
 
 	return false;