On Tue, Dec 29, 2020 at 08:48:28PM +0100, Klemens Nanni wrote:
> Earlier this year `struct pppoe_softc' was annotated with lock comments
> showing no member being protected by KERNEL_LOCK() alone.
>
> After further review of the code paths starting from pppoeintr() I also
> could not find sleeping points, which must be avoided in the sofnet
> thread. (As part of this, I specifically went through all possible
> malloc(9) calls and verified that none of them use `M_WAITOK'.)
>
> The only thing I see neccessary now is wrapping mbuf queue access with
> if_get(9) to prevent interfaces from disappearing while processing the
> mbuf -- currently the big lock protects against that.
>
> I've been running with this diff for over four months on an octeon
> edgerouter 4 which updates to snapshots modulo the custom kernel roughly
> once a week.
>
> Others also successfully tested this on octeon and amd64 based setups.
>
> Did I miss anything?
> Feedback? Objections? OK?
Generally I would prefer to go for direct dispatch and not use netisr.
This removes a queue and a scheduling point and should help reduce the
latency in processing pppoe packages.
This does not mean that I'm against this change. I just think it may be
benefitial to move one step further.
> Index: if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.621
> diff -u -p -r1.621 if.c
> --- if.c 15 Dec 2020 03:43:34 -0000 1.621
> +++ if.c 28 Dec 2020 18:13:02 -0000
> @@ -907,9 +907,7 @@ if_netisr(void *unused)
> #endif
> #if NPPPOE > 0
> if (n & (1 << NETISR_PPPOE)) {
> - KERNEL_LOCK();
> pppoeintr();
> - KERNEL_UNLOCK();
> }
> #endif
> t |= n;
> 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 28 Dec 2020 18:13:02 -0000
> @@ -346,14 +346,29 @@ void
> pppoeintr(void)
> {
> struct mbuf *m;
> + struct ifnet *ifp;
>
> NET_ASSERT_LOCKED();
>
> - while ((m = niq_dequeue(&pppoediscinq)) != NULL)
> + while ((m = niq_dequeue(&pppoediscinq)) != NULL) {
> + ifp = if_get(m->m_pkthdr.ph_ifidx);
> + if (ifp == NULL) {
> + m_freem(m);
> + continue;
> + }
> pppoe_disc_input(m);
> + if_put(ifp);
> + }
>
> - while ((m = niq_dequeue(&pppoeinq)) != NULL)
> + while ((m = niq_dequeue(&pppoeinq)) != NULL) {
> + ifp = if_get(m->m_pkthdr.ph_ifidx);
> + if (ifp == NULL) {
> + m_freem(m);
> + continue;
> + }
> pppoe_data_input(m);
> + if_put(ifp);
> + }
> }
>
> /* Analyze and handle a single received packet while not in session state. */
>
--
:wq Claudio
