Date: Mon, 11 Apr 2016 02:04:14 +0000 From: Ryota Ozaki <ozak...@netbsd.org>
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