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)?
>
> 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
