[src/trunk]: src/sys/kern KERNEL_LOCK(9): Minor tweaks to ci->ci_biglock_want...

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/893a09c791ac
branches:  trunk
changeset: 373676:893a09c791ac
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Thu Feb 23 14:57:29 2023 +0000

description:
KERNEL_LOCK(9): Minor tweaks to ci->ci_biglock_wanted access.

1. Use atomic_load_relaxed to read ci->ci_biglock_wanted from another
   CPU, for clarity and to avoid the appearance of data races in thread
   sanitizers.  (Reading ci->ci_biglock_wanted on the local CPU need
   not be atomic because no other CPU can be writing to it.)

2. Use atomic_store_relaxed to update ci->ci_biglock_wanted when we
   start to spin, to avoid the appearance of data races.

3. Add comments to explain what's going on and cross-reference the
   specific matching membars in mutex_vector_enter.

related to PR kern/57240

diffstat:

 sys/kern/kern_lock.c  |  38 ++++++++++++++++++++++----------------
 sys/kern/kern_mutex.c |   6 +++---
 2 files changed, 25 insertions(+), 19 deletions(-)

diffs (99 lines):

diff -r ae30c377602d -r 893a09c791ac sys/kern/kern_lock.c
--- a/sys/kern/kern_lock.c      Thu Feb 23 14:57:08 2023 +0000
+++ b/sys/kern/kern_lock.c      Thu Feb 23 14:57:29 2023 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_lock.c,v 1.182 2023/01/27 09:28:41 ozaki-r Exp $  */
+/*     $NetBSD: kern_lock.c,v 1.183 2023/02/23 14:57:29 riastradh Exp $        */
 
 /*-
  * Copyright (c) 2002, 2006, 2007, 2008, 2009, 2020 The NetBSD Foundation, Inc.
@@ -31,7 +31,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_lock.c,v 1.182 2023/01/27 09:28:41 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_lock.c,v 1.183 2023/02/23 14:57:29 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_lockdebug.h"
@@ -235,10 +235,18 @@
         * is required to ensure that the result of any mutex_exit()
         * by the current LWP becomes visible on the bus before the set
         * of ci->ci_biglock_wanted becomes visible.
+        *
+        * This membar_producer matches the membar_consumer in
+        * mutex_vector_enter.
+        *
+        * That way, if l has just released a mutex, mutex_vector_enter
+        * can't see this store ci->ci_biglock_wanted := l until it
+        * will also see the mutex_exit store mtx->mtx_owner := 0 which
+        * clears the has-waiters bit.
         */
        membar_producer();
        owant = ci->ci_biglock_wanted;
-       ci->ci_biglock_wanted = l;
+       atomic_store_relaxed(&ci->ci_biglock_wanted, l);
 #if defined(DIAGNOSTIC) && !defined(LOCKDEBUG)
        l->l_ld_wanted = __builtin_return_address(0);
 #endif
@@ -286,22 +294,20 @@
 
        /*
         * Now that we have kernel_lock, reset ci_biglock_wanted.  This
-        * store must be unbuffered (immediately visible on the bus) in
-        * order for non-interlocked mutex release to work correctly.
-        * It must be visible before a mutex_exit() can execute on this
-        * processor.
+        * store must be visible on other CPUs before a mutex_exit() on
+        * this CPU can test the has-waiters bit.
         *
-        * Note: only where CAS is available in hardware will this be
-        * an unbuffered write, but non-interlocked release cannot be
-        * done on CPUs without CAS in hardware.
+        * This membar_enter matches the membar_enter in
+        * mutex_vector_enter.  (Yes, not membar_exit -- the legacy
+        * naming is confusing, but store-before-load usually pairs
+        * with store-before-load, in the extremely rare cases where it
+        * is used at all.)
+        *
+        * That way, mutex_vector_enter can't see this store
+        * ci->ci_biglock_wanted := owant until it has set the
+        * has-waiters bit.
         */
        (void)atomic_swap_ptr(&ci->ci_biglock_wanted, owant);
-
-       /*
-        * Issue a memory barrier as we have acquired a lock.  This also
-        * prevents stores from a following mutex_exit() being reordered
-        * to occur before our store to ci_biglock_wanted above.
-        */
 #ifndef __HAVE_ATOMIC_AS_MEMBAR
        membar_enter();
 #endif
diff -r ae30c377602d -r 893a09c791ac sys/kern/kern_mutex.c
--- a/sys/kern/kern_mutex.c     Thu Feb 23 14:57:08 2023 +0000
+++ b/sys/kern/kern_mutex.c     Thu Feb 23 14:57:29 2023 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_mutex.c,v 1.102 2023/01/27 09:28:41 ozaki-r Exp $ */
+/*     $NetBSD: kern_mutex.c,v 1.103 2023/02/23 14:57:29 riastradh Exp $       */
 
 /*-
  * Copyright (c) 2002, 2006, 2007, 2008, 2019 The NetBSD Foundation, Inc.
@@ -40,7 +40,7 @@
 #define        __MUTEX_PRIVATE
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_mutex.c,v 1.102 2023/01/27 09:28:41 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_mutex.c,v 1.103 2023/02/23 14:57:29 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/atomic.h>
@@ -427,7 +427,7 @@
 
        if (ci && ci->ci_curlwp == l) {
                /* Target is running; do we need to block? */
-               return (ci->ci_biglock_wanted != l);
+               return (atomic_load_relaxed(&ci->ci_biglock_wanted) != l);
        }
 
        /* Not running.  It may be safe to block now. */