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

[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:
>    Date: Thu, 14 Apr 2016 15:55:45 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    I prepared two patches for bridge(4):
>      http://www.netbsd.org/~ozaki-r/remove-BRIDGE_MPSAFE.diff
>      http://www.netbsd.org/~ozaki-r/psref-bridge.diff
>
>    The former removes BRIDGE_MPSAFE switch and enables the
>    MP-safe code by default. After introducing softint-based
>    if_input, bridge can run in parallel even without NET_MPSAFE,
>    so I think we need to always enable it.
>
> Nice!  Some comments on the patch below.
>
>    The latter applies psref(9) to bridge. I confirmed the new
>    implementation survives load test, which includes repeating
>    bridge creations/deletions and member interface
>    additions/removals, over several hours.
>
> One test it would be nice to see is something that fails due to the
> bug of attempting to use a list element after bridge_release_member,
> outside a pserialize read section, and then does not fail with psref.
> Maybe you could add a busy-wait delay after the bridge_release_member
> to exacerbate it.  (However, it may be hard to actually cause this to
> happen, so I won't demand that you do it before committing!)

Sure. We need some negative tests. And also we want tests
that wait for readers to release references in psref_target_destroy,
which rarely happens.

>
>    Note that I notice that we need to tweak shmif of the
>    rump kernel because it seems that the interrupt handler
>    of shmif can migration between CPUs and so the behavior
>    violates a contract of psref. We can fix it by applying
>    if_percpuq to shmif, but I'm not sure that's the way
>    to go. (I'll ask pooka about the issue.)
>
> 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.

>
>    diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
>    index 6023061..86ca7c3 100644
>    --- a/sys/net/if_bridge.c
>    +++ b/sys/net/if_bridge.c
>    @@ -395,6 +400,7 @@ bridge_clone_create(struct if_clone *ifc, int unit)
>            sc->sc_bridge_priority = BSTP_DEFAULT_BRIDGE_PRIORITY;
>            sc->sc_hold_time = BSTP_DEFAULT_HOLD_TIME;
>            sc->sc_filter_flags = 0;
>    +       sc->sc_dying = false;
>
> Why do you add a dying flag here?  If there's a problem of
> coordinating *_clone_destroy and *_ioctl, it seems to me that that's
> something that should be handled higher up.
>
> 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?

Hmm, right. It's still racy.

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.

>
>    @@ -407,10 +413,14 @@ bridge_clone_create(struct if_clone *ifc, int unit)
>    [...]
>    +       sc->sc_iflist_psref.bip_class =
>    +           psref_class_create("if_bridge_cv", IPL_SOFTNET);
>
> The wmesg here should be only 8 characters long (and preferably make
> the first 5 or 6 the important ones).  Maybe just `bridge' or
> `bridgeif'.
>
> Also, I would suggest that you make the psref class be global, not
> per-bridge.  The main reason for having different psref classes in the
> first place is for different IPLs.  There is conceivably a tiny
> performance benefit for destruction -- but not likely to matter much.

Okay. Changed to have just one class for bridges.

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

Thanks,
  ozaki-r