> Date: Mon, 18 Apr 2016 10:50:46 +0200
> From: Martin Pieuchot <m...@openbsd.org>
>
> The current goal of the Network SMP effort is to have a single CPU
> process the IP forwarding path in a process context without holding
> the KERNEL_LOCK(). To achieve this goal we're progressively moving
> code from the softnet interrupt context to the if_input_task. In
> the end we'll completely get rid of this soft-interrupt.
>
> So now would be a good time to know if moving all the code currently
> run in a soft-interrupt context to a task uncovers any bug. I'm
> happily running the diff below on amd64 and macppc, it even gives me
> a small performance boost.
>
> I'd appreciate more tests especially on exotic archs.
Yes, please test this. And report back how this changes the results
of your favourite benchmark.
However, this diff uses the same taskq for both the "unlocked" and
"locked" parts. My experience with crypto code was that frequently
grabbing and releasing the kernel lock did affect latency in a bad
way. Some time ago dlg@ had a somewhat similar diff that had a
separate taskq for the "locked" bits. That would avoid blocking of
the "unlocked" bits when the "locked" code is grabbing the kernel
lock. Of course such an approach would probably introduce additional
context switches. Did you consider such an approach?
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.429
> diff -u -p -r1.429 if.c
> --- net/if.c 16 Mar 2016 12:08:09 -0000 1.429
> +++ net/if.c 18 Apr 2016 08:00:08 -0000
> @@ -64,9 +64,12 @@
> #include "bpfilter.h"
> #include "bridge.h"
> #include "carp.h"
> +#include "ether.h"
> #include "pf.h"
> +#include "pfsync.h"
> +#include "ppp.h"
> +#include "pppoe.h"
> #include "trunk.h"
> -#include "ether.h"
>
> #include <sys/param.h>
> #include <sys/systm.h>
> @@ -148,6 +151,7 @@ int if_group_egress_build(void);
> void if_watchdog_task(void *);
>
> void if_input_process(void *);
> +void if_netisr(void *);
>
> #ifdef DDB
> void ifa_print_all(void);
> @@ -216,13 +220,15 @@ void net_tick(void *);
> int net_livelocked(void);
> int ifq_congestion;
>
> -struct taskq *softnettq;
> +int netisr;
> +struct taskq *softnettq;
> +
> +struct mbuf_queue if_input_queue = MBUF_QUEUE_INITIALIZER(8192, IPL_NET);
> +struct task if_input_task = TASK_INITIALIZER(if_input_process,
> &if_input_queue);
> +struct task if_input_task_locked = TASK_INITIALIZER(if_netisr, NULL);
>
> /*
> * Network interface utility routines.
> - *
> - * Routines with ifa_ifwith* names take sockaddr *'s as
> - * parameters.
> */
> void
> ifinit(void)
> @@ -590,9 +596,6 @@ if_enqueue(struct ifnet *ifp, struct mbu
> return (0);
> }
>
> -struct mbuf_queue if_input_queue = MBUF_QUEUE_INITIALIZER(8192, IPL_NET);
> -struct task if_input_task = TASK_INITIALIZER(if_input_process,
> &if_input_queue);
> -
> void
> if_input(struct ifnet *ifp, struct mbuf_list *ml)
> {
> @@ -803,6 +806,50 @@ if_input_process(void *xmq)
> if_put(ifp);
> }
> splx(s);
> +}
> +
> +void
> +if_netisr(void *unused)
> +{
> + int n, t = 0;
> + int s;
> +
> + KERNEL_LOCK();
> + s = splsoftnet();
> +
> + while ((n = netisr) != 0) {
> + sched_pause();
> +
> + atomic_clearbits_int(&netisr, n);
> +
> + if (n & (1 << NETISR_IP))
> + ipintr();
> +#ifdef INET6
> + if (n & (1 << NETISR_IPV6))
> + ip6intr();
> +#endif
> +#if NPPP > 0
> + if (n & (1 << NETISR_PPP))
> + pppintr();
> +#endif
> +#if NBRIDGE > 0
> + if (n & (1 << NETISR_BRIDGE))
> + bridgeintr();
> +#endif
> +#if NPPPOE > 0
> + if (n & (1 << NETISR_PPPOE))
> + pppoeintr();
> +#endif
> + t |= n;
> + }
> +
> +#if NPFSYNC > 0
> + if (t & (1 << NETISR_PFSYNC))
> + pfsyncintr();
> +#endif
> +
> + splx(s);
> + KERNEL_UNLOCK();
> }
>
> void
> Index: net/netisr.c
> ===================================================================
> RCS file: net/netisr.c
> diff -N net/netisr.c
> --- net/netisr.c 8 Jan 2016 13:53:24 -0000 1.10
> +++ /dev/null 1 Jan 1970 00:00:00 -0000
> @@ -1,74 +0,0 @@
> -/*
> - * Copyright (c) 2010 Owain G. Ainsworth <o...@openbsd.org>
> - *
> - * Permission to use, copy, modify, and distribute this software for any
> - * purpose with or without fee is hereby granted, provided that the above
> - * copyright notice and this permission notice appear in all copies.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> - * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> - * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> - * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> - * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> - * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> - * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> - */
> -#include <sys/param.h>
> -#include <sys/systm.h>
> -
> -#include <net/netisr.h>
> -
> -#include <machine/intr.h>
> -
> -#include "ppp.h"
> -#include "bridge.h"
> -#include "pppoe.h"
> -#include "pfsync.h"
> -
> -void netintr(void *);
> -
> -int netisr;
> -void *netisr_intr;
> -
> -void
> -netintr(void *unused)
> -{
> - int n, t = 0;
> -
> - while ((n = netisr) != 0) {
> - atomic_clearbits_int(&netisr, n);
> -
> - if (n & (1 << NETISR_IP))
> - ipintr();
> -#ifdef INET6
> - if (n & (1 << NETISR_IPV6))
> - ip6intr();
> -#endif
> -#if NPPP > 0
> - if (n & (1 << NETISR_PPP))
> - pppintr();
> -#endif
> -#if NBRIDGE > 0
> - if (n & (1 << NETISR_BRIDGE))
> - bridgeintr();
> -#endif
> -#if NPPPOE > 0
> - if (n & (1 << NETISR_PPPOE))
> - pppoeintr();
> -#endif
> - t |= n;
> - }
> -
> -#if NPFSYNC > 0
> - if (t & (1 << NETISR_PFSYNC))
> - pfsyncintr();
> -#endif
> -}
> -
> -void
> -netisr_init(void)
> -{
> - netisr_intr = softintr_establish(IPL_SOFTNET, netintr, NULL);
> - if (netisr_intr == NULL)
> - panic("can't establish softnet handler");
> -}
> Index: net/netisr.h
> ===================================================================
> RCS file: /cvs/src/sys/net/netisr.h,v
> retrieving revision 1.44
> diff -u -p -r1.44 netisr.h
> --- net/netisr.h 8 Jan 2016 13:53:24 -0000 1.44
> +++ net/netisr.h 18 Apr 2016 07:52:28 -0000
> @@ -61,7 +61,12 @@
>
> #ifndef _LOCORE
> #ifdef _KERNEL
> +
> +#include <sys/task.h>
> +#include <sys/atomic.h>
> +
> extern int netisr; /* scheduling bits for network */
> +extern struct task if_input_task_locked;
>
> void ipintr(void);
> void ip6intr(void);
> @@ -70,16 +75,11 @@ void bridgeintr(void);
> void pppoeintr(void);
> void pfsyncintr(void);
>
> -#include <machine/atomic.h>
> -
> -extern void *netisr_intr;
> #define schednetisr(anisr)
> \
> do { \
> atomic_setbits_int(&netisr, (1 << (anisr))); \
> - softintr_schedule(netisr_intr); \
> + task_add(softnettq, &if_input_task_locked); \
> } while (/* CONSTCOND */0)
> -
> -void netisr_init(void);
>
> #endif /* _KERNEL */
> #endif /*_LOCORE */
> Index: kern/init_main.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/init_main.c,v
> retrieving revision 1.249
> diff -u -p -r1.249 init_main.c
> --- kern/init_main.c 19 Mar 2016 12:04:15 -0000 1.249
> +++ kern/init_main.c 18 Apr 2016 07:48:40 -0000
> @@ -394,7 +394,6 @@ main(void *framep)
> * until everything is ready.
> */
> s = splnet();
> - netisr_init();
> domaininit();
> splx(s);
>
>
>
