Re: Simplify bridge(4)

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

On Tue, Mar 29, 2016 at 12:27 AM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Mon, 28 Mar 2016 16:07:19 +0900
>    From: 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:
>    > 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