On Tue, Mar 29, 2016 at 12:27 AM, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> wrote: > 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
Yes. So I thought I need to use a spin mutex if I want to put bridge_release_member inside the pserialize read section. > -- but it doesn't look > like it actually needs to acquire that lock. Hm, I'm confused. I added the lock because convar(9) says: The mutex passed to the wait function (mtx) must also be held when calling cv_broadcast(). Isn't it correct? > > 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!) Sure! Thanks, ozaki-r
