Re: CVS commit: src/sys/net

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Apr 11, 2016 at 12:31 PM, Taylor R Campbell
<campbell+netbsd-source-changes-d%mumble.net@localhost> wrote:
>    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?

Yes. It passes ATF tests (that enable 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!

Oh yes. I think I already forgot bridgestp.c at all!

>
>    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.

Just because it's too long (for me) to put it in the condition.

>
>    @@ -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.

I see. (I should be claimed RTFM :-/)

>
> 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

Hm, so we need to choose a macro not what we do in the loop but how the loop
is protected. Indeed in terms of use of membar.

Thanks for reviewing!
  ozaki-r