[src/trunk]: src Adapt <sys/pslist.h> to use atomic_load/store_*.

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

details:   https://anonhg.NetBSD.org/src/rev/6b1b08aaa98e
branches:  trunk
changeset: 465661:6b1b08aaa98e
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sun Dec 01 15:28:19 2019 +0000

description:
Adapt <sys/pslist.h> to use atomic_load/store_*.

Changes:

- membar_producer();
  *p = v;

    =>

  atomic_store_release(p, v);

  (Effectively like using membar_exit instead of membar_producer,
  which is what we should have been doing all along so that stores by
  the `reader' can't affect earlier loads by the writer, such as
  KASSERT(p->refcnt == 0) in the writer and atomic_inc(&p->refcnt) in
  the reader.)

- p = *pp;
  if (p != NULL) membar_datadep_consumer();

    =>

  p = atomic_load_consume(pp);

  (Only makes a difference on DEC Alpha.  As long as lists generally
  have at least one element, this is not likely to make a big
  difference, and keeps the code simpler and clearer.)

No other functional change intended.  While here, annotate each
synchronizing load and store with its counterpart in a comment.

diffstat:

 sys/sys/pslist.h             |  71 ++++++++++++++++++++++++-------------------
 tests/include/sys/t_pslist.c |  21 ++++++++----
 2 files changed, 54 insertions(+), 38 deletions(-)

diffs (202 lines):

diff -r f91be6653c8c -r 6b1b08aaa98e sys/sys/pslist.h
--- a/sys/sys/pslist.h  Sun Dec 01 15:28:02 2019 +0000
+++ b/sys/sys/pslist.h  Sun Dec 01 15:28:19 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: pslist.h,v 1.6 2019/09/20 13:38:00 maxv Exp $  */
+/*     $NetBSD: pslist.h,v 1.7 2019/12/01 15:28:19 riastradh Exp $     */
 
 /*-
  * Copyright (c) 2016 The NetBSD Foundation, Inc.
@@ -65,7 +65,7 @@
 pslist_init(struct pslist_head *head)
 {
 
-       head->plh_first = NULL;
+       head->plh_first = NULL; /* not yet published, so no atomic */
 }
 
 static __inline void
@@ -94,7 +94,7 @@
         * would think they were simply at the end of the list.
         * Instead, cause readers to crash.
         */
-       entry->ple_next = _PSLIST_POISON;
+       atomic_store_relaxed(&entry->ple_next, _PSLIST_POISON);
 }
 
 /*
@@ -119,11 +119,10 @@
        _PSLIST_ASSERT(new->ple_prevp == NULL);
 
        new->ple_prevp = &head->plh_first;
-       new->ple_next = head->plh_first;
+       new->ple_next = head->plh_first; /* not yet published, so no atomic */
        if (head->plh_first != NULL)
                head->plh_first->ple_prevp = &new->ple_next;
-       membar_producer();
-       head->plh_first = new;
+       atomic_store_release(&head->plh_first, new);
 }
 
 static __inline void
@@ -138,9 +137,14 @@
        _PSLIST_ASSERT(new->ple_prevp == NULL);
 
        new->ple_prevp = entry->ple_prevp;
-       new->ple_next = entry;
-       membar_producer();
-       *entry->ple_prevp = new;
+       new->ple_next = entry;  /* not yet published, so no atomic */
+
+       /*
+        * Pairs with atomic_load_consume in pslist_reader_first or
+        * pslist_reader_next.
+        */
+       atomic_store_release(entry->ple_prevp, new);
+
        entry->ple_prevp = &new->ple_next;
 }
 
@@ -156,11 +160,12 @@
        _PSLIST_ASSERT(new->ple_prevp == NULL);
 
        new->ple_prevp = &entry->ple_next;
-       new->ple_next = entry->ple_next;
+       new->ple_next = entry->ple_next; /* not yet published, so no atomic */
        if (new->ple_next != NULL)
                new->ple_next->ple_prevp = &new->ple_next;
-       membar_producer();
-       entry->ple_next = new;
+
+       /* Pairs with atomic_load_consume in pslist_reader_next.  */
+       atomic_store_release(&entry->ple_next, new);
 }
 
 static __inline void
@@ -173,7 +178,15 @@
 
        if (entry->ple_next != NULL)
                entry->ple_next->ple_prevp = entry->ple_prevp;
-       *entry->ple_prevp = entry->ple_next;
+
+       /*
+        * No need for atomic_store_release because there's no
+        * initialization that this must happen after -- the store
+        * transitions from a good state with the entry to a good state
+        * without the entry, both of which are valid for readers to
+        * witness.
+        */
+       atomic_store_relaxed(entry->ple_prevp, entry->ple_next);
        entry->ple_prevp = NULL;
 
        /*
@@ -221,29 +234,30 @@
 /*
  * Reader operations.  Caller must block pserialize_perform or
  * equivalent and be bound to a CPU.  Only plh_first/ple_next may be
- * read, and after that, a membar_datadep_consumer must precede
- * dereferencing the resulting pointer.
+ * read, and only with consuming memory order so that data-dependent
+ * loads happen afterward.
  */
 
 static __inline struct pslist_entry *
 pslist_reader_first(const struct pslist_head *head)
 {
-       struct pslist_entry *first = head->plh_first;
-
-       if (first != NULL)
-               membar_datadep_consumer();
-
-       return first;
+       /*
+        * Pairs with atomic_store_release in pslist_writer_insert_head
+        * or pslist_writer_insert_before.
+        */
+       return atomic_load_consume(&head->plh_first);
 }
 
 static __inline struct pslist_entry *
 pslist_reader_next(const struct pslist_entry *entry)
 {
-       struct pslist_entry *next = entry->ple_next;
+       /*
+        * Pairs with atomic_store_release in
+        * pslist_writer_insert_before or pslist_writer_insert_after.
+        */
+       struct pslist_entry *next = atomic_load_consume(&entry->ple_next);
 
        _PSLIST_ASSERT(next != _PSLIST_POISON);
-       if (next != NULL)
-               membar_datadep_consumer();
 
        return next;
 }
@@ -252,12 +266,10 @@
 _pslist_reader_first_container(const struct pslist_head *head,
     const ptrdiff_t offset)
 {
-       struct pslist_entry *first = head->plh_first;
+       struct pslist_entry *first = pslist_reader_first(head);
 
        if (first == NULL)
                return NULL;
-       membar_datadep_consumer();
-
        return (char *)first - offset;
 }
 
@@ -265,13 +277,10 @@
 _pslist_reader_next_container(const struct pslist_entry *entry,
     const ptrdiff_t offset)
 {
-       struct pslist_entry *next = entry->ple_next;
+       struct pslist_entry *next = pslist_reader_next(entry);
 
-       _PSLIST_ASSERT(next != _PSLIST_POISON);
        if (next == NULL)
                return NULL;
-       membar_datadep_consumer();
-
        return (char *)next - offset;
 }
 
diff -r f91be6653c8c -r 6b1b08aaa98e tests/include/sys/t_pslist.c
--- a/tests/include/sys/t_pslist.c      Sun Dec 01 15:28:02 2019 +0000
+++ b/tests/include/sys/t_pslist.c      Sun Dec 01 15:28:19 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: t_pslist.c,v 1.1 2016/04/09 04:39:47 riastradh Exp $   */
+/*     $NetBSD: t_pslist.c,v 1.2 2019/12/01 15:28:20 riastradh Exp $   */
 
 /*-
  * Copyright (c) 2016 The NetBSD Foundation, Inc.
@@ -29,16 +29,23 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
+/*
+ * XXX This is a limited test to make sure the operations behave as
+ * described on a sequential machine.  It does nothing to test the
+ * pserialize-safety of any operations.  The following definitions must
+ * be replaced in any parallel tests.
+ */
+
+#define        atomic_load_relaxed(p)          (*(p))
+#define        atomic_load_acquire(p)          (*(p))
+#define        atomic_load_consume(p)          (*(p))
+#define        atomic_store_relaxed(p,v)       (*(p) = (v))
+#define        atomic_store_release(p,v)       (*(p) = (v))
+
 #include <sys/pslist.h>
 
 #include <atf-c.h>
 
-/*
- * XXX This is a limited test to make sure the operations behave as
- * described on a sequential machine.  It does nothing to test the
- * pserialize-safety of any operations.
- */
-
 ATF_TC(misc);
 ATF_TC_HEAD(misc, tc)
 {