Re: Simplify bridge(4)

[Ryota Ozaki <ozaki.ryota%gmail.com@localhost>]

On Fri, Mar 25, 2016 at 11:40 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Wed, 23 Mar 2016 20:03:54 +0900
>    From: Ryota Ozaki <ozaki.ryota%gmail.com@localhost>
>
>    ...not so much soon though, I prepared patches:
>      http://www.netbsd.org/~ozaki-r/bridge-psz-list.diff
>      http://www.netbsd.org/~ozaki-r/bridge-default-mpsafe.diff
>
>    The first one introduces membar to list operations used in bridge
>    for pserialize, and the other one removes BRIDGE_MPSAFE switch and
>    make bridge MP-safe by default. The first one should be revisited
>    once we have the official API of list operations for pserialize.
>
> This looks great.  Let's try again to get the pserialize-safe list
> operations in properly -- not having them is obviously getting in the
> way of useful progress.  But I have no objection to putting in local
> definitions in the bridge code for now, as long as we make sure to get
> them into queue.h soon.
>
> The comments below suggest to me that we should perhaps change the
> `_PSZ' suffix, because it does not generically mean `if you're doing
> pserialize, you need to use the _PSZ variant'.  E.g., LIST_FOREACH_PSZ
> is for use by readers -- writers under the exclusive write lock don't
> need it.
>
> We could have
>
> LIST_INSERT_HEAD_WRITER = LIST_INSERT_HEAD with membar_producer
> LIST_REMOVE_WRITER = LIST_REMOVE without QUEUEDEBUG
> LIST_FOREACH_WRITER = LIST_FOREACH
> LIST_FOREACH_READER = LIST_FOREACH with membar_datadep_consumer
>
> so that it's still easy to eyeball whether you're using a
> pserialize-safe thing or not (no suffix? not safe), and then to
> eyeball whether it's the correct one for the context.

Thanks. I changed the names so.

>
> Some comments in-line:
>
>    diff --git a/sys/net/bridgestp.c b/sys/net/bridgestp.c
>    index 01968f9..fa494dc 100644
>    --- a/sys/net/bridgestp.c
>    +++ b/sys/net/bridgestp.c
>    @@ -341,7 +341,7 @@ bstp_config_bpdu_generation(struct bridge_softc *sc)
>     {
>            struct bridge_iflist *bif;
>
>    -       LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
>    +       LIST_FOREACH_PSZ(bif, &sc->sc_iflist, bif_next) {
>
> It looks like all of the bstp code runs under an exclusive lock, so
> there's no need for the _PSZ here.

Sure. Reverted.

>
>    diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
>    index 651e830..57b37c2 100644
>    --- a/sys/net/if_bridge.c
>    +++ b/sys/net/if_bridge.c
>    @@ -433,7 +433,7 @@ bridge_clone_create(struct if_clone *ifc, int unit)
>            if_alloc_sadl(ifp);
>
>            mutex_enter(&bridge_list_lock);
>    -       LIST_INSERT_HEAD(&bridge_list, sc, sc_list);
>    +       LIST_INSERT_HEAD_PSZ(&bridge_list, sc, sc_list);
>            mutex_exit(&bridge_list_lock);
>
>            return (0);
>    @@ -461,7 +461,7 @@ bridge_clone_destroy(struct ifnet *ifp)
>            BRIDGE_UNLOCK(sc);
>
>            mutex_enter(&bridge_list_lock);
>    -       LIST_REMOVE(sc, sc_list);
>    +       LIST_REMOVE_PSZ(sc, sc_list);
>            mutex_exit(&bridge_list_lock);
>
> No need for the _PSZ variants here: the bridge list is used only under
> an exclusive lock.
>
> Actually...  I don't see any users of that list!  Do we need it any
> more?  Why was it there to begin with?
>
> Looks like it was added in the first commit and never used.  Let's get
> rid of it!  (Separate commit.)

Heh, that's true. Removed!

So patches are updated:
  http://www.netbsd.org/~ozaki-r/bridge-psz-list.diff
  http://www.netbsd.org/~ozaki-r/bridge-default-mpsafe.diff


BTW, I found a possible race condition in bridge_output:

    BRIDGE_PSZ_RENTER(s);
    LIST_FOREACH_READER(bif, &sc->sc_iflist, bif_next) {
        bif = bridge_try_hold_bif(bif);
        if (bif == NULL)
            continue;
        BRIDGE_PSZ_REXIT(s);

        // sleepable operations

        bridge_release_member(sc, bif);
        BRIDGE_PSZ_RENTER(s);
    }
    BRIDGE_PSZ_REXIT(s);

bif can be freed after bridge_release_member on another CPU,
however, LIST_FOREACH_READER touches bif after it.

IIUC, using psref solves the issue like this:

    BRIDGE_PSZ_RENTER(s);
    bif = (&sc->sc_iflist)->lh_first;
    while (bif != LIST_END(&sc->sc_iflist)) {
        struct bridge_iflist *next;

        membar_datadep_consumer();

        psref_acquire(&psref, &bif->target);
        BRIDGE_PSZ_REXIT(s);

        // sleepable operations

        BRIDGE_PSZ_RENTER(s);
        next = bif->field.le_next;
        psref_release(&psref, &bif->target);
        bif = next;
    }
    BRIDGE_PSZ_REXIT(s);

So I'm thinking the above patches should be committed
after introducing psref.

Thanks,
  ozaki-r