[src/trunk]: src/sys/kern kern: Fix synchronization of clearing LP_RUNNING an...

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

details:   https://anonhg.NetBSD.org/src/rev/01f4f8da1ff9
branches:  trunk
changeset: 363411:01f4f8da1ff9
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Thu Mar 10 12:21:25 2022 +0000

description:
kern: Fix synchronization of clearing LP_RUNNING and lwp_free.

1. membar_sync is not necessary here -- only a store-release is
   required.

2. membar_consumer _before_ loading l->l_pflag is not enough; a
   load-acquire is required.

Actually it's not really clear to me why any barriers are needed, since
the store-release and load-acquire should be implied by releasing and
acquiring the lwp lock (and maybe we could spin with the lock instead
of reading l->l_pflag unlocked).  But maybe there's something subtle
about access to l->l_mutex that's not obvious here.

diffstat:

 sys/kern/kern_lwp.c   |  26 ++++++++++++++++----------
 sys/kern/kern_synch.c |  15 ++++++++++-----
 2 files changed, 26 insertions(+), 15 deletions(-)

diffs (104 lines):

diff -r 85e1fd33628e -r 01f4f8da1ff9 sys/kern/kern_lwp.c
--- a/sys/kern/kern_lwp.c       Thu Mar 10 04:14:34 2022 +0000
+++ b/sys/kern/kern_lwp.c       Thu Mar 10 12:21:25 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_lwp.c,v 1.246 2021/12/22 16:57:28 thorpej Exp $   */
+/*     $NetBSD: kern_lwp.c,v 1.247 2022/03/10 12:21:25 riastradh Exp $ */
 
 /*-
  * Copyright (c) 2001, 2006, 2007, 2008, 2009, 2019, 2020
@@ -217,7 +217,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_lwp.c,v 1.246 2021/12/22 16:57:28 thorpej Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_lwp.c,v 1.247 2022/03/10 12:21:25 riastradh Exp $");
 
 #include "opt_ddb.h"
 #include "opt_lockdebug.h"
@@ -1017,16 +1017,19 @@
        KASSERT(curcpu()->ci_mtx_count == -2);
 
        /*
-        * Immediately mark the previous LWP as no longer running and unlock
-        * (to keep lock wait times short as possible).  If a zombie, don't
-        * touch after clearing LP_RUNNING as it could be reaped by another
-        * CPU.  Issue a memory barrier to ensure this.
+        * Immediately mark the previous LWP as no longer running and
+        * unlock (to keep lock wait times short as possible).  If a
+        * zombie, don't touch after clearing LP_RUNNING as it could be
+        * reaped by another CPU.  Use atomic_store_release to ensure
+        * this -- matches atomic_load_acquire in lwp_free.
         */
        lock = prev->l_mutex;
        if (__predict_false(prev->l_stat == LSZOMB)) {
-               membar_sync();
+               atomic_store_release(&prev->l_pflag,
+                   prev->l_pflag & ~LP_RUNNING);
+       } else {
+               prev->l_pflag &= ~LP_RUNNING;
        }
-       prev->l_pflag &= ~LP_RUNNING;
        mutex_spin_exit(lock);
 
        /* Correct spin mutex count after mi_switch(). */
@@ -1246,9 +1249,12 @@
        /*
         * In the unlikely event that the LWP is still on the CPU,
         * then spin until it has switched away.
+        *
+        * atomic_load_acquire matches atomic_store_release in
+        * lwp_startup and mi_switch.
         */
-       membar_consumer();
-       while (__predict_false((l->l_pflag & LP_RUNNING) != 0)) {
+       while (__predict_false((atomic_load_acquire(&l->l_pflag) & LP_RUNNING)
+               != 0)) {
                SPINLOCK_BACKOFF_HOOK;
        }
 
diff -r 85e1fd33628e -r 01f4f8da1ff9 sys/kern/kern_synch.c
--- a/sys/kern/kern_synch.c     Thu Mar 10 04:14:34 2022 +0000
+++ b/sys/kern/kern_synch.c     Thu Mar 10 12:21:25 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_synch.c,v 1.349 2020/05/23 23:42:43 ad Exp $      */
+/*     $NetBSD: kern_synch.c,v 1.350 2022/03/10 12:21:25 riastradh Exp $       */
 
 /*-
  * Copyright (c) 1999, 2000, 2004, 2006, 2007, 2008, 2009, 2019, 2020
@@ -69,7 +69,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_synch.c,v 1.349 2020/05/23 23:42:43 ad Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_synch.c,v 1.350 2022/03/10 12:21:25 riastradh Exp $");
 
 #include "opt_kstack.h"
 #include "opt_dtrace.h"
@@ -783,18 +783,23 @@
 
                /*
                 * Immediately mark the previous LWP as no longer running
-                * and unlock (to keep lock wait times short as possible). 
+                * and unlock (to keep lock wait times short as possible).
                 * We'll still be at IPL_SCHED afterwards.  If a zombie,
                 * don't touch after clearing LP_RUNNING as it could be
                 * reaped by another CPU.  Issue a memory barrier to ensure
                 * this.
+                *
+                * atomic_store_release matches atomic_load_acquire in
+                * lwp_free.
                 */
                KASSERT((prevlwp->l_pflag & LP_RUNNING) != 0);
                lock = prevlwp->l_mutex;
                if (__predict_false(prevlwp->l_stat == LSZOMB)) {
-                       membar_sync();
+                       atomic_store_release(&prevlwp->l_pflag,
+                           prevlwp->l_pflag & ~LP_RUNNING);
+               } else {
+                       prevlwp->l_pflag &= ~LP_RUNNING;
                }
-               prevlwp->l_pflag &= ~LP_RUNNING;
                mutex_spin_exit(lock);
 
                /*