Date: Thu, 4 Feb 2016 17:58:02 +0900
From: Ryota Ozaki <ozak...@netbsd.org>
So here is latest patches that apply the above fixes:
http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq3-diff.diff
http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq3.diff
Some little notes:
--- a/sys/dev/pci/ixgbe/ixgbe.c
+++ b/sys/dev/pci/ixgbe/ixgbe.c
@@ -1650,7 +1650,12 @@ ixgbe_legacy_irq(void *arg)
...
+#ifdef __NetBSD__
+ /* Don't run ixgbe_rxeof in interrupt context */
+ more = true;
+#else
more = ixgbe_rxeof(que);
+#endif
Is this necessary for the present patch? It appears that ixgbe will
use the percpuq when ixgbe_rxeof calls if_input, so this shouldn't be
a problem for now. Of course, maybe in the future we shall want to
make more extensive changes for polling or similar, but that's a
separate issue.
--- a/sys/dev/usb/if_upl.c
+++ b/sys/dev/usb/if_upl.c
@@ -300,14 +300,14 @@ upl_attach(device_t parent, device_t self, void *aux)
...
if_attach(ifp);
- if_alloc_sadl(ifp);
+ if_register(ifp);
Should be either if_attach or if_initialize/if_register?
upl(4) seems to have a special if_input routine. In this case, it
looks like you don't need if_percpuq or if_input at all --
ifp->_if_input will go straight to ip_pktq with no ethernet headers.
--- a/sys/net/if.h
+++ b/sys/net/if.h
@@ -947,6 +950,21 @@ int if_mcast_op(ifnet_t *, const unsigned long,
const struct sockaddr *);
...
+struct if_percpuq {
+ struct ifnet *ipq_ifp;
+ void *ipq_si;
+ struct percpu *ipq_ifqs; /* struct ifqueue */
+};
I would make this private to net/if.c -- no need to publish the guts
of if_percpuq at the moment. Just need a forward declaration `struct
if_percpuq;' in net/if.h.
+void if_percpuq_enqueue(struct if_percpuq *, struct mbuf *);
+struct mbuf *
+ if_percpuq_dequeue(struct if_percpuq *);
I don't think these need to be published either for the moment -- they
can be static in net/if.c.
Another note on the API that occurred to me while reviewing: I've had
to determine whether a packet goes via a per-CPU queue and softint or
goes directly into, e.g., ether_input, based not on the call site
where that happens, but on whether the device did if_attach or
if_initialize/if_register. This is relevant because the context of
the if_input call site determines whether it makes sense to defer to a
softint or go directly into the network stack.
It might be better to have two separate routines, say if_input and
if_schedule_input, with a KASSERT(ifp->ifp_percpuq != NULL) in
if_schedule_input. On the other hand, that's hard to compile-test
unless we also push the percpuq into each driver's softc and require
the caller to pass it to if_schedule_input. So maybe we should put
that off until then and just have callers use if_percpuq_enqueue.
