On Mon, Feb 1, 2016 at 2:41 PM, Ryota Ozaki <ozak...@netbsd.org> wrote: > On Fri, Jan 29, 2016 at 11:58 PM, Taylor R Campbell > <campbell+netbsd-tech-k...@mumble.net> wrote: >> Date: Fri, 29 Jan 2016 12:44:07 +0900 >> From: Ryota Ozaki <ozak...@netbsd.org> >> >> So here is a complete patch: >> http://www.netbsd.org/~ozaki-r/softint-if_input-full.diff >> >> With the patch if_input and above should not run in HW >> interrupt anymore. >> >> Instead of adding fields to the already overpopulated struct ifnet, >> how about creating a new struct if_percpuq or something which drivers >> can voluntarily use if they desire? > > While I agree on providing a separate API and letting all drivers use > it finally, I don't agree on applying it to all drivers that need it > at this point. There are so many driver that we have to let use > softint-based if_input and they are much more than ones we don't need > to do. (I'm assuming your proposal needs to add if_percpuq to every > drivers' sc. Am I correct?) My patch intended to minimize the changes > to the drivers. > > So I think it's better to adopt two approaches: > - Add if_percpuq to ifnet and let drivers use it by default (via if_attach) > - Provide a new API that you propose, and let drivers that are ready > for softint use it (we use if_initialize instead) > - Change drivers to use the new API one by one (it would take long time) > - Once we migrate all drivers, then remove if_percpuq from ifnet > (I'm assuming if_percpuq is in ifdef _KERNEL and removable) > > Does this approach satisfy you (or not)?
http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq.diff This is a patch that uses if_percpuq. There is no actual user of it yet. I'm trying to apply it to wm. ozaki-r > >> >> struct if_percpuq { >> void *ipq_si; >> struct percpu *ipq_ifqs; /* struct ifqueue */ >> }; >> >> struct if_percpuq * >> if_percpuq_create(void (*)(void *), void *); >> void if_percpuq_destroy(struct if_percpuq *); >> void if_percpuq_enqueue(struct if_percpuq *, struct mbuf *); > > if_percpuq_create takes the same argument as softint_establish > and the first argument may be a driver-specific if_input function > or a some common if_percpuq_softint? > > I'm a bit worried if we have driver-specific if_input functions, > bpf_mtap hooks will be still scattered. > >> >> To check at compile-time to make sure all callers have been converted >> to the softint-only ifp->if_input, we could change its signature to >> add a dummy argument, or rename the member. The signature/name of >> ifp->if_input is also closer to the real change -- forbidding *_input >> in hardintr context -- than the signature of if_initialize. > > It may be good. I think ifp->_if_input is a candidate alike rtentry's > _rt_key. > >> >> The reason I suggest a separate API which drivers can voluntarily use >> is that I expect we may want alternatives -- e.g., drivers without >> multiqueue support may want to use a pktq instead and automatically >> distribute to other CPUs[*]. > > Do you assume we'll have another API such as if_pktq_*? > >> I've also never liked initialization >> flags like IF_INPUTF_NO_SOFTINT to suppress a newly-default option. > > I introduced the inversion flag because there are less users of the > flag than users of the default; it reduces the number of places I > have to modify. Anyway I'll change to use a separated API instead of > the flag. > >> >> Also: I think your patch is broken as is for all USB NICs -- all their >> `interrupt' routines actually run in softint context, if I'm not >> mistaken, so this kassert will fire: >> >> +void >> +if_input(struct ifnet *ifp, struct mbuf *m) >> +{ >> + >> + if (ifp->if_input_ifqs) { >> + struct ifqueue *ifq = percpu_getref(ifp->if_input_ifqs); >> + >> + KASSERT(cpu_intr_p()); >> >> The same may go for some other NICs, such as se(4), for which there is >> a possible use from thread context which I can't rule out in thirty >> seconds. Another possible case is xennet(4). > > I didn't know them... I'll fix them. > > BTW I shouldn't put KASSERT to warn constraint violations > and instead we should log once (by RUN_ONCE or something)? > >> >> >> [*] Currently this happens (or could happen) with ip_pktq anyway, but >> if I recall correctly, on discussion, rmind@ agreed with me that >> that's the wrong place and because of variation in hardware support >> for rx multiqueue or hashing flows, it should be the NIC driver's job >> to decide how to distribute packets -- even if that means explicitly >> calling a software abstraction to make the decision when the NIC can't >> do rx multiqueue or hash flows itself. > > I agree too. > > > Thanks, > ozaki-r
