Re: RFC: softint-based if_input

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

On Mon, Feb 1, 2016 at 2:41 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> 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)?

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