* [patch net-next] net: virtio_net: implement exact header length guest feature
@ 2023-02-17 12:15 Jiri Pirko
2023-02-17 12:22 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2023-02-17 12:15 UTC (permalink / raw)
To: netdev; +Cc: mst, virtualization, edumazet, kuba, pabeni, davem
From: Jiri Pirko <jiri@nvidia.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@nvidia.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-17 12:15 [patch net-next] net: virtio_net: implement exact header length guest feature Jiri Pirko
@ 2023-02-17 12:22 ` Michael S. Tsirkin
2023-02-17 12:53 ` Jiri Pirko
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2023-02-17 12:22 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, virtualization, edumazet, kuba, pabeni, davem
On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.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@nvidia.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?
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-17 12:22 ` Michael S. Tsirkin
@ 2023-02-17 12:53 ` Jiri Pirko
2023-02-17 13:47 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2023-02-17 12:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, virtualization, edumazet, kuba, pabeni, davem
Fri, Feb 17, 2023 at 01:22:01PM CET, mst@redhat.com wrote:
>On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
>
>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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-17 12:53 ` Jiri Pirko
@ 2023-02-17 13:47 ` Michael S. Tsirkin
2023-02-20 8:35 ` Jiri Pirko
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2023-02-17 13:47 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
> Fri, Feb 17, 2023 at 01:22:01PM CET, mst@redhat.com wrote:
> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-17 13:47 ` Michael S. Tsirkin
@ 2023-02-20 8:35 ` Jiri Pirko
2023-02-20 12:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2023-02-20 8:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
Fri, Feb 17, 2023 at 02:47:36PM CET, mst@redhat.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@redhat.com wrote:
>> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
I will add this to the patch description and send v2.
>
>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
>> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 8:35 ` Jiri Pirko
@ 2023-02-20 12:55 ` Michael S. Tsirkin
2023-02-20 13:56 ` Jiri Pirko
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2023-02-20 12:55 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
> Fri, Feb 17, 2023 at 02:47:36PM CET, mst@redhat.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@redhat.com wrote:
> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
> >> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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?
And making it bigger than necessary works fine ...
> 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?
>
> >
> >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
> >> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 12:55 ` Michael S. Tsirkin
@ 2023-02-20 13:56 ` Jiri Pirko
2023-02-20 22:43 ` Michael S. Tsirkin
2023-02-21 2:38 ` Jason Wang
2023-02-21 2:24 ` Jason Wang
2023-02-21 13:39 ` Alvaro Karsz
2 siblings, 2 replies; 14+ messages in thread
From: Jiri Pirko @ 2023-02-20 13:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
Mon, Feb 20, 2023 at 01:55:33PM CET, mst@redhat.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@redhat.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@redhat.com wrote:
>> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>> >> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
>
>> 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.
>
>
>
>>
>> >
>> >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
>> >> >
>> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 13:56 ` Jiri Pirko
@ 2023-02-20 22:43 ` Michael S. Tsirkin
2023-02-21 12:32 ` Jiri Pirko
2023-02-21 2:38 ` Jason Wang
1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2023-02-20 22:43 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
On Mon, Feb 20, 2023 at 02:56:08PM +0100, Jiri Pirko wrote:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@redhat.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@redhat.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@redhat.com wrote:
> >> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
> >> >> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
>
I mean if hdr_len is bigger than necessary tap does work.
> >
> >> 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.
OK so do you have hardware which exposes this feature?
That is the bit I am missing. Maybe mention the make
in the commit log so
we know where to turn if we need to make changes here?
Or "under development" if it is not on the market yet.
>
> >
> >
> >
> >>
> >> >
> >> >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
> >> >> >
> >> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 12:55 ` Michael S. Tsirkin
2023-02-20 13:56 ` Jiri Pirko
@ 2023-02-21 2:24 ` Jason Wang
2023-02-21 13:39 ` Alvaro Karsz
2 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2023-02-21 2:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jiri Pirko, Vitaly Mireyno, netdev, virtualization, edumazet,
kuba, pabeni, davem
On Mon, Feb 20, 2023 at 8:55 PM Michael S. Tsirkin <mst@redhat.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@redhat.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@redhat.com wrote:
> > >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
> > >> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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?
> And making it bigger than necessary works fine ...
It should work otherwise it's a bug after this commit:
commit 96f8d9ecf227638c89f98ccdcdd50b569891976c
Author: Jason Wang <jasowang@redhat.com>
Date: Wed Nov 13 14:00:39 2013 +0800
tuntap: limit head length of skb allocated
Thanks
>
> > 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?
>
>
>
> >
> > >
> > >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
> > >> >
> > >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 13:56 ` Jiri Pirko
2023-02-20 22:43 ` Michael S. Tsirkin
@ 2023-02-21 2:38 ` Jason Wang
2023-02-21 12:29 ` Jiri Pirko
1 sibling, 1 reply; 14+ messages in thread
From: Jason Wang @ 2023-02-21 2:38 UTC (permalink / raw)
To: Jiri Pirko, Michael S. Tsirkin
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
在 2023/2/20 21:56, Jiri Pirko 写道:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@redhat.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@redhat.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@redhat.com wrote:
>>>>>> On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>>>>>>> From: Jiri Pirko <jiri@nvidia.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@nvidia.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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-21 2:38 ` Jason Wang
@ 2023-02-21 12:29 ` Jiri Pirko
0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2023-02-21 12:29 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, netdev, Vitaly Mireyno, virtualization,
edumazet, kuba, pabeni, davem
Tue, Feb 21, 2023 at 03:38:10AM CET, jasowang@redhat.com wrote:
>
>在 2023/2/20 21:56, Jiri Pirko 写道:
>> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@redhat.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@redhat.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@redhat.com wrote:
>> > > > > > On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>> > > > > > > From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
That's probably why the spec says:
5.1.6.2.2
...
Note: Caution should be taken by the implementation so as to prevent a malicious driver from attacking
the device by setting an incorrect hdr_len.
And that is exactly what tun does by caping the linear size to
SKB_MAX_HEAD(TAP_RESERVE)
>
>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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 22:43 ` Michael S. Tsirkin
@ 2023-02-21 12:32 ` Jiri Pirko
0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2023-02-21 12:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
Mon, Feb 20, 2023 at 11:43:52PM CET, mst@redhat.com wrote:
>On Mon, Feb 20, 2023 at 02:56:08PM +0100, Jiri Pirko wrote:
>> Mon, Feb 20, 2023 at 01:55:33PM CET, mst@redhat.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@redhat.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@redhat.com wrote:
>> >> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko wrote:
>> >> >> >> From: Jiri Pirko <jiri@nvidia.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@nvidia.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.
>>
>
>I mean if hdr_len is bigger than necessary tap does work.
>
>
>> >
>> >> 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.
>
>OK so do you have hardware which exposes this feature?
I believe it is not implemented yet, but it most certainly will be in
near future.
>That is the bit I am missing. Maybe mention the make
>in the commit log so
>we know where to turn if we need to make changes here?
>Or "under development" if it is not on the market yet.
Will put a note in the commit log.
>
>>
>> >
>> >
>> >
>> >>
>> >> >
>> >> >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
>> >> >> >
>> >> >
>> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-20 12:55 ` Michael S. Tsirkin
2023-02-20 13:56 ` Jiri Pirko
2023-02-21 2:24 ` Jason Wang
@ 2023-02-21 13:39 ` Alvaro Karsz
2023-02-21 14:27 ` Jiri Pirko
2 siblings, 1 reply; 14+ messages in thread
From: Alvaro Karsz @ 2023-02-21 13:39 UTC (permalink / raw)
To: Michael S. Tsirkin, Jiri Pirko
Cc: Vitaly Mireyno, netdev, virtualization, edumazet, kuba, pabeni, davem
Hi,
Our device offers this feature bit as well.
I don't have concrete numbers, but this feature will save our device a
few cycles for every GSO packet.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch net-next] net: virtio_net: implement exact header length guest feature
2023-02-21 13:39 ` Alvaro Karsz
@ 2023-02-21 14:27 ` Jiri Pirko
0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2023-02-21 14:27 UTC (permalink / raw)
To: Alvaro Karsz
Cc: Vitaly Mireyno, netdev, Michael S. Tsirkin, virtualization,
edumazet, kuba, pabeni, davem
Tue, Feb 21, 2023 at 02:39:31PM CET, alvaro.karsz@solid-run.com wrote:
>Hi,
>
>Our device offers this feature bit as well.
>I don't have concrete numbers, but this feature will save our device a
>few cycles for every GSO packet.
Cool. I will try to include this into the patch description. Thanks!
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-02-21 14:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 12:15 [patch net-next] net: virtio_net: implement exact header length guest feature Jiri Pirko
2023-02-17 12:22 ` Michael S. Tsirkin
2023-02-17 12:53 ` Jiri Pirko
2023-02-17 13:47 ` Michael S. Tsirkin
2023-02-20 8:35 ` Jiri Pirko
2023-02-20 12:55 ` Michael S. Tsirkin
2023-02-20 13:56 ` Jiri Pirko
2023-02-20 22:43 ` Michael S. Tsirkin
2023-02-21 12:32 ` Jiri Pirko
2023-02-21 2:38 ` Jason Wang
2023-02-21 12:29 ` Jiri Pirko
2023-02-21 2:24 ` Jason Wang
2023-02-21 13:39 ` Alvaro Karsz
2023-02-21 14:27 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).