| Message ID | 20190515080319.15514-2-sthemmin@microsoft.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | netvsc fixes to VF frame handling | expand |
On 2019/5/15 下午4:03, Stephen Hemminger wrote: > XDP generic does not work correctly with the Hyper-V/Azure netvsc > device because of packet processing order. Only packets on the > synthetic path get seen by the XDP program. The VF device packets > are not seen. > > By the time the packets that arrive on the VF are handled by > netvsc after the first pass of XDP generic (on the VF) has already > been done. > > A fix for the netvsc device is to do this in the VF packet handler. > by directly calling do_xdp_generic() if XDP program is present > on the parent device. > > A riskier but maybe better alternative would be to do this netdev core > code after the receive handler is invoked (if RX_HANDLER_ANOTHER > is returned). Something like what I propose at https://lore.kernel.org/patchwork/patch/973819/ ? It belongs to a series that try to make XDP (both native and generic) work for stacked device. But for some reason (probably performance), the maintainer seems not like the idea. Maybe it's time to reconsider that? Thanks > > Fixes: 0c195567a8f6 ("netvsc: transparent VF management") > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > --- > drivers/net/hyperv/netvsc_drv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 06393b215102..bb0fc1869bde 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -1999,9 +1999,15 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct sk_buff **pskb) > struct net_device_context *ndev_ctx = netdev_priv(ndev); > struct netvsc_vf_pcpu_stats *pcpu_stats > = this_cpu_ptr(ndev_ctx->vf_stats); > + struct bpf_prog *xdp_prog; > > skb->dev = ndev; > > + xdp_prog = rcu_dereference(ndev->xdp_prog); > + if (xdp_prog && > + do_xdp_generic(xdp_prog, skb) != XDP_PASS) > + return RX_HANDLER_CONSUMED; > + > u64_stats_update_begin(&pcpu_stats->syncp); > pcpu_stats->rx_packets++; > pcpu_stats->rx_bytes += skb->len;
On Wed, 15 May 2019 16:12:42 +0800 Jason Wang <jasowang@redhat.com> wrote: > On 2019/5/15 下午4:03, Stephen Hemminger wrote: > > XDP generic does not work correctly with the Hyper-V/Azure netvsc > > device because of packet processing order. Only packets on the > > synthetic path get seen by the XDP program. The VF device packets > > are not seen. > > > > By the time the packets that arrive on the VF are handled by > > netvsc after the first pass of XDP generic (on the VF) has already > > been done. > > > > A fix for the netvsc device is to do this in the VF packet handler. > > by directly calling do_xdp_generic() if XDP program is present > > on the parent device. > > > > A riskier but maybe better alternative would be to do this netdev core > > code after the receive handler is invoked (if RX_HANDLER_ANOTHER > > is returned). > > > Something like what I propose at > https://lore.kernel.org/patchwork/patch/973819/ ? > > It belongs to a series that try to make XDP (both native and generic) > work for stacked device. But for some reason (probably performance), the > maintainer seems not like the idea. > > Maybe it's time to reconsider that? > > Thanks I like your generic solution but it introduces a change in semantics. Netvsc always changes device when returning a ANOTHER but do all devices? If some other stacked device did this then there a chance that using XDP on that device would see same packet twice.
> -----Original Message----- > From: Stephen Hemminger <stephen@networkplumber.org> > Sent: Wednesday, May 15, 2019 4:03 AM > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; davem@davemloft.net > Cc: netdev@vger.kernel.org; Stephen Hemminger <sthemmin@microsoft.com> > Subject: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler > > XDP generic does not work correctly with the Hyper-V/Azure netvsc device > because of packet processing order. Only packets on the synthetic path get > seen by the XDP program. The VF device packets are not seen. > > By the time the packets that arrive on the VF are handled by netvsc after the > first pass of XDP generic (on the VF) has already been done. > > A fix for the netvsc device is to do this in the VF packet handler. > by directly calling do_xdp_generic() if XDP program is present on the parent > device. > > A riskier but maybe better alternative would be to do this netdev core code > after the receive handler is invoked (if RX_HANDLER_ANOTHER is returned). > > Fixes: 0c195567a8f6 ("netvsc: transparent VF management") > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > --- > drivers/net/hyperv/netvsc_drv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 06393b215102..bb0fc1869bde 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -1999,9 +1999,15 @@ static rx_handler_result_t > netvsc_vf_handle_frame(struct sk_buff **pskb) > struct net_device_context *ndev_ctx = netdev_priv(ndev); > struct netvsc_vf_pcpu_stats *pcpu_stats > = this_cpu_ptr(ndev_ctx->vf_stats); > + struct bpf_prog *xdp_prog; > > skb->dev = ndev; > > + xdp_prog = rcu_dereference(ndev->xdp_prog); > + if (xdp_prog && > + do_xdp_generic(xdp_prog, skb) != XDP_PASS) > + return RX_HANDLER_CONSUMED; Looks fine overall. The function do_xdp_generic() already checks NULL on xdp_prog, so we don't need to check it in our code. int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb) { if (xdp_prog) { Thanks, - Haiyang
On Wed, 15 May 2019 17:50:25 +0000 Haiyang Zhang <haiyangz@microsoft.com> wrote: > > -----Original Message----- > > From: Stephen Hemminger <stephen@networkplumber.org> > > Sent: Wednesday, May 15, 2019 4:03 AM > > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > > <haiyangz@microsoft.com>; davem@davemloft.net > > Cc: netdev@vger.kernel.org; Stephen Hemminger <sthemmin@microsoft.com> > > Subject: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler > > > > XDP generic does not work correctly with the Hyper-V/Azure netvsc device > > because of packet processing order. Only packets on the synthetic path get > > seen by the XDP program. The VF device packets are not seen. > > > > By the time the packets that arrive on the VF are handled by netvsc after the > > first pass of XDP generic (on the VF) has already been done. > > > > A fix for the netvsc device is to do this in the VF packet handler. > > by directly calling do_xdp_generic() if XDP program is present on the parent > > device. > > > > A riskier but maybe better alternative would be to do this netdev core code > > after the receive handler is invoked (if RX_HANDLER_ANOTHER is returned). > > > > Fixes: 0c195567a8f6 ("netvsc: transparent VF management") > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > > --- > > drivers/net/hyperv/netvsc_drv.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > > index 06393b215102..bb0fc1869bde 100644 > > --- a/drivers/net/hyperv/netvsc_drv.c > > +++ b/drivers/net/hyperv/netvsc_drv.c > > @@ -1999,9 +1999,15 @@ static rx_handler_result_t > > netvsc_vf_handle_frame(struct sk_buff **pskb) > > struct net_device_context *ndev_ctx = netdev_priv(ndev); > > struct netvsc_vf_pcpu_stats *pcpu_stats > > = this_cpu_ptr(ndev_ctx->vf_stats); > > + struct bpf_prog *xdp_prog; > > > > skb->dev = ndev; > > > > + xdp_prog = rcu_dereference(ndev->xdp_prog); > > + if (xdp_prog && > > + do_xdp_generic(xdp_prog, skb) != XDP_PASS) > > + return RX_HANDLER_CONSUMED; > > Looks fine overall. > > The function do_xdp_generic() already checks NULL on xdp_prog, > so we don't need to check it in our code. > > int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb) > { > if (xdp_prog) { > The null check in the netvsc code was just an minor optimization to avoid unnecessary function call in fast path.
> -----Original Message----- > From: Stephen Hemminger <stephen@networkplumber.org> > Sent: Wednesday, May 15, 2019 1:54 PM > To: Haiyang Zhang <haiyangz@microsoft.com> > Cc: KY Srinivasan <kys@microsoft.com>; davem@davemloft.net; > netdev@vger.kernel.org; Stephen Hemminger <sthemmin@microsoft.com> > Subject: Re: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler > > On Wed, 15 May 2019 17:50:25 +0000 > Haiyang Zhang <haiyangz@microsoft.com> wrote: > > > > -----Original Message----- > > > From: Stephen Hemminger <stephen@networkplumber.org> > > > Sent: Wednesday, May 15, 2019 4:03 AM > > > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > > > <haiyangz@microsoft.com>; davem@davemloft.net > > > Cc: netdev@vger.kernel.org; Stephen Hemminger > <sthemmin@microsoft.com> > > > Subject: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler > > > > > > XDP generic does not work correctly with the Hyper-V/Azure netvsc device > > > because of packet processing order. Only packets on the synthetic path get > > > seen by the XDP program. The VF device packets are not seen. > > > > > > By the time the packets that arrive on the VF are handled by netvsc after > the > > > first pass of XDP generic (on the VF) has already been done. > > > > > > A fix for the netvsc device is to do this in the VF packet handler. > > > by directly calling do_xdp_generic() if XDP program is present on the > parent > > > device. > > > > > > A riskier but maybe better alternative would be to do this netdev core code > > > after the receive handler is invoked (if RX_HANDLER_ANOTHER is returned). > > > > > > Fixes: 0c195567a8f6 ("netvsc: transparent VF management") > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > > > --- > > > drivers/net/hyperv/netvsc_drv.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/net/hyperv/netvsc_drv.c > b/drivers/net/hyperv/netvsc_drv.c > > > index 06393b215102..bb0fc1869bde 100644 > > > --- a/drivers/net/hyperv/netvsc_drv.c > > > +++ b/drivers/net/hyperv/netvsc_drv.c > > > @@ -1999,9 +1999,15 @@ static rx_handler_result_t > > > netvsc_vf_handle_frame(struct sk_buff **pskb) > > > struct net_device_context *ndev_ctx = netdev_priv(ndev); > > > struct netvsc_vf_pcpu_stats *pcpu_stats > > > = this_cpu_ptr(ndev_ctx->vf_stats); > > > + struct bpf_prog *xdp_prog; > > > > > > skb->dev = ndev; > > > > > > + xdp_prog = rcu_dereference(ndev->xdp_prog); > > > + if (xdp_prog && > > > + do_xdp_generic(xdp_prog, skb) != XDP_PASS) > > > + return RX_HANDLER_CONSUMED; > > > > Looks fine overall. > > > > The function do_xdp_generic() already checks NULL on xdp_prog, > > so we don't need to check it in our code. > > > > int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb) > > { > > if (xdp_prog) { > > > > The null check in the netvsc code was just an minor optimization > to avoid unnecessary function call in fast path. Thanks for the explanation. Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
On 2019/5/15 下午11:23, Stephen Hemminger wrote: > On Wed, 15 May 2019 16:12:42 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> On 2019/5/15 下午4:03, Stephen Hemminger wrote: >>> XDP generic does not work correctly with the Hyper-V/Azure netvsc >>> device because of packet processing order. Only packets on the >>> synthetic path get seen by the XDP program. The VF device packets >>> are not seen. >>> >>> By the time the packets that arrive on the VF are handled by >>> netvsc after the first pass of XDP generic (on the VF) has already >>> been done. >>> >>> A fix for the netvsc device is to do this in the VF packet handler. >>> by directly calling do_xdp_generic() if XDP program is present >>> on the parent device. >>> >>> A riskier but maybe better alternative would be to do this netdev core >>> code after the receive handler is invoked (if RX_HANDLER_ANOTHER >>> is returned). >> >> Something like what I propose at >> https://lore.kernel.org/patchwork/patch/973819/ ? >> >> It belongs to a series that try to make XDP (both native and generic) >> work for stacked device. But for some reason (probably performance), the >> maintainer seems not like the idea. >> >> Maybe it's time to reconsider that? >> >> Thanks > > I like your generic solution but it introduces a change in semantics. > Netvsc always changes device when returning a ANOTHER but do all devices? > If some other stacked device did this then there a chance that using > XDP on that device would see same packet twice. Good point. Can we simply add a check and call XDP only if dev is changed in this case? Thanks
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 06393b215102..bb0fc1869bde 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -1999,9 +1999,15 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct sk_buff **pskb) struct net_device_context *ndev_ctx = netdev_priv(ndev); struct netvsc_vf_pcpu_stats *pcpu_stats = this_cpu_ptr(ndev_ctx->vf_stats); + struct bpf_prog *xdp_prog; skb->dev = ndev; + xdp_prog = rcu_dereference(ndev->xdp_prog); + if (xdp_prog && + do_xdp_generic(xdp_prog, skb) != XDP_PASS) + return RX_HANDLER_CONSUMED; + u64_stats_update_begin(&pcpu_stats->syncp); pcpu_stats->rx_packets++; pcpu_stats->rx_bytes += skb->len;
XDP generic does not work correctly with the Hyper-V/Azure netvsc device because of packet processing order. Only packets on the synthetic path get seen by the XDP program. The VF device packets are not seen. By the time the packets that arrive on the VF are handled by netvsc after the first pass of XDP generic (on the VF) has already been done. A fix for the netvsc device is to do this in the VF packet handler. by directly calling do_xdp_generic() if XDP program is present on the parent device. A riskier but maybe better alternative would be to do this netdev core code after the receive handler is invoked (if RX_HANDLER_ANOTHER is returned). Fixes: 0c195567a8f6 ("netvsc: transparent VF management") Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- drivers/net/hyperv/netvsc_drv.c | 6 ++++++ 1 file changed, 6 insertions(+)