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

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   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!)

   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.

   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?

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

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.