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