Date:   Tue, 21 Feb 2023 10:38:10 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Jiri Pirko <jiri@...nulli.us>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com,
        virtualization@...ts.linux-foundation.org,
        Vitaly Mireyno <vmireyno@...vell.com>
Subject: Re: [patch net-next] net: virtio_net: implement exact header length
 guest feature


在 2023/2/20 21:56, Jiri Pirko 写道:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@...hat.com wrote:
>> On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
>>> Fri, Feb 17, 2023 at 02:47:36PM CET, mst@...hat.com wrote:
>>>> On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
>>>>> Fri, Feb 17, 2023 at 01:22:01PM CET, mst@...hat.com wrote:
>>>>>> On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>>>>>>> From: Jiri Pirko <jiri@...dia.com>
>>>>>>>
>>>>>>> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
>>>>>>>
>>>>>>> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>>>>>>> set implicates that the driver provides the exact size of the header.
>>>>>>>
>>>>>>> The driver already complies to fill the correct value. Introduce the
>>>>>>> feature and advertise it.
>>>>>>>
>>>>>>> Signed-off-by: Jiri Pirko <jiri@...dia.com>
>>>>>> Could you add a bit of motivation just for the record?
>>>>>> Does this improve performance for some card? By how much?
>>>>>> Expected to help some future card?
>>>>> I can get that info, but isn't that rather something to be appended to
>>>>> the virtio-spec patch? I mean, the feature is there, this is just
>>>>> implementing it in one driver.
>>>> It is more like using it in the driver.  It's not like we have to use
>>>> everything - it could be useful for e.g. dpdk but not linux.
>>>> Implementing it in the Linux driver has support costs - for example what
>>>> if there's a bug and sometimes the length is incorrect?
>>>> We'll be breaking things.
>>> I understand. To my understanding this feature just fixes the original
>>> ambiguity in the virtio spec.
>>>
>>> Quoting the original virtio spec:
>>> "hdr_len is a hint to the device as to how much of the header needs to
>>>   be kept to copy into each packet"
>>>
>>> "a hint" might not be clear for the reader what does it mean, if it is
>>> "maybe like that" of "exactly like that". This feature just makes it
>>> crystal clear.
>>>
>>> If you look at the tap implementation, it uses hdr_len to alloc
>>> skb linear part. No hint, it counts with the provided value.
>>> So if the driver is currently not precise, it breaks tap.
>> Well that's only for gso though right?
> Yep.
>
>
>> And making it bigger than necessary works fine ...
> Well yeah. But tap does not do that, does it? it uses hdr_len directly.


tap_get_user() limit the head length:


static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
                             struct iov_iter *from, int noblock)
{
     int good_linear = SKB_MAX_HEAD(TAP_RESERVE);
...


>
>
>>> I will add this to the patch description and send v2.
>>>
>> I feel this does not answer the question yet, or maybe I am being dense.
>> My point was not about making hdr_len precise.  My point was that we are
>> making a change here for no apparent reason. I am guessing you are not
>> doing it for fun - so why? Is there a device with this feature bit
>> you are aware of?
> Afaik real hw which does emulation of virtio_net would benefit from
> that, our hw including.


Note that driver can choose to no negotiate this feature, so malicious 
drivers can still try to use illegal value.

Thanks


>
>
>>
>>
>>>> The patch was submitted by Marvell but they never bothered with
>>>> using it in Linux. I guess they are using it for something else?
>>>> CC Vitaly who put this in.
>>>>
>>>>>> thanks!
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>>   drivers/net/virtio_net.c        | 6 ++++--
>>>>>>>   include/uapi/linux/virtio_net.h | 1 +
>>>>>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>> index fb5e68ed3ec2..e85b03988733 100644
>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>>>>>>>   	VIRTIO_NET_F_GUEST_UFO,
>>>>>>>   	VIRTIO_NET_F_GUEST_CSUM,
>>>>>>>   	VIRTIO_NET_F_GUEST_USO4,
>>>>>>> -	VIRTIO_NET_F_GUEST_USO6
>>>>>>> +	VIRTIO_NET_F_GUEST_USO6,
>>>>>>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>>   };
>>>>>>>   
>>>>>>>   #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>>>>>>> @@ -4213,7 +4214,8 @@ static struct virtio_device_id id_table[] = {
>>>>>>>   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>>>>>   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>>>>>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>>>>>>> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
>>>>>>> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
>>>>>>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>>   
>>>>>>>   static unsigned int features[] = {
>>>>>>>   	VIRTNET_FEATURES,
>>>>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>>>>> index b4062bed186a..12c1c9699935 100644
>>>>>>> --- a/include/uapi/linux/virtio_net.h
>>>>>>> +++ b/include/uapi/linux/virtio_net.h
>>>>>>> @@ -61,6 +61,7 @@
>>>>>>>   #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in. */
>>>>>>>   #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
>>>>>>>   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
>>>>>>> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact hdr_len value. */
>>>>>>>   #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
>>>>>>>   #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
>>>>>>>   #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another device
>>>>>>> -- 
>>>>>>> 2.39.0

Powered by blists - more mailing lists