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

[John Fastabend <john.fastabend@gmail.com>]

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