[src/trunk]: src/sys/kern timecounter(9): Sprinkle membar_consumer around th-...

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

details:   https://anonhg.NetBSD.org/src/rev/c06633eecdf4
branches:  trunk
changeset: 377572:c06633eecdf4
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Mon Jul 17 21:51:45 2023 +0000

description:
timecounter(9): Sprinkle membar_consumer around th->th_generation.

This code was apparently written under the misapprehension that
membar_producer on one side is good enough, but that doesn't
accomplish anything other than making the code unnecessarily obscure.

For semantics, you always always always need memory barriers to come
in pairs, with membar_consumer on the reading side if you want the
membar_producer to have on the writing side to have any useful
effect.

It is unfortunate that this might hurt performance, but that's an
unfortunate consequence of the design made without understanding
memory barriers, not an unfortunate consequence of the memory
barriers.

If it is really critical for the read side to avoid memory barriers,
then the write side needs to issue an IPI or xcall to effect memory
barriers -- at higher cost to the write side, of course.

diffstat:

 sys/kern/kern_tc.c |  22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diffs (100 lines):

diff -r 2e9ca98a3723 -r c06633eecdf4 sys/kern/kern_tc.c
--- a/sys/kern/kern_tc.c        Mon Jul 17 21:51:30 2023 +0000
+++ b/sys/kern/kern_tc.c        Mon Jul 17 21:51:45 2023 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_tc.c,v 1.72 2023/07/17 21:51:30 riastradh Exp $ */
+/* $NetBSD: kern_tc.c,v 1.73 2023/07/17 21:51:45 riastradh Exp $ */
 
 /*-
  * Copyright (c) 2008, 2009 The NetBSD Foundation, Inc.
@@ -40,7 +40,7 @@
 
 #include <sys/cdefs.h>
 /* __FBSDID("$FreeBSD: src/sys/kern/kern_tc.c,v 1.166 2005/09/19 22:16:31 andre Exp $"); */
-__KERNEL_RCSID(0, "$NetBSD: kern_tc.c,v 1.72 2023/07/17 21:51:30 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_tc.c,v 1.73 2023/07/17 21:51:45 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_ntp.h"
@@ -457,6 +457,10 @@ binuptime(struct bintime *bt)
         * updating timecounter_removals will issue a broadcast cross call
         * before inspecting our l_tcgen value (this elides memory ordering
         * issues).
+        *
+        * XXX If the author of the above comment knows how to make it
+        * safe to avoid memory barriers around the access to
+        * th->th_generation, I'm all ears.
         */
        l = curlwp;
        lgen = l->l_tcgen;
@@ -468,8 +472,10 @@ binuptime(struct bintime *bt)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                *bt = th->th_offset;
                bintime_addx(bt, th->th_scale * tc_delta(th));
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 
        __insn_barrier();
@@ -537,7 +543,9 @@ getbinuptime(struct bintime *bt)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                *bt = th->th_offset;
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 }
 
@@ -551,7 +559,9 @@ getnanouptime(struct timespec *tsp)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                bintime2timespec(&th->th_offset, tsp);
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 }
 
@@ -565,7 +575,9 @@ getmicrouptime(struct timeval *tvp)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                bintime2timeval(&th->th_offset, tvp);
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 }
 
@@ -580,7 +592,9 @@ getbintime(struct bintime *bt)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                *bt = th->th_offset;
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
        getbinboottime(&boottime);
        bintime_add(bt, &boottime);
@@ -596,7 +610,9 @@ dogetnanotime(struct timespec *tsp)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                *tsp = th->th_nanotime;
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 }
 
@@ -626,7 +642,9 @@ getmicrotime(struct timeval *tvp)
        do {
                th = atomic_load_consume(&timehands);
                gen = th->th_generation;
+               membar_consumer();
                *tvp = th->th_microtime;
+               membar_consumer();
        } while (gen == 0 || gen != th->th_generation);
 }