[net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[John Fastabend]

In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.

Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a10500..d97bb71 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1890,8 +1890,12 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 			put_page(vi->rq[i].alloc_frag.page);
 }
 
-static bool is_xdp_queue(struct virtnet_info *vi, int q)
+static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
 {
+	/* For small receive mode always use kfree_skb variants */
+	if (!vi->mergeable_rx_bufs)
+		return false;
+
 	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
 		return false;
 	else if (q < vi->curr_queue_pairs)
@@ -1908,7 +1912,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		struct virtqueue *vq = vi->sq[i].vq;
 		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (!is_xdp_queue(vi, i))
+			if (!is_xdp_raw_buffer_queue(vi, i))
 				dev_kfree_skb(buf);
 			else
 				put_page(virt_to_head_page(buf));

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> In the small buffer case during driver unload we currently use
> put_page instead of dev_kfree_skb. Resolve this by adding a check
> for virtnet mode when checking XDP queue type. Also name the
> function so that the code reads correctly to match the additional
> check.
> 
> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4a10500..d97bb71 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1890,8 +1890,12 @@ static void free_receive_page_frags(struct virtnet_info *vi)
>  			put_page(vi->rq[i].alloc_frag.page);
>  }
>  
> -static bool is_xdp_queue(struct virtnet_info *vi, int q)
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
>  {
> +	/* For small receive mode always use kfree_skb variants */
> +	if (!vi->mergeable_rx_bufs)
> +		return false;
> +
>  	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
>  		return false;
>  	else if (q < vi->curr_queue_pairs)
> @@ -1908,7 +1912,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
>  	for (i = 0; i < vi->max_queue_pairs; i++) {
>  		struct virtqueue *vq = vi->sq[i].vq;
>  		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> -			if (!is_xdp_queue(vi, i))
> +			if (!is_xdp_raw_buffer_queue(vi, i))
>  				dev_kfree_skb(buf);
>  			else
>  				put_page(virt_to_head_page(buf));

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> In the small buffer case during driver unload we currently use
> put_page instead of dev_kfree_skb. Resolve this by adding a check
> for virtnet mode when checking XDP queue type. Also name the
> function so that the code reads correctly to match the additional
> check.
> 
> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

I think we definitely want this one in -net as it's
a bugfix.

> ---
>  drivers/net/virtio_net.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4a10500..d97bb71 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1890,8 +1890,12 @@ static void free_receive_page_frags(struct virtnet_info *vi)
>  			put_page(vi->rq[i].alloc_frag.page);
>  }
>  
> -static bool is_xdp_queue(struct virtnet_info *vi, int q)
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
>  {
> +	/* For small receive mode always use kfree_skb variants */
> +	if (!vi->mergeable_rx_bufs)
> +		return false;
> +
>  	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
>  		return false;
>  	else if (q < vi->curr_queue_pairs)
> @@ -1908,7 +1912,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
>  	for (i = 0; i < vi->max_queue_pairs; i++) {
>  		struct virtqueue *vq = vi->sq[i].vq;
>  		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> -			if (!is_xdp_queue(vi, i))
> +			if (!is_xdp_raw_buffer_queue(vi, i))
>  				dev_kfree_skb(buf);
>  			else
>  				put_page(virt_to_head_page(buf));

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[John Fastabend]

On 17-01-23 01:08 PM, Michael S. Tsirkin wrote:
> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>> In the small buffer case during driver unload we currently use
>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>> for virtnet mode when checking XDP queue type. Also name the
>> function so that the code reads correctly to match the additional
>> check.
>>
>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I think we definitely want this one in -net as it's
> a bugfix.
> 

Agreed, let me pull this fix out of the series and submit it for
net.

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200

> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>> In the small buffer case during driver unload we currently use
>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>> for virtnet mode when checking XDP queue type. Also name the
>> function so that the code reads correctly to match the additional
>> check.
>> 
>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I think we definitely want this one in -net as it's
> a bugfix.

This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 23 Jan 2017 23:08:35 +0200
> 
> > On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> >> In the small buffer case during driver unload we currently use
> >> put_page instead of dev_kfree_skb. Resolve this by adding a check
> >> for virtnet mode when checking XDP queue type. Also name the
> >> function so that the code reads correctly to match the additional
> >> check.
> >> 
> >> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> >> Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > I think we definitely want this one in -net as it's
> > a bugfix.
> 
> This whole series is a bug fix, we must have adjust_header XDP
> support in the virtio_net driver before v4.10 goes out, it is
> a requires base feature for XDP.

I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs. I am rather uncomfortable
doing that after -rc1.

How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 22:08:33 +0200

> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Mon, 23 Jan 2017 23:08:35 +0200
>> 
>> > On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>> >> In the small buffer case during driver unload we currently use
>> >> put_page instead of dev_kfree_skb. Resolve this by adding a check
>> >> for virtnet mode when checking XDP queue type. Also name the
>> >> function so that the code reads correctly to match the additional
>> >> check.
>> >> 
>> >> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> >> Acked-by: Jason Wang <jasowang@redhat.com>
>> > 
>> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> > 
>> > I think we definitely want this one in -net as it's
>> > a bugfix.
>> 
>> This whole series is a bug fix, we must have adjust_header XDP
>> support in the virtio_net driver before v4.10 goes out, it is
>> a requires base feature for XDP.
> 
> I have to say device resets outside probe have a huge potential
> to uncover hypervisor bugs. I am rather uncomfortable
> doing that after -rc1.
> 
> How about a module option to disable it by default?
> We can then ship a partial implementation in 4.10
> and work on completing it in 4.11.

XDP programmers must be able to assume a base set of features being
present, adjust_header is one of them.

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 24, 2017 at 03:11:39PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 24 Jan 2017 22:08:33 +0200
> 
> > On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> >> Date: Mon, 23 Jan 2017 23:08:35 +0200
> >> 
> >> > On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> >> >> In the small buffer case during driver unload we currently use
> >> >> put_page instead of dev_kfree_skb. Resolve this by adding a check
> >> >> for virtnet mode when checking XDP queue type. Also name the
> >> >> function so that the code reads correctly to match the additional
> >> >> check.
> >> >> 
> >> >> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> >> >> Acked-by: Jason Wang <jasowang@redhat.com>
> >> > 
> >> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >> > 
> >> > I think we definitely want this one in -net as it's
> >> > a bugfix.
> >> 
> >> This whole series is a bug fix, we must have adjust_header XDP
> >> support in the virtio_net driver before v4.10 goes out, it is
> >> a requires base feature for XDP.
> > 
> > I have to say device resets outside probe have a huge potential
> > to uncover hypervisor bugs. I am rather uncomfortable
> > doing that after -rc1.
> > 
> > How about a module option to disable it by default?
> > We can then ship a partial implementation in 4.10
> > and work on completing it in 4.11.
> 
> XDP programmers must be able to assume a base set of features being
> present, adjust_header is one of them.

Let's make all of XDP depend on this extra_headroom option then?

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Jason Wang]

On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Mon, 23 Jan 2017 23:08:35 +0200
>>
>>> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>>>> In the small buffer case during driver unload we currently use
>>>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>>>> for virtnet mode when checking XDP queue type. Also name the
>>>> function so that the code reads correctly to match the additional
>>>> check.
>>>>
>>>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> I think we definitely want this one in -net as it's
>>> a bugfix.
>> This whole series is a bug fix, we must have adjust_header XDP
>> support in the virtio_net driver before v4.10 goes out, it is
>> a requires base feature for XDP.
> I have to say device resets outside probe have a huge potential
> to uncover hypervisor bugs.

Maybe not if it reuses most of current codes? Since we've already used 
them in sleep or hibernation?

Thanks

>   I am rather uncomfortable
> doing that after -rc1.
>
> How about a module option to disable it by default?
> We can then ship a partial implementation in 4.10
> and work on completing it in 4.11.
>

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
> > On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > Date: Mon, 23 Jan 2017 23:08:35 +0200
> > > 
> > > > On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> > > > > In the small buffer case during driver unload we currently use
> > > > > put_page instead of dev_kfree_skb. Resolve this by adding a check
> > > > > for virtnet mode when checking XDP queue type. Also name the
> > > > > function so that the code reads correctly to match the additional
> > > > > check.
> > > > > 
> > > > > Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> > > > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > I think we definitely want this one in -net as it's
> > > > a bugfix.
> > > This whole series is a bug fix, we must have adjust_header XDP
> > > support in the virtio_net driver before v4.10 goes out, it is
> > > a requires base feature for XDP.
> > I have to say device resets outside probe have a huge potential
> > to uncover hypervisor bugs.
> 
> Maybe not if it reuses most of current codes? Since we've already used them
> in sleep or hibernation?
> 
> Thanks

Except almost no one uses sleep or hybernate with VMs.  I'm not saying
it's a bad idea, just that it needs a lot of testing before release and
we won't get enough if we merge at this point.

> >   I am rather uncomfortable
> > doing that after -rc1.
> > 
> > How about a module option to disable it by default?
> > We can then ship a partial implementation in 4.10
> > and work on completing it in 4.11.
> > 

To clarify, I'm thinking an option similar to enable_xdp,
and have all packets have a 256 byte headroom for 4.10.

Consider our options for 4.11.

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[John Fastabend]

On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
>>
>>
>> On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
>>> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
>>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Date: Mon, 23 Jan 2017 23:08:35 +0200
>>>>
>>>>> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>>>>>> In the small buffer case during driver unload we currently use
>>>>>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>>>>>> for virtnet mode when checking XDP queue type. Also name the
>>>>>> function so that the code reads correctly to match the additional
>>>>>> check.
>>>>>>
>>>>>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>
>>>>> I think we definitely want this one in -net as it's
>>>>> a bugfix.
>>>> This whole series is a bug fix, we must have adjust_header XDP
>>>> support in the virtio_net driver before v4.10 goes out, it is
>>>> a requires base feature for XDP.
>>> I have to say device resets outside probe have a huge potential
>>> to uncover hypervisor bugs.
>>
>> Maybe not if it reuses most of current codes? Since we've already used them
>> in sleep or hibernation?
>>
>> Thanks
> 
> Except almost no one uses sleep or hybernate with VMs.  I'm not saying
> it's a bad idea, just that it needs a lot of testing before release and
> we won't get enough if we merge at this point.
> 

Then it would seem like a good thing to have another user of these paths and
find the bugs versus letting them sit there for some poor folks who do use
sleep/hybernate.

>>>   I am rather uncomfortable
>>> doing that after -rc1.
>>>
>>> How about a module option to disable it by default?
>>> We can then ship a partial implementation in 4.10
>>> and work on completing it in 4.11.
>>>

Ugh I would prefer to avoid module options. This will only happen if users
push XDP program into driver anyways.

> 
> To clarify, I'm thinking an option similar to enable_xdp,
> and have all packets have a 256 byte headroom for 4.10.

An option where? In QEMU side, in driver? Is the reset really that bad, coming
from a hardware driver side lots of configuration changes can cause resets. I
agree its not overly elegant but could follow on patches be used to make it
prettier if possible.

I know folks prefer to avoid tuning knobs but I think exposing the headroom
configuration to users might not be a bad idea. After all these same users are
already programming maps and ebpf codes. A simple tuning knob should not be a
big deal and reasonable defaults would of course be used. That is a net-next
debate though.

> 
> Consider our options for 4.11.
> 

Finally just to point out here are the drivers with XDP support on latest
net tree,

	mlx/mlx5
	mlx/mlx4
	qlogic/qede
	netronome/nfp
	virtio_net

And here is the list of adjust header support,

	mlx/mlx4

So we currently have the same feature gap on all the other drivers except one.
Although I do not think that is a very good excuse. Lets figure out what we
should do about virtio.

Thanks,
John

	
	

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Jason Wang]

On 2017年01月25日 12:02, John Fastabend wrote:
> On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
>> On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
>>> On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
>>>> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
>>>>> From: "Michael S. Tsirkin"<mst@redhat.com>
>>>>> Date: Mon, 23 Jan 2017 23:08:35 +0200
>>>>>
>>>>>> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
>>>>>>> In the small buffer case during driver unload we currently use
>>>>>>> put_page instead of dev_kfree_skb. Resolve this by adding a check
>>>>>>> for virtnet mode when checking XDP queue type. Also name the
>>>>>>> function so that the code reads correctly to match the additional
>>>>>>> check.
>>>>>>>
>>>>>>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>>>>> Acked-by: Jason Wang<jasowang@redhat.com>
>>>>>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>>>>>
>>>>>> I think we definitely want this one in -net as it's
>>>>>> a bugfix.
>>>>> This whole series is a bug fix, we must have adjust_header XDP
>>>>> support in the virtio_net driver before v4.10 goes out, it is
>>>>> a requires base feature for XDP.
>>>> I have to say device resets outside probe have a huge potential
>>>> to uncover hypervisor bugs.
>>> Maybe not if it reuses most of current codes? Since we've already used them
>>> in sleep or hibernation?
>>>
>>> Thanks
>> Except almost no one uses sleep or hybernate with VMs.  I'm not saying
>> it's a bad idea, just that it needs a lot of testing before release and
>> we won't get enough if we merge at this point.
>>
> Then it would seem like a good thing to have another user of these paths and
> find the bugs versus letting them sit there for some poor folks who do use
> sleep/hybernate.
>

Yes, and uncovering hypervisor bugs now is better than uncovering it in 
the future.

Thanks

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Alexei Starovoitov]

On Tue, Jan 24, 2017 at 8:02 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Finally just to point out here are the drivers with XDP support on latest
> net tree,
>
>         mlx/mlx5
>         mlx/mlx4
>         qlogic/qede
>         netronome/nfp
>         virtio_net
>
> And here is the list of adjust header support,
>
>         mlx/mlx4
>

in net-next it's actually:
yes: mlx4, mlx5
no: qede, nfp, virtio
while nfp and virtio are working on it.

xdp_adjust_head() is must have for load balancer,
so the sooner it lands for virtio the easier it will be
to develop xdp programs. Initially I expected
e1k+xdp to be the base line for debugging and
development of xdp programs, but since not everyone
agreed on e1k the virtio+xdp filled in the gap.
So without adjust_head in virtio I see very little use for it
in our environment.
It is a must have feature regardless of timing.
I will backport whatever is necessary, but distros
will stick with official releases and imo it's not great
from xdp adoption point of view to have
virtio driver lacking key features.

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 24, 2017 at 08:02:29PM -0800, John Fastabend wrote:
> On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
> > On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
> >>
> >>
> >> On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
> >>> On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
> >>>> From: "Michael S. Tsirkin" <mst@redhat.com>
> >>>> Date: Mon, 23 Jan 2017 23:08:35 +0200
> >>>>
> >>>>> On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
> >>>>>> In the small buffer case during driver unload we currently use
> >>>>>> put_page instead of dev_kfree_skb. Resolve this by adding a check
> >>>>>> for virtnet mode when checking XDP queue type. Also name the
> >>>>>> function so that the code reads correctly to match the additional
> >>>>>> check.
> >>>>>>
> >>>>>> Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
> >>>>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> >>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
> >>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> I think we definitely want this one in -net as it's
> >>>>> a bugfix.
> >>>> This whole series is a bug fix, we must have adjust_header XDP
> >>>> support in the virtio_net driver before v4.10 goes out, it is
> >>>> a requires base feature for XDP.
> >>> I have to say device resets outside probe have a huge potential
> >>> to uncover hypervisor bugs.
> >>
> >> Maybe not if it reuses most of current codes? Since we've already used them
> >> in sleep or hibernation?
> >>
> >> Thanks
> > 
> > Except almost no one uses sleep or hybernate with VMs.  I'm not saying
> > it's a bad idea, just that it needs a lot of testing before release and
> > we won't get enough if we merge at this point.
> > 
> 
> Then it would seem like a good thing to have another user of these paths and
> find the bugs versus letting them sit there for some poor folks who do use
> sleep/hybernate.

Absolutely. But -rc6 is not the time to test waters IMO.

> >>>   I am rather uncomfortable
> >>> doing that after -rc1.
> >>>
> >>> How about a module option to disable it by default?
> >>> We can then ship a partial implementation in 4.10
> >>> and work on completing it in 4.11.
> >>>
> 
> Ugh I would prefer to avoid module options. This will only happen if users
> push XDP program into driver anyways.

Again I agree, it's an idea for a stopgap measure so we can have
something in 4.10 - and also assuming that 256b headroom is a must.

> > 
> > To clarify, I'm thinking an option similar to enable_xdp,
> > and have all packets have a 256 byte headroom for 4.10.
> 
> An option where? In QEMU side, in driver? Is the reset really that bad, coming
> from a hardware driver side lots of configuration changes can cause resets. I
> agree its not overly elegant but could follow on patches be used to make it
> prettier if possible.

Again I agree and it's not that bad it's just not something we should
do past rc5.

> I know folks prefer to avoid tuning knobs but I think exposing the headroom
> configuration to users might not be a bad idea. After all these same users are
> already programming maps and ebpf codes. A simple tuning knob should not be a
> big deal and reasonable defaults would of course be used. That is a net-next
> debate though.

No arguments from my side here.

> > 
> > Consider our options for 4.11.
> > 
> 
> Finally just to point out here are the drivers with XDP support on latest
> net tree,
> 
> 	mlx/mlx5
> 	mlx/mlx4
> 	qlogic/qede
> 	netronome/nfp
> 	virtio_net
> 
> And here is the list of adjust header support,
> 
> 	mlx/mlx4

Above seems to imply an interface for userspace to detect the amount
of head space would be benefitial.

> 
> So we currently have the same feature gap on all the other drivers except one.
> Although I do not think that is a very good excuse. Lets figure out what we
> should do about virtio.
> 
> Thanks,
> John

If we can simply defer adjust_head patches to 4.11 then that's fine.

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Wed, Jan 25, 2017 at 01:46:46PM +0800, Jason Wang wrote:
> > Then it would seem like a good thing to have another user of these paths and
> > find the bugs versus letting them sit there for some poor folks who do use
> > sleep/hybernate.
> > 
> 
> Yes, and uncovering hypervisor bugs now is better than uncovering it in the
> future.
> 
> Thanks

Not really, all the uncovering should happen in -next or early rc.
Right now we need to fix what has been uncovered so far.

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[Michael S. Tsirkin]

On Tue, Jan 24, 2017 at 11:33:56PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 24, 2017 at 8:02 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Finally just to point out here are the drivers with XDP support on latest
> > net tree,
> >
> >         mlx/mlx5
> >         mlx/mlx4
> >         qlogic/qede
> >         netronome/nfp
> >         virtio_net
> >
> > And here is the list of adjust header support,
> >
> >         mlx/mlx4
> >
> 
> in net-next it's actually:
> yes: mlx4, mlx5
> no: qede, nfp, virtio
> while nfp and virtio are working on it.
> 
> xdp_adjust_head() is must have for load balancer,

What amount of head space does it need? 70 bytes
to do vxlan kind of thing?

> so the sooner it lands for virtio the easier it will be
> to develop xdp programs. Initially I expected
> e1k+xdp to be the base line for debugging and
> development of xdp programs, but since not everyone
> agreed on e1k the virtio+xdp filled in the gap.
> So without adjust_head in virtio I see very little use for it
> in our environment.
> It is a must have feature regardless of timing.
> I will backport whatever is necessary, but distros
> will stick with official releases and imo it's not great
> from xdp adoption point of view to have
> virtio driver lacking key features.

If everyone can agree it's net-next material then I'm happy.

-- 
MST

Re: [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

[John Fastabend]

On 17-01-25 06:52 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 24, 2017 at 11:33:56PM -0800, Alexei Starovoitov wrote:
>> On Tue, Jan 24, 2017 at 8:02 PM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>>
>>> Finally just to point out here are the drivers with XDP support on latest
>>> net tree,
>>>
>>>         mlx/mlx5
>>>         mlx/mlx4
>>>         qlogic/qede
>>>         netronome/nfp
>>>         virtio_net
>>>
>>> And here is the list of adjust header support,
>>>
>>>         mlx/mlx4
>>>
>>
>> in net-next it's actually:
>> yes: mlx4, mlx5
>> no: qede, nfp, virtio
>> while nfp and virtio are working on it.
>>
>> xdp_adjust_head() is must have for load balancer,
> 
> What amount of head space does it need? 70 bytes
> to do vxlan kind of thing?
> 
>> so the sooner it lands for virtio the easier it will be
>> to develop xdp programs. Initially I expected
>> e1k+xdp to be the base line for debugging and
>> development of xdp programs, but since not everyone
>> agreed on e1k the virtio+xdp filled in the gap.
>> So without adjust_head in virtio I see very little use for it
>> in our environment.
>> It is a must have feature regardless of timing.
>> I will backport whatever is necessary, but distros
>> will stick with official releases and imo it's not great
>> from xdp adoption point of view to have
>> virtio driver lacking key features.
> 
> If everyone can agree it's net-next material then I'm happy.
> 

Considering that the only support for adjust_head in net branch
is in mlx4 and most drivers are aborting when programs get loaded
with adjust_head support I am OK with applying the patch below to
net and this series to net-next.

https://patchwork.ozlabs.org/patch/707118/

 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index 08327e005ccc..db761f37783e 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -1677,6 +1677,11 @@  static int virtnet_xdp_set(struct net_device *dev,
struct bpf_prog *prog)
  	u16 xdp_qp = 0, curr_qp;
  	int i, err;

 +	if (prog && prog->xdp_adjust_head) {
 +		netdev_warn(dev, "Does not support bpf_xdp_adjust_head()\n");
 +		return -EOPNOTSUPP;
 +	}
 +
  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
  	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6)) {
  		netdev_warn(dev, "can't set XDP while host is implementing LRO, disable LRO
first\n");


Thanks,
John

[PATCH net resend] virtio_net: reject XDP programs using header adjustment

[Jakub Kicinski]

commit 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
added a new XDP helper to prepend and remove data from a frame.
Make virtio_net reject programs making use of this helper until
proper support is added.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: John Fastabend <john.r.fastabend@intel.com>
---
 drivers/net/virtio_net.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f9bf94887ff1..a2aac4fd8e42 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1710,6 +1710,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	u16 xdp_qp = 0, curr_qp;
 	int i, err;
 
+	if (prog && prog->xdp_adjust_head) {
+		netdev_warn(dev, "Does not support bpf_xdp_adjust_head()\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
 	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
-- 
2.11.0

Re: [PATCH net resend] virtio_net: reject XDP programs using header adjustment

[Jason Wang]

On 2017年01月26日 06:56, Jakub Kicinski wrote:
> commit 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
> added a new XDP helper to prepend and remove data from a frame.
> Make virtio_net reject programs making use of this helper until
> proper support is added.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>   drivers/net/virtio_net.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f9bf94887ff1..a2aac4fd8e42 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1710,6 +1710,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   	u16 xdp_qp = 0, curr_qp;
>   	int i, err;
>   
> +	if (prog && prog->xdp_adjust_head) {
> +		netdev_warn(dev, "Does not support bpf_xdp_adjust_head()\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>   	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>   	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>   	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net resend] virtio_net: reject XDP programs using header adjustment

[Michael S. Tsirkin]

On Wed, Jan 25, 2017 at 02:56:36PM -0800, Jakub Kicinski wrote:
> commit 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
> added a new XDP helper to prepend and remove data from a frame.
> Make virtio_net reject programs making use of this helper until
> proper support is added.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f9bf94887ff1..a2aac4fd8e42 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1710,6 +1710,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>  	u16 xdp_qp = 0, curr_qp;
>  	int i, err;
>  
> +	if (prog && prog->xdp_adjust_head) {
> +		netdev_warn(dev, "Does not support bpf_xdp_adjust_head()\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>  	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>  	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> -- 
> 2.11.0

Re: [PATCH net resend] virtio_net: reject XDP programs using header adjustment

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Wed, 25 Jan 2017 14:56:36 -0800

> commit 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
> added a new XDP helper to prepend and remove data from a frame.
> Make virtio_net reject programs making use of this helper until
> proper support is added.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>

Applied.