Re: CVS commit: src/sys/net

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

   Date: Mon, 11 Apr 2016 02:04:14 +0000
   From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>

   Module Name:    src
   Committed By:   ozaki-r
   Date:           Mon Apr 11 02:04:14 UTC 2016

   Modified Files:
           src/sys/net: bridgestp.c if_bridge.c if_bridgevar.h

   Log Message:
   Use pslist(9) in bridge(4)

   This adds missing memory barriers to list operations for pserialize.

Nice, thanks for doing this!  Have you tried subjecting it to load,
with options DIAGNOSTIC?

   Index: src/sys/net/bridgestp.c
   diff -u src/sys/net/bridgestp.c:1.19 src/sys/net/bridgestp.c:1.20
   --- src/sys/net/bridgestp.c:1.19        Mon Feb 15 01:11:41 2016
   +++ src/sys/net/bridgestp.c     Mon Apr 11 02:04:14 2016
   @@ -341,7 +341,8 @@ bstp_config_bpdu_generation(struct bridg
    {
           struct bridge_iflist *bif;

   -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
   +       PSLIST_READER_FOREACH(bif, &sc->sc_iflist, struct bridge_iflist,
   +           bif_next) {

I'm pretty sure everything in bridgestp.c runs under a writer lock, so
you should use PSLIST_WRITER_FOREACH here, not PSLIST_READER_FOREACH.

(PSLIST_WRITER_FOREACH is faster on alpha -- but more important is the
statement of intent that this is in an exclusive write section, not a
shared read section.)

   @@ -828,7 +833,8 @@ bstp_initialization(struct bridge_softc 

           BRIDGE_LOCK(sc);

   -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
   +       PSLIST_READER_FOREACH(bif, &sc->sc_iflist, struct bridge_iflist,
   +           bif_next) {

For example, this one is definitely done under a writer lock!

   Index: src/sys/net/if_bridge.c
   diff -u src/sys/net/if_bridge.c:1.111 src/sys/net/if_bridge.c:1.112
   --- src/sys/net/if_bridge.c:1.111       Mon Mar 28 04:38:04 2016
   +++ src/sys/net/if_bridge.c     Mon Apr 11 02:04:14 2016
   @@ -447,8 +447,14 @@ bridge_clone_destroy(struct ifnet *ifp)
           bridge_stop(ifp, 1);

           BRIDGE_LOCK(sc);
   -       while ((bif = LIST_FIRST(&sc->sc_iflist)) != NULL)
   +       for (;;) {
   +               bif = PSLIST_WRITER_FIRST(&sc->sc_iflist, struct bridge_iflist,
   +                   bif_next);
   +               if (bif == NULL)
   +                       break;
                   bridge_delete_member(sc, bif);
   +       }
   +       PSLIST_DESTROY(&sc->sc_iflist);

Any particular reason you switched from `while ((bif = ...) != NULL)'
to `for (;;) { bif = ...; if (bif == NULL) break;'?   I find the while
construct clearer, myself.

   @@ -705,7 +712,8 @@ bridge_delete_member(struct bridge_softc
           ifs->if_bridge = NULL;
           ifs->if_bridgeif = NULL;

   -       LIST_REMOVE(bif, bif_next);
   +       PSLIST_WRITER_REMOVE(bif, bif_next);
   +       PSLIST_ENTRY_DESTROY(bif, bif_next);

           BRIDGE_PSZ_PERFORM(sc);

This order is incorrect.  You cannot destroy the entry until you have
confirmed all readers are done with it.  You must pserialize_perform
before doing PSLIST_ENTRY_DESTROY.  See the note in the pslist(9) man
page about this:

`Either _element_ must never have been inserted into a list, or it
must have been inserted and removed, and the caller must have waited
for all parallel readers to finish reading it first.'

The reason is that PSLIST_WRITER_REMOVE only changes the next pointer
of the *previous* element so that no new readers can discover the
current element, but leaves the next pointer of the *current* element
intact so that readers that are still active can get to the next
element.  Then PSLIST_ENTRY_DESTROY nullifies the next pointer of the
current element and kasserts that you removed the entry.

Maybe PSLIST_ENTRY_DESTROY should really set it to some non-null
poison pointer like QUEUEDEBUG does, so that anyone trying to use it
afterward will actually crash instead of just thinking they hit the
end of the list.

   @@ -939,7 +949,8 @@ bridge_ioctl_gifs(struct bridge_softc *s
    retry:
           BRIDGE_LOCK(sc);
           count = 0;
   -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next)
   +       PSLIST_READER_FOREACH(bif, &sc->sc_iflist, struct bridge_iflist,
   +           bif_next)

Use PSLIST_WRITER_FOREACH here too.

   @@ -959,7 +970,8 @@ retry:
           BRIDGE_LOCK(sc);

           i = 0;
   -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next)
   +       PSLIST_READER_FOREACH(bif, &sc->sc_iflist, struct bridge_iflist,
   +           bif_next)

PSLIST_WRITER_FOREACH

                   i++;
           if (i > count) {
                   /*
   @@ -972,7 +984,8 @@ retry:
           }

           i = 0;
   -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
   +       PSLIST_READER_FOREACH(bif, &sc->sc_iflist, struct bridge_iflist,
   +           bif_next) {

PSLIST_WRITER_FOREACH