On Fri, Feb 5, 2016 at 4:49 PM, Ryota Ozaki <ozak...@netbsd.org> wrote: > On Fri, Feb 5, 2016 at 2:32 AM, Taylor R Campbell > <campbell+netbsd-tech-k...@mumble.net> wrote: >> 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. > > I forgot though, I intended to treat it as already softint-ified one > (except for hwintr), so what I should do is replacing if_attach with > if_intialize. saitouh-san already confirmed the driver with the above > change works without any noticeable regressions. > >> >> --- 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. > > Yes, it's my fault. I should use if_initialize/if_register. > >> >> --- 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 */ >> +}; > > Sure. I'll tweak it. > >> >> 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. > > if_percpuq_dequeue can be static but if_percpuq_enqueue cannot > if we let drivers use it (wm already does so). > >> >> 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. > > Hmm, I'm not much convinced by this though, I have no strong objection, > so I changed as you suggest. > > Here is a new patch (and a diff) that applies the above fixes: > http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq4.diff > http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq4-diff.diff
nitpick: #include <sys/intr.h> moved from if.h to if.c: http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq5.diff Any other suggestions or comments? ozaki-r
