On Mon, Jan 04, 2021 at 01:46:43AM +0100, Klemens Nanni wrote:
> On Tue, Dec 29, 2020 at 11:18:26PM +0100, Claudio Jeker wrote:
> > 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.
> Here's a diff that removes the kernel lock and calls input routines
> directly instead of (de)queuing through netisr.
>
> Previously, if_netisr() handled the net lock around those calls, now
> if_input_process() does it before calling ether_input(), so no need to
> add or remove NET_*LOCK() anywhere.
>
> I'm running this on my home router without any regression so far.
>
> Feedback? Objections? OK?
> NB: I want to commit this separately modulo the previous diff.
>
Looks good to me. OK claudio@
> 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 3 Jan 2021 21:38:57 -0000
> @@ -68,7 +68,6 @@
> #include "pf.h"
> #include "pfsync.h"
> #include "ppp.h"
> -#include "pppoe.h"
> #include "switch.h"
> #include "if_wg.h"
>
> @@ -902,13 +901,6 @@ if_netisr(void *unused)
> if (n & (1 << NETISR_SWITCH)) {
> KERNEL_LOCK();
> switchintr();
> - KERNEL_UNLOCK();
> - }
> -#endif
> -#if NPPPOE > 0
> - if (n & (1 << NETISR_PPPOE)) {
> - KERNEL_LOCK();
> - pppoeintr();
> KERNEL_UNLOCK();
> }
> #endif
> Index: if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.267
> diff -u -p -r1.267 if_ethersubr.c
> --- if_ethersubr.c 1 Oct 2020 05:14:10 -0000 1.267
> +++ if_ethersubr.c 3 Jan 2021 21:41:17 -0000
> @@ -532,9 +532,9 @@ ether_input(struct ifnet *ifp, struct mb
> }
> #endif
> if (etype == ETHERTYPE_PPPOEDISC)
> - niq_enqueue(&pppoediscinq, m);
> + pppoe_disc_input(m);
> else
> - niq_enqueue(&pppoeinq, m);
> + pppoe_data_input(m);
> return;
> #endif
> #ifdef MPLS
> Index: if_pppoe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.75
> diff -u -p -r1.75 if_pppoe.c
> --- if_pppoe.c 30 Dec 2020 13:18:07 -0000 1.75
> +++ if_pppoe.c 4 Jan 2021 00:14:30 -0000
> @@ -143,14 +143,8 @@ struct pppoe_softc {
> struct timeval sc_session_time; /* [N] time the session was established
> */
> };
>
> -/* incoming traffic will be queued here */
> -struct niqueue pppoediscinq = NIQUEUE_INITIALIZER(IFQ_MAXLEN, NETISR_PPPOE);
> -struct niqueue pppoeinq = NIQUEUE_INITIALIZER(IFQ_MAXLEN, NETISR_PPPOE);
> -
> /* input routines */
> -static void pppoe_disc_input(struct mbuf *);
> static void pppoe_dispatch_disc_pkt(struct mbuf *);
> -static void pppoe_data_input(struct mbuf *);
>
> /* management routines */
> void pppoeattach(int);
> @@ -341,21 +335,6 @@ pppoe_find_softc_by_hunique(u_int8_t *to
> return (sc);
> }
>
> -/* Interface interrupt handler routine. */
> -void
> -pppoeintr(void)
> -{
> - struct mbuf *m;
> -
> - NET_ASSERT_LOCKED();
> -
> - while ((m = niq_dequeue(&pppoediscinq)) != NULL)
> - pppoe_disc_input(m);
> -
> - while ((m = niq_dequeue(&pppoeinq)) != NULL)
> - pppoe_data_input(m);
> -}
> -
> /* Analyze and handle a single received packet while not in session state. */
> static void
> pppoe_dispatch_disc_pkt(struct mbuf *m)
> @@ -649,7 +628,7 @@ done:
> }
>
> /* Input function for discovery packets. */
> -static void
> +void
> pppoe_disc_input(struct mbuf *m)
> {
> /* avoid error messages if there is not a single pppoe instance */
> @@ -661,7 +640,7 @@ pppoe_disc_input(struct mbuf *m)
> }
>
> /* Input function for data packets */
> -static void
> +void
> pppoe_data_input(struct mbuf *m)
> {
> struct pppoe_softc *sc;
> Index: if_pppoe.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppoe.h,v
> retrieving revision 1.6
> diff -u -p -r1.6 if_pppoe.h
> --- if_pppoe.h 10 Apr 2015 13:58:20 -0000 1.6
> +++ if_pppoe.h 3 Jan 2021 21:46:11 -0000
> @@ -66,10 +66,8 @@ struct pppoeconnectionstate {
>
> #ifdef _KERNEL
>
> -extern struct niqueue pppoediscinq;
> -extern struct niqueue pppoeinq;
> -
> -void pppoeintr(void);
> +void pppoe_disc_input(struct mbuf *);
> +void pppoe_data_input(struct mbuf *);
>
> #endif /* _KERNEL */
> #endif /* _NET_IF_PPPOE_H_ */
> Index: netisr.h
> ===================================================================
> RCS file: /cvs/src/sys/net/netisr.h,v
> retrieving revision 1.53
> diff -u -p -r1.53 netisr.h
> --- netisr.h 6 Aug 2020 12:00:46 -0000 1.53
> +++ netisr.h 3 Jan 2021 21:38:37 -0000
> @@ -45,7 +45,6 @@
> #define NETISR_ARP 18 /* same as AF_LINK */
> #define NETISR_PPP 28 /* for PPP processing */
> #define NETISR_BRIDGE 29 /* for bridge processing */
> -#define NETISR_PPPOE 30 /* for pppoe processing */
> #define NETISR_SWITCH 31 /* for switch dataplane */
>
> #ifndef _LOCORE
--
:wq Claudio
