On 13/09/20(Sun) 15:12, Klemens Nanni wrote: > This is my first try trading global locks for interface specific ones. > > pppoe(4) keeps a list of all its interfaces which is then obviously > traversed during create and destroy. > > Currently, the net lock is grabbed for this, but there seems to be no > justification other than reusing^Wabusing an existing lock. > > I run this diff with WITNESS and kern.witness=2 on my edgerouter 4 > providing my home uplink via pppoe0: the kernel runs stable, there's > not witness log showing up and creating and destroying hundreds of > additional pppoe(4) devices works without disruption. > > Is this the right direction?
I doubt it is, see below: > Index: if_pppoe.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppoe.c,v > retrieving revision 1.73 > diff -u -p -r1.73 if_pppoe.c > --- if_pppoe.c 13 Sep 2020 11:00:40 -0000 1.73 > +++ if_pppoe.c 13 Sep 2020 11:31:12 -0000 > @@ -460,8 +463,10 @@ static void pppoe_dispatch_disc_pkt(stru > err_msg = "TAG HUNIQUE ERROR"; > break; > } > + rw_enter_read(&pppoe_lock); > sc = pppoe_find_softc_by_hunique(mtod(n, caddr_t) + > noff, > len, m->m_pkthdr.ph_ifidx); > + rw_exit_read(&pppoe_lock); This introduce a new sleeping point in the packet processing path, something we are avoiding thanks to the NET_LOCK(). Plus this use of a lock alone is insufficient to guarantee the integrity of `sc' because it is used outside of the dance. The NET_LOCK() is fine here, please concentrate on the KERNEL_LOCK() that's where contention happens.
