On Fri, Feb 5, 2016 at 2:32 AM, Taylor R Campbell
<campbell+netbsd-tech-k...@mumble.net> wrote:
> Date: Thu, 4 Feb 2016 17:58:02 +0900
> From: Ryota Ozaki <ozak...@netbsd.org>
>
> 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
The patch also includes fixes for if_cnmac.c pointed out in another
email and a fix to make if_percpuq_destroy safe when it is called
directly from a driver.
Thanks,
ozaki-r
