[RFC 0/2] netvsc fixes to VF frame handling

[RFC 0/2] netvsc fixes to VF frame handling

[Stephen Hemminger]

XDP generic would not work with the transparent VF
support in the netvsc device. This problem is generic
and probably also happens with failsafe, team, bridge,
bond and any other device with a receive handler.

Stephen Hemminger (2):
  netvsc: invoke xdp_generic from VF frame handler
  netvsc: unshare skb in VF rx handler

 drivers/net/hyperv/netvsc_drv.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.20.1

[RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Stephen Hemminger]

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;
+
 	u64_stats_update_begin(&pcpu_stats->syncp);
 	pcpu_stats->rx_packets++;
 	pcpu_stats->rx_bytes += skb->len;
-- 
2.20.1

Re: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Jason Wang]

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;

Re: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Stephen Hemminger]

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.

Re: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Jason Wang]

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

RE: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Haiyang Zhang]

> -----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

Re: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Stephen Hemminger]

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.

RE: [RFC 1/2] netvsc: invoke xdp_generic from VF frame handler

[Haiyang Zhang]

> -----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>

[RFC 2/2] netvsc: unshare skb in VF rx handler

[Stephen Hemminger]

The netvsc VF skb handler should make sure that skb is not
shared. Similar logic already exists in bonding and team device
drivers.

This does not happen in practice because the mlx device
driver does not return shared skb's.

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 bb0fc1869bde..eb666908b0fa 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2001,6 +2001,12 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct sk_buff **pskb)
 		 = this_cpu_ptr(ndev_ctx->vf_stats);
 	struct bpf_prog *xdp_prog;
 
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (unlikely(!skb))
+		return RX_HANDLER_CONSUMED;
+
+	*pskb = skb;
+
 	skb->dev = ndev;
 
 	xdp_prog = rcu_dereference(ndev->xdp_prog);
-- 
2.20.1