On Wed, Dec 21, 2016 at 00:13 +0100, Alexander Bluhm wrote: > On Tue, Dec 20, 2016 at 06:47:31PM +0100, Mike Belopuhov wrote: > > @@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name) > > s = splnet(); > > if_down(ifp); > > splx(s); > > } > > > > - return ((*ifc->ifc_destroy)(ifp)); > > + /* XXXSMP breaks atomicity */ > > + rw_exit_write(&netlock); > > + ret = (*ifc->ifc_destroy)(ifp); > > + rw_enter_write(&netlock); > > + > > + return (ret); > > } > > This broke pflow again. > > panic: netlock: lock not held > Stopped at Debugger+0x7: leave > TID PID UID PRFLAGS PFLAGS CPU COMMAND > *440943 4323 0 0x2 0 1 ifconfig > Debugger(d09facfd,f57a2ca8,d09d23c6,f57a2ca8,d76fd000) at Debugger+0x7 > panic(d09d23c6,d09dc32f,f57a2cec,d76fd000,0) at panic+0x71 > rw_assert_wrlock(d0b56f38,40,f57a2cec,d03e506b,d8e1eb00) at > rw_assert_wrlock+0x > 3d > rw_exit_write(d0b56f38,0,7a2d4c,d046b949,d76fd000) at rw_exit_write+0x19 > pflow_clone_destroy(d76fd000,0,c0186943,d055d8ed,d96ad7a0) at > pflow_clone_destr > oy+0x71 > if_clone_destroy(f57a2e74,0,d76fd000,1,d76fd000) at if_clone_destroy+0x7b > ifioctl(d9587648,80206979,f57a2e74,d97e22d8,d956d26c) at ifioctl+0x1ed > soo_ioctl(d96405a8,80206979,f57a2e74,d97e22d8,d97b1f64) at soo_ioctl+0x21c > sys_ioctl(d97e22d8,f57a2f5c,f57a2f7c,0,f57a2fa8) at sys_ioctl+0x19f > syscall() at syscall+0x250 > --- syscall (number 9) --- > 0x14: > https://www.openbsd.org/ddb.html describes the minimum info required in bug > reports. Insufficient info makes it difficult to find and fix bugs. > ddb{1}> > > Now we have two conflicting XXXSMP workarounds in if_clone_destroy() > and pflow_clone_destroy(). > > bluhm
With the diff above, the workaround in pflow_clone_destroy is not needed. The diff below fixes this for me. However, I start doubting this approach. A lot of code is now run with the lock released. While I agree that the amount of lock recursions should be minimized, I don't think that eliminating them completely is a necessity at this point. Recursive rwlock would work just fine. We can add a flag to make rrw_enter fail to recurse for the purpose of minimizing the amount of recursions. Haesbaert has experimented extensively with (r)rwlocks instead of KERNEL_LOCKs (which is what we're doing, sort of) and found out (r)rwlocks to be about 25% slower in real life applications (make build) and produce a ton of IPI calls due to sleep/wakeup cycles. He then moved on to turnstile exclusive recursive locks (bmtx) [1] with better semantics to regain performance and lower down the number of IPIs. Anyways, OK for the diff below? [1] https://github.com/bitrig/bitrig/blob/smpns/sys/kern/kern_bmtx.c diff --git sys/net/if_pflow.c sys/net/if_pflow.c index 0df0b69fd8a..8592d98a743 100644 --- sys/net/if_pflow.c +++ sys/net/if_pflow.c @@ -265,14 +265,11 @@ pflow_clone_destroy(struct ifnet *ifp) if (timeout_initialized(&sc->sc_tmo_tmpl)) timeout_del(&sc->sc_tmo_tmpl); pflow_flush(sc); m_freem(sc->send_nam); if (sc->so != NULL) { - /* XXXSMP breaks atomicity */ - rw_exit_write(&netlock); error = soclose(sc->so); - rw_enter_write(&netlock); sc->so = NULL; } if (sc->sc_flowdst != NULL) free(sc->sc_flowdst, M_DEVBUF, sc->sc_flowdst->sa_len); if (sc->sc_flowsrc != NULL)
