From: Sebastien Marie Subject: Re: pppoe transmit optimisation To: David Gwynne , tech@openbsd.org Date: Wed, 14 May 2025 11:19:07 +0200 Sebastien Marie writes: > David Gwynne writes: > >> this let's packets being sent out pppoe interfaces bypass queues and go >> straight onto the underlying interface. >> >> it's complementary to src/sys/net/if_pppoe.c r1.85 "let pppoe data >> packets go through if_vinput instead of the pppoeinq", but where that >> skipped a queue on the rx side, this diff does it on the tx side. >> >> i don't use pppoe anymore, so i need someone to test this diff before i >> can commiti it. > > it doesn't apply cleanly in my tree (/* $OpenBSD: if_pppoe.c,v 1.86 2025/03/02 21:28:32 bluhm Exp $ */) > > patching file if_pppoe.c > Hunk #1 FAILED at 42. > Hunk #2 succeeded at 160 (offset 4 lines). > Hunk #3 succeeded at 249 with fuzz 2 (offset 17 lines). > Hunk #4 succeeded at 266 with fuzz 2 (offset 18 lines). > Hunk #5 succeeded at 1544 (offset 152 lines). > Hunk #6 FAILED at 1452. > 2 out of 6 hunks FAILED I adapted the diff to match -current, and tested it. But on startup (when starting network interfaces), the kernel panics with the following trace: panic: kernel diagnostic assertion "ifp->if_counters == NULL" failed: file "/sy s/net/if.c", line 2795 Starting stack trace... panic(ffffffff8266389a) at panic+0x175 __assert(ffffffff8261233f,ffffffff8257b628,aeb,ffffffff82609cce) at __assert+0x29 if_counters_alloc(ffff80000071d000) at if_counters_alloc+0x7f pppoe_clone_create(ffffffff829ef910,0) at pppoe_clone_create+0x190 if_clone_create(ffff800042808840,0) at if_clone_create+0xb5 ifioctl(ffff800016e62008,8020697a,ffff800042808840,ffff8000ffff4a68) at ifioctl+0x1ec sys_ioctl(ffff8000ffff4a68,ffff8000428089f0,ffff800042808960) at sys_ioctl+0x301 syscall(ffff8000428089f0) at syscall+0x5ec Xsyscall() at Xsyscall+0x128 end of kernel end trace frame: 0x7cd554e4bd30, count: 248 End of stack trace. Below the used diff (to check if the bug is mine). Thanks. -- Sebastien Marie Commit ID: 78a188743907c943967ca06da1c9b64ce48e1e5e Change ID: ntmxvxtvtlqzrqklosutvqnyzppqrkrm Author : Sebastien Marie (2025-05-14 08:36:59) Committer: Sebastien Marie (2025-05-14 09:03:12) let's packets being sent out pppoe interfaces bypass queues and go straight onto the underlying interface. from dlg@ in id:aCPw6hi1M2/zToMq@animata.net patch updated manually for -current diff --git a/sys/net/if_pppoe.c b/sys/net/if_pppoe.c index b212ac929d..69571439fb 100644 --- a/sys/net/if_pppoe.c +++ b/sys/net/if_pppoe.c @@ -160,6 +160,7 @@ static int pppoe_ioctl(struct ifnet *, unsigned long, caddr_t); static void pppoe_tls(struct sppp *); static void pppoe_tlf(struct sppp *); +static int pppoe_enqueue(struct ifnet *, struct mbuf *); static void pppoe_start(struct ifnet *); /* internal timeout handling */ @@ -248,6 +249,7 @@ sc->sc_sppp.pp_if.if_input = p2p_input; sc->sc_sppp.pp_if.if_bpf_mtap = p2p_bpf_mtap; sc->sc_sppp.pp_if.if_ioctl = pppoe_ioctl; + sc->sc_sppp.pp_if.if_enqueue = pppoe_enqueue; sc->sc_sppp.pp_if.if_start = pppoe_start; sc->sc_sppp.pp_if.if_rtrequest = p2p_rtrequest; sc->sc_sppp.pp_if.if_xflags = IFXF_CLONED; @@ -264,6 +266,7 @@ if_attach(&sc->sc_sppp.pp_if); if_alloc_sadl(&sc->sc_sppp.pp_if); sppp_attach(&sc->sc_sppp.pp_if); + if_counters_alloc(&sc->sc_sppp.pp_if); #if NBPFILTER > 0 bpfattach(&sc->sc_bpf, &sc->sc_sppp.pp_if, DLT_PPP_ETHER, 0); bpfattach(&sc->sc_sppp.pp_if.if_bpf, &sc->sc_sppp.pp_if, @@ -1541,13 +1544,55 @@ timeout_add_msec(&sc->sc_timeout, 20); } +int +pppoe_transmit(struct pppoe_softc *sc, struct mbuf *m) +{ + size_t len; + uint8_t *p; + + len = m->m_pkthdr.len; + M_PREPEND(m, PPPOE_HEADERLEN, M_DONTWAIT); + if (m == NULL) + return (ENOBUFS); + + p = mtod(m, u_int8_t *); + PPPOE_ADD_HEADER(p, 0, sc->sc_session, len); + +#if NBPFILTER > 0 + if (sc->sc_sppp.pp_if.if_bpf) + bpf_mtap(sc->sc_sppp.pp_if.if_bpf, m, BPF_DIRECTION_OUT); +#endif + + return (pppoe_output(sc, m)); +} + +int +pppoe_enqueue(struct ifnet *ifp, struct mbuf *m) +{ + struct pppoe_softc *sc; + int error; + + if (!ifq_is_priq(&ifp->if_snd)) + return (if_enqueue_ifq(ifp, m)); + + sc = ifp->if_softc; + if (sc->sc_state < PPPOE_STATE_SESSION) { + m_freem(m); + return (ENETDOWN); + } + + error = pppoe_transmit(sc, m); + if (error != 0) + counters_inc(ifp->if_counters, ifc_oerrors); + + return (error); +} + static void pppoe_start(struct ifnet *ifp) { struct pppoe_softc *sc = (void *)ifp; struct mbuf *m; - size_t len; - u_int8_t *p; if (sppp_isempty(ifp)) return; @@ -1558,21 +1603,6 @@ return; } - while ((m = sppp_dequeue(ifp)) != NULL) { - len = m->m_pkthdr.len; - M_PREPEND(m, PPPOE_HEADERLEN, M_DONTWAIT); - if (m == NULL) { - ifp->if_oerrors++; - continue; - } - p = mtod(m, u_int8_t *); - PPPOE_ADD_HEADER(p, 0, sc->sc_session, len); - -#if NBPFILTER > 0 - if (sc->sc_bpf) - bpf_mtap(sc->sc_bpf, m, BPF_DIRECTION_OUT); -#endif - - pppoe_output(sc, m); - } + while ((m = sppp_dequeue(ifp)) != NULL) + pppoe_transmit(sc, m); }