MCS Lock: MCS lock code cleanup and optimizations

In this patch series, we separated out the MCS lock code which was previously embedded in the mutex.c. This allows for easier reuse of MCS lock in other places like rwsem and qrwlock. We also did some micro optimizations and barrier cleanup.

This patches were previously part of the rwsem optimization patch series but now we spearate them out.

Tim Chen

v2:
1. change export mcs_spin_lock as a GPL export symbol
2. corrected mcs_spin_lock to references

Jason Low (2):
MCS Lock: optimizations and extra comments
MCS Lock: Barrier corrections

Tim Chen (1):
MCS Lock: Restructure the MCS lock defines and locking code into its
own file

Waiman Long (1):
MCS Lock: Make mcs_spinlock.h includable in other files

 include/linux/mcs_spinlock.h |   99 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mutex.h        |    5 +-
 kernel/Makefile              |    6 +-
 kernel/mcs_spinlock.c        |   21 +++++++++
 kernel/mutex.c               |   60 +++----------------------
 5 files changed, 133 insertions(+), 58 deletions(-)
 create mode 100644 include/linux/mcs_spinlock.h
 create mode 100644 kernel/mcs_spinlock.c

Quoted 25 lines · Tim Chen

In this patch series, we separated out the MCS lock code which was previously embedded in the mutex.c. This allows for easier reuse of MCS lock in other places like rwsem and qrwlock. We also did some micro optimizations and barrier cleanup.

This patches were previously part of the rwsem optimization patch series but now we spearate them out.

Tim Chen

v2:
1. change export mcs_spin_lock as a GPL export symbol
2. corrected mcs_spin_lock to references

Jason Low (2):
MCS Lock: optimizations and extra comments
MCS Lock: Barrier corrections

Tim Chen (1):
MCS Lock: Restructure the MCS lock defines and locking code into its
own file

Waiman Long (1):
MCS Lock: Make mcs_spinlock.h includable in other files

Quoted 8 lines

 include/linux/mcs_spinlock.h |   99 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mutex.h        |    5 +-
 kernel/Makefile              |    6 +-
 kernel/mcs_spinlock.c        |   21 +++++++++
 kernel/mutex.c               |   60 +++----------------------
 5 files changed, 133 insertions(+), 58 deletions(-)
 create mode 100644 include/linux/mcs_spinlock.h
 create mode 100644 kernel/mcs_spinlock.c

What base kernel does this apply over ?

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -0

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */

include/linux/mcs_spinlock.h +1 -0

 }
 
 /*

@@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
 
 	if (likely(!next)) {
 		/*
+		 * cmpxchg() provides a memory barrier.
 		 * Release the lock by setting it to NULL
 		 */
 		if (likely(cmpxchg(lock, node, NULL) == node))

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 19 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -0

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 10 lines

include/linux/mcs_spinlock.h +3 -1

 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))
 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

Quoted 16 lines

include/linux/mcs_spinlock.h +2 -0

 }
 
 /*

@@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
 
 	if (likely(!next)) {
 		/*
+		 * cmpxchg() provides a memory barrier.
 		 * Release the lock by setting it to NULL
 		 */
 		if (likely(cmpxchg(lock, node, NULL) == node))

@@ -65,9 +69,14 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
 		/* Wait until the next pointer is set */
 		while (!(next = ACCESS_ONCE(node->next)))
 			arch_mutex_cpu_relax();
+	} else {

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod · line 77

Quoted 5 lines

include/linux/mcs_spinlock.h +5 -0

+		/*
+		 * Make sure all operations within the critical section
+		 * happen before the lock is released.
+		 */
+		smp_wmb();

...but I don't see what prevents reads inside the critical section from moving across the smp_wmb() here.

What am I missing?

Will

Quoted 32 lines · Will Deacon

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 25 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -1

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */
 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))

Quoted 8 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 4 lines

include/linux/mcs_spinlock.h +3 -0

 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

Quoted 33 lines · Tim Chen

Quoted 32 lines · Will Deacon

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 25 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -1

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */
 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))

Quoted 8 lines

Quoted 8 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 4 lines

include/linux/mcs_spinlock.h +3 -0

 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

PaulMck completely confused me a few days ago with control dependencies etc.. Pretty much saying that C/C++ doesn't do those.

Quoted 34 lines · Peter Zijlstra

Quoted 33 lines · Tim Chen

Quoted 32 lines · Will Deacon

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 25 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -1

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */
 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 4 lines

include/linux/mcs_spinlock.h +3 -0

 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

The smp_rmb was put in to make sure that the lock
is indeed set before we start doing speculative reads in next critical section.

Wonder if your concern is about the possibility of write in next critical section bleeding into read in previous critical section?

If reads and writes are re-ordered in previous critical section before mcs_spin_unlock, it may be possible that the previous critical section is still
reading when it set the lock for the next mcs in mcs_spin_unlock.
This allows the next critical section to start writing prematurely, before previous critical section finished all reads.

If this concern is valid, we should change the smp_wmb() to smp_mb() in the unlock function, to make sure previous critical section has completed all operations before next section starts.

Quoted 2 lines

PaulMck completely confused me a few days ago with control dependencies etc.. Pretty much saying that C/C++ doesn't do those.

Will appreciate feedback getting the barriers right.

Quoted 33 lines · Tim Chen

Quoted 32 lines · Will Deacon

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 25 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -1

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */
 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))

Quoted 8 lines

Quoted 8 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 4 lines

include/linux/mcs_spinlock.h +3 -0

 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

PaulMck completely confused me a few days ago with control dependencies etc.. Pretty much saying that C/C++ doesn't do those.

Quoted 43 lines · Peter Zijlstra

Quoted 41 lines · Tim Chen

Quoted 40 lines · Will Deacon

Quoted 2 lines · Tim Chen

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely understand how what you have is correct. Hopefully you can help me out :)

Quoted 19 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

include/linux/mcs_spinlock.h +1 -0

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 	node->locked = 0;
 	node->next   = NULL;
 
+	/* xchg() provides a memory barrier */
 	prev = xchg(lock, node);
 	if (likely(prev == NULL)) {
 		/* Lock acquired */

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 10 lines

include/linux/mcs_spinlock.h +3 -1

 		return;
 	}
 	ACCESS_ONCE(prev->next) = node;
-	smp_wmb();
 	/* Wait until the lock holder passes the lock down */
 	while (!ACCESS_ONCE(node->locked))
 		arch_mutex_cpu_relax();
+
+	/* Make sure subsequent operations happen after the lock is acquired */
+	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

Quoted 2 lines

PaulMck completely confused me a few days ago with control dependencies etc.. Pretty much saying that C/C++ doesn't do those.

I remember that there was a subtlety here, but don't remember what it was...

And while I do remember reviewing this code, I don't find any evidence that I gave my "Reviewed-by". Tim/Jason, if I fat-fingered this, please forward that email back to me.

Thanx, Paul

Quoted 3 lines · Tim Chen

Quoted 2 lines

This patch corrects the way memory barriers are used in the MCS lock and removes ones that are not needed. Also add comments on all

barriers.

Quoted 2 lines

Hmm, I see that you're fixing up the barriers, but I still don't

completely

Quoted 10 lines

understand how what you have is correct. Hopefully you can help me out :)

Review

Quoted 8 lines

Reviewed-by Paul E. McKenney <[email protected]>

Reviewed-by Tim Chen <[email protected]>

Signed-off-by Jason Low <[email protected]>

---
 include/linux/mcs_spinlock.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/include/linux/mcs_spinlock.h

Quoted 4 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock,

Quoted 4 lines

index 96f14299..93d445d 100644

include/linux/mcs_spinlock.h +0 -0

@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock,

struct mcs_spinlock *node)

Quoted 16 lines

Quoted 6 lines

include/linux/mcs_spinlock.h +1 -0

    node->locked = 0;
    node->next   = NULL;
 
+   /* xchg() provides a memory barrier */
    prev = xchg(lock, node);
    if (likely(prev == NULL)) {

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)

Quoted 10 lines

include/linux/mcs_spinlock.h +2 -1

            /* Lock acquired */
            return;
    }
    ACCESS_ONCE(prev->next) = node;
-   smp_wmb();
    /* Wait until the lock holder passes the lock down */
    while (!ACCESS_ONCE(node->locked))
            arch_mutex_cpu_relax();
+
+   /* Make sure subsequent operations happen after the lock is

acquired */

Quoted 3 lines

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 1 line

include/linux/mcs_spinlock.h +1 -0

+   smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't

speculated,

Quoted 1 line

right? (i.e. the control dependency above is enough for stores to be

ordered

Quoted 6 lines

with respect to taking the lock)...

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock

Quoted 4 lines

include/linux/mcs_spinlock.h +0 -0

 }
 
 /*

@@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock

**lock, struct mcs_spinlock *nod

Quoted 8 lines

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock

Quoted 8 lines

include/linux/mcs_spinlock.h +1 -0

    if (likely(!next)) {
            /*
+            * cmpxchg() provides a memory barrier.
             * Release the lock by setting it to NULL
             */
            if (likely(cmpxchg(lock, node, NULL) == node))

@@ -65,9 +69,14 @@ static void mcs_spin_unlock(struct mcs_spinlock

**lock, struct mcs_spinlock *nod

Quoted 10 lines

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod · line 77

Quoted 9 lines

include/linux/mcs_spinlock.h +6 -0

            /* Wait until the next pointer is set */
            while (!(next = ACCESS_ONCE(node->next)))
                    arch_mutex_cpu_relax();
+   } else {
+           /*
+            * Make sure all operations within the critical section
+            * happen before the lock is released.
+            */
+           smp_wmb();

Quoted 2 lines

...but I don't see what prevents reads inside the critical section from moving across the smp_wmb() here.

This is to prevent any read in next critical section from creeping up before write in the previous critical section has completed

e.g.
CPU 1 execute

        mcs_lock
        x = 1;
        ...
        x = 2;
        mcs_unlock

and CPU 2 execute

        mcs_lock
        y = x;
        ...
        mcs_unlock

We expect y to be 2 after the "y = x" assignment. Without the proper rmb in lock and wmb in unlock, y could be 1 for CPU 2 with speculative read (i.e. before the x=2 assignment is completed).

2013/11/6 Tim Chen <[email protected]<mailto:[email protected]>>

Quoted 44 lines · Will Deacon

Quoted 12 lines · Tim Chen

include/linux/mcs_spinlock.h +1 -0

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
    node->locked = 0;
    node->next   = NULL;
 
+   /* xchg() provides a memory barrier */
    prev = xchg(lock, node);
    if (likely(prev == NULL)) {
            /* Lock acquired */

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 10 lines

include/linux/mcs_spinlock.h +3 -1

            return;
    }
    ACCESS_ONCE(prev->next) = node;
-   smp_wmb();
    /* Wait until the lock holder passes the lock down */
    while (!ACCESS_ONCE(node->locked))
            arch_mutex_cpu_relax();
+
+   /* Make sure subsequent operations happen after the lock is acquired */
+   smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

Quoted 16 lines

include/linux/mcs_spinlock.h +2 -0

 }
 
 /*

@@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
 
    if (likely(!next)) {
            /*
+            * cmpxchg() provides a memory barrier.
             * Release the lock by setting it to NULL
             */
            if (likely(cmpxchg(lock, node, NULL) == node))

@@ -65,9 +69,14 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
            /* Wait until the next pointer is set */
            while (!(next = ACCESS_ONCE(node->next)))
                    arch_mutex_cpu_relax();
+   } else {

Quoted 8 lines

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod · line 77

Quoted 5 lines

include/linux/mcs_spinlock.h +5 -0

+           /*
+            * Make sure all operations within the critical section
+            * happen before the lock is released.
+            */
+           smp_wmb();

...but I don't see what prevents reads inside the critical section from moving across the smp_wmb() here.

This is to prevent any read in next critical section from creeping up before write in the previous critical section has completed

e.g.
CPU 1 execute

        mcs_lock
        x = 1;
        ...
        x = 2;
        mcs_unlock

and CPU 2 execute

        mcs_lock
        y = x;
        ...
        mcs_unlock

We expect y to be 2 after the "y = x" assignment. Without the proper rmb in lock and wmb in unlock, y could be 1 for CPU 2 with speculative read (i.e. before the x=2 assignment is completed).

is it not a good example ?

Quoted 52 lines · Figo.zhang

2013/11/6 Tim Chen<[email protected]<mailto:[email protected]>>

Quoted 50 lines · Will Deacon

Quoted 12 lines · Tim Chen

include/linux/mcs_spinlock.h +1 -0

index 96f14299..93d445d 100644
@@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
     node->locked = 0;
     node->next   = NULL;
 
+   /* xchg() provides a memory barrier */
     prev = xchg(lock, node);
     if (likely(prev == NULL)) {
             /* Lock acquired */

include/linux/mcs_spinlock.h void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) · line 51

Quoted 10 lines

include/linux/mcs_spinlock.h +3 -1

             return;
     }
     ACCESS_ONCE(prev->next) = node;
-   smp_wmb();
     /* Wait until the lock holder passes the lock down */
     while (!ACCESS_ONCE(node->locked))
             arch_mutex_cpu_relax();
+
+   /* Make sure subsequent operations happen after the lock is acquired */
+   smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated, right? (i.e. the control dependency above is enough for stores to be ordered with respect to taking the lock)...

Quoted 12 lines

include/linux/mcs_spinlock.h +1 -0

  }
 
  /*

@@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
 
     if (likely(!next)) {
             /*
+            * cmpxchg() provides a memory barrier.
              * Release the lock by setting it to NULL
              */
             if (likely(cmpxchg(lock, node, NULL) == node))

include/linux/mcs_spinlock.h static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod · line 77

Quoted 9 lines

include/linux/mcs_spinlock.h +6 -0

@@ -65,9 +69,14 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
             /* Wait until the next pointer is set */
             while (!(next = ACCESS_ONCE(node->next)))
                     arch_mutex_cpu_relax();
+   } else {
+           /*
+            * Make sure all operations within the critical section
+            * happen before the lock is released.
+            */
+           smp_wmb();

...but I don't see what prevents reads inside the critical section from moving across the smp_wmb() here.

Quoted 3 lines

This is to prevent any read in next critical section from creeping up before write in the previous critical section has completed

Understood, but an smp_wmb() doesn't provide any ordering guarantees with respect to reads, hence why I think you need an smp_mb() here.

include/linux/mcs_spinlock.h struct mcs_spinlock { · line 49

Quoted 10 lines · Tim Chen

+ * The _raw_mcs_spin_lock() function should not be called directly. Instead,
+ * users should call mcs_spin_lock().
*/

include/linux/mcs_spinlock.h +2 -2

-static noinline
-void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
+static inline
+void _raw_mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 {
 	struct mcs_spinlock *prev;

So why keep it in the header at all?

Quoted 12 lines · Peter Zijlstra

include/linux/mcs_spinlock.h struct mcs_spinlock { · line 49

Quoted 10 lines · Tim Chen

+ * The _raw_mcs_spin_lock() function should not be called directly. Instead,
+ * users should call mcs_spin_lock().
*/

include/linux/mcs_spinlock.h +2 -2

-static noinline
-void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
+static inline
+void _raw_mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
  {
  	struct mcs_spinlock *prev;

So why keep it in the header at all?

I also made the suggestion originally of keeping both lock and unlock in mcs_spinlock.c. Wonder if Waiman decides to keep them in header because in-lining the unlock function makes execution a bit faster?

Tim

Quoted 17 lines · Tim Chen

Quoted 12 lines · Peter Zijlstra

include/linux/mcs_spinlock.h struct mcs_spinlock { · line 49

Quoted 10 lines · Tim Chen

+ * The _raw_mcs_spin_lock() function should not be called directly. Instead,
+ * users should call mcs_spin_lock().
*/

include/linux/mcs_spinlock.h +2 -2

-static noinline
-void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
+static inline
+void _raw_mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
 {
 	struct mcs_spinlock *prev;

So why keep it in the header at all?

I also made the suggestion originally of keeping both lock and unlock in mcs_spinlock.c. Wonder if Waiman decides to keep them in header because in-lining the unlock function makes execution a bit faster?

Quoted 2 lines

Tim

I was following the example of the spinlock code where the lock function is not inlined, but the unlock function is. I have no objection to make them both as non-inlined functions, if you think that is the right move.