Re: bridge(4): BRIDGE_MPSAFE by default and applying psref(9)

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Tue, Apr 19, 2016 at 12:27 AM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Mon, 18 Apr 2016 17:06:51 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    On Fri, Apr 15, 2016 at 5:37 AM, Taylor R Campbell
>    <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    > You could make (e.g.) bridge_input always satisfy the contract by
>    > doing
>    >
>    >         int bound = curlwp->l_pflag & LP_BOUND;
>    >         curlwp->l_pflag |= LP_BOUND;
>    >         ...
>    >         curlwp->l_pflag ^= bound ^ LP_BOUND;
>    >
>    > if you can't guarantee that every caller will be in a softint.
>
>    Good idea. It works for bridge.
>
>    Only shmif(4) and virt(4) of the rump kernel are the objectives.
>    Considering that introducing psref to other L2 components
>    (vlan and bpf) in the future, I think it's better to put
>    the LP_BOUND stuffs to shmif and virt.
>
> Sounds good to me.
>
>    > And this protocol seems fishy to me -- what stops bridge_clone_destroy
>    > from freeing a bridge while another CPU is in the middle of
>    > bridge_ioctl_add, after the sc_dying check?
>
>    I wrote a patch to solve the issue by putting ifioctl_detach
>    before calling destruction routines of an interface:
>      http://www.netbsd.org/~ozaki-r/early-ifioctl_detach.diff
>
>    The change should prevent *_clone_destroy and *_ioctl from
>    running in parallel.
>
> Hmm...  I'm rather confused by the ifnet_lock_enter/exit business.
> Why isn't ifnet_lock_enter simply mutex_enter(&il->il_lock) and
> ifnet_lock_exit simply mutex_exit(&il->il_lock)?  I think it's trying
> to do something like psref, using per-CPU-per-object counts instead of
> per-CPU lists -- but it doesn't actually avoid the lock.
>
> We should try to get dyoung@ to look at this.

We will apply psref to struct ifnet for safe interface destruction
at some point. Can we use the psref for ioctl instead of il_lock?

>
>    > bridge_try_hold_bif should perhaps be renamed now that it never fails,
>    > say to bridge_acquire_member -- or maybe we can just remove
>    > bridge_try_hold_bif and bridge_release_member altogether, since
>    > they're just psref_acquire and psref_release.
>
>    I choose bridge_acquire_member for now. I would get rid of
>    bridge_*_member in another step later.
>
>    So new patches are here:
>      http://www.netbsd.org/~ozaki-r/psref-bridge-v2.diff
>      http://www.netbsd.org/~ozaki-r/psref-bridge-v2-diff.diff
>
>    The changes also reflect christos and mrg suggestions.
>
> Looks good!  I don't have time for a thorough review right now, but
> nothing jumped out at me on a quick skim.

Thanks! I committed the patch anyway, although we have to fix the above
issue somehow.

  ozaki-r