Re: RFC: softint-based if_input

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

On Fri, Feb 5, 2016 at 4:49 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Fri, Feb 5, 2016 at 2:32 AM, Taylor R Campbell
> <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>>    Date: Thu, 4 Feb 2016 17:58:02 +0900
>>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>>
>>    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