On Fri, Mar 25, 2016 at 11:40 PM, Taylor R Campbell
<campbell+netbsd-tech-k...@mumble.net> wrote:
> Date: Wed, 23 Mar 2016 20:03:54 +0900
> From: Ryota Ozaki <ozaki.ry...@gmail.com>
>
> ...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
