Re: RFC: softint-based if_input

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Fri, Jan 29, 2016 at 11:58 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Fri, 29 Jan 2016 12:44:07 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    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