tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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



   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.

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


Home | Main Index | Thread Index | Old Index