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