Date: Mon, 28 Mar 2016 16:07:19 +0900
From: Ryota Ozaki <ozaki.ry...@gmail.com>
On Fri, Mar 25, 2016 at 11:40 PM, Taylor R Campbell
<campbell+netbsd-tech-k...@mumble.net> wrote:
> 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!
Great!
So patches are updated:
http://www.netbsd.org/~ozaki-r/bridge-psz-list.diff
http://www.netbsd.org/~ozaki-r/bridge-default-mpsafe.diff
Thanks, I'll take a look when I have a chance.
BTW, I found a possible race condition in bridge_output:
[...]
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.
You don't *need* to switch to psref for this to work: it is sufficient
for bridge_release_member to work inside a pserialize read section.
Currently the only reason it doesn't work inside a pserialize read
section is that it acquires an adaptive lock -- but it doesn't look
like it actually needs to acquire that lock.
However, as an aside, I am a little puzzled by the protocol that
bridge_try_hold_bif, bridge_release_member, and bridge_delete_member
follow.
(Also: Use LIST_FIRST and LIST_NEXT, not .lh_first and .le_next!)
