diff mbox

[RFC,v4,1/3] virtio-net rsc: add a new host offload(rsc) feature bit

Message ID 1459711556-10273-2-git-send-email-wexu@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Xu April 3, 2016, 7:25 p.m. UTC 
From: Wei Xu <wexu@redhat.com>

A new feature bit 'VIRTIO_NET_F_GUEST_RSC' is introduced to support WHQL
Receive-Segment-Offload test, this feature will coalesce tcp packets in
IPv4/6 for userspace virtio-net driver.

This feature can be enabled either by 'ACK' the feature when loading
the driver in the guest, or by sending the 'VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET'
command to the host via control queue.

Signed-off-by: Wei Xu <wexu@redhat.com>
---
 hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++--
 include/standard-headers/linux/virtio_net.h |  1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

Comments

Jason Wang April 5, 2016, 2:05 a.m. UTC | #1 
On 04/04/2016 03:25 AM, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> A new feature bit 'VIRTIO_NET_F_GUEST_RSC' is introduced to support WHQL
> Receive-Segment-Offload test, this feature will coalesce tcp packets in
> IPv4/6 for userspace virtio-net driver.
>
> This feature can be enabled either by 'ACK' the feature when loading
> the driver in the guest, or by sending the 'VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET'
> command to the host via control queue.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> ---
>  hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++--
>  include/standard-headers/linux/virtio_net.h |  1 +
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5798f87..bd91a4b 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -537,6 +537,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
>          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
>          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_RSC);

Several questions here:

- I think RSC should work even without vnet_hdr?
- Need we differentiate ipv4 and ipv6 like TSO here?
- And looks like this patch should be squash to following patches.

>      }
>  
>      if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
> @@ -582,7 +583,8 @@ static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
>          (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>          (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
>          (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
> -        (1ULL << VIRTIO_NET_F_GUEST_UFO);
> +        (1ULL << VIRTIO_NET_F_GUEST_UFO)  |
> +        (1ULL << VIRTIO_NET_F_GUEST_RSC);

Looks like this is unnecessary since we won't set peer offload based on
GUEST_RSC.

>  
>      return guest_offloads_mask & features;
>  }
> @@ -1089,7 +1091,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>      return 0;
>  }
>  
> -static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> +static ssize_t virtio_net_do_receive(NetClientState *nc,
> +                                     const uint8_t *buf, size_t size)
>  {
>      VirtIONet *n = qemu_get_nic_opaque(nc);
>      VirtIONetQueue *q = virtio_net_get_subqueue(nc);
> @@ -1685,6 +1688,26 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
>      return 0;
>  }
>  
> +
> +static ssize_t virtio_net_rsc_receive(NetClientState *nc,
> +                                      const uint8_t *buf, size_t size)
> +{
> +    return virtio_net_do_receive(nc, buf, size);
> +}
> +
> +static ssize_t virtio_net_receive(NetClientState *nc,
> +                                  const uint8_t *buf, size_t size)
> +{
> +    VirtIONet *n;
> +
> +    n = qemu_get_nic_opaque(nc);
> +    if (n->curr_guest_offloads & VIRTIO_NET_F_GUEST_RSC) {
> +        return virtio_net_rsc_receive(nc, buf, size);
> +    } else {
> +        return virtio_net_do_receive(nc, buf, size);
> +    }
> +}

The changes here looks odd since it did nothing. Like I've mentioned,
better merge the patch into following ones.

> +
>  static NetClientInfo net_virtio_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_NIC,
>      .size = sizeof(NICState),
> @@ -1909,6 +1932,8 @@ static Property virtio_net_properties[] = {
>                         TX_TIMER_INTERVAL),
>      DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
>      DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
> +    DEFINE_PROP_BIT("guest_rsc", VirtIONet, host_features,
> +                    VIRTIO_NET_F_GUEST_RSC, true),

Need to compat the bit for old machine type to unbreak migration I believe?

Btw, also need a patch for virtio spec.

Thanks

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> index a78f33e..5b95762 100644
> --- a/include/standard-headers/linux/virtio_net.h
> +++ b/include/standard-headers/linux/virtio_net.h
> @@ -55,6 +55,7 @@
>  #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
>  					 * Steering */
>  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> +#define VIRTIO_NET_F_GUEST_RSC  24      /* Guest can coalesce tcp packets */
>  
>  #ifndef VIRTIO_NET_NO_LEGACY
>  #define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
Michael S. Tsirkin April 5, 2016, 8:17 a.m. UTC | #2 
On Tue, Apr 05, 2016 at 10:05:17AM +0800, Jason Wang wrote:
> 
> 
> On 04/04/2016 03:25 AM, wexu@redhat.com wrote:
> > From: Wei Xu <wexu@redhat.com>
> >
> > A new feature bit 'VIRTIO_NET_F_GUEST_RSC' is introduced to support WHQL
> > Receive-Segment-Offload test, this feature will coalesce tcp packets in
> > IPv4/6 for userspace virtio-net driver.
> >
> > This feature can be enabled either by 'ACK' the feature when loading
> > the driver in the guest, or by sending the 'VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET'
> > command to the host via control queue.
> >
> > Signed-off-by: Wei Xu <wexu@redhat.com>
> > ---
> >  hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++--
> >  include/standard-headers/linux/virtio_net.h |  1 +
> >  2 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 5798f87..bd91a4b 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -537,6 +537,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
> >          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
> >          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
> > +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_RSC);
> 
> Several questions here:
> 
> - I think RSC should work even without vnet_hdr?
> - Need we differentiate ipv4 and ipv6 like TSO here?
> - And looks like this patch should be squash to following patches.
> 
> >      }
> >  
> >      if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
> > @@ -582,7 +583,8 @@ static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
> >          (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> >          (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
> >          (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
> > -        (1ULL << VIRTIO_NET_F_GUEST_UFO);
> > +        (1ULL << VIRTIO_NET_F_GUEST_UFO)  |
> > +        (1ULL << VIRTIO_NET_F_GUEST_RSC);
> 
> Looks like this is unnecessary since we won't set peer offload based on
> GUEST_RSC.
> 
> >  
> >      return guest_offloads_mask & features;
> >  }
> > @@ -1089,7 +1091,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> >      return 0;
> >  }
> >  
> > -static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> > +static ssize_t virtio_net_do_receive(NetClientState *nc,
> > +                                     const uint8_t *buf, size_t size)
> >  {
> >      VirtIONet *n = qemu_get_nic_opaque(nc);
> >      VirtIONetQueue *q = virtio_net_get_subqueue(nc);
> > @@ -1685,6 +1688,26 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
> >      return 0;
> >  }
> >  
> > +
> > +static ssize_t virtio_net_rsc_receive(NetClientState *nc,
> > +                                      const uint8_t *buf, size_t size)
> > +{
> > +    return virtio_net_do_receive(nc, buf, size);
> > +}
> > +
> > +static ssize_t virtio_net_receive(NetClientState *nc,
> > +                                  const uint8_t *buf, size_t size)
> > +{
> > +    VirtIONet *n;
> > +
> > +    n = qemu_get_nic_opaque(nc);
> > +    if (n->curr_guest_offloads & VIRTIO_NET_F_GUEST_RSC) {
> > +        return virtio_net_rsc_receive(nc, buf, size);
> > +    } else {
> > +        return virtio_net_do_receive(nc, buf, size);
> > +    }
> > +}
> 
> The changes here looks odd since it did nothing. Like I've mentioned,
> better merge the patch into following ones.
> 
> > +
> >  static NetClientInfo net_virtio_info = {
> >      .type = NET_CLIENT_OPTIONS_KIND_NIC,
> >      .size = sizeof(NICState),
> > @@ -1909,6 +1932,8 @@ static Property virtio_net_properties[] = {
> >                         TX_TIMER_INTERVAL),
> >      DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
> >      DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
> > +    DEFINE_PROP_BIT("guest_rsc", VirtIONet, host_features,
> > +                    VIRTIO_NET_F_GUEST_RSC, true),
> 
> Need to compat the bit for old machine type to unbreak migration I believe?

And definitely disable by default.

> Btw, also need a patch for virtio spec.
> 
> Thanks
> 
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> > index a78f33e..5b95762 100644
> > --- a/include/standard-headers/linux/virtio_net.h
> > +++ b/include/standard-headers/linux/virtio_net.h
> > @@ -55,6 +55,7 @@
> >  #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
> >  					 * Steering */
> >  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> > +#define VIRTIO_NET_F_GUEST_RSC  24      /* Guest can coalesce tcp packets */
> >  
> >  #ifndef VIRTIO_NET_NO_LEGACY
> >  #define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
Wei Xu April 10, 2016, 3:23 p.m. UTC | #3 
On 2016?04?05? 16:17, Michael S. Tsirkin wrote:
> On Tue, Apr 05, 2016 at 10:05:17AM +0800, Jason Wang wrote:
>>
>> On 04/04/2016 03:25 AM, wexu@redhat.com wrote:
>>> From: Wei Xu <wexu@redhat.com>
>>>
>>> A new feature bit 'VIRTIO_NET_F_GUEST_RSC' is introduced to support WHQL
>>> Receive-Segment-Offload test, this feature will coalesce tcp packets in
>>> IPv4/6 for userspace virtio-net driver.
>>>
>>> This feature can be enabled either by 'ACK' the feature when loading
>>> the driver in the guest, or by sending the 'VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET'
>>> command to the host via control queue.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> ---
>>>   hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++--
>>>   include/standard-headers/linux/virtio_net.h |  1 +
>>>   2 files changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 5798f87..bd91a4b 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -537,6 +537,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>           virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
>>>           virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
>>>           virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
>>> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_RSC);
>> Several questions here:
>>
>> - I think RSC should work even without vnet_hdr?
That's interesting, but i'm wondering how to test this? could you please 
point me out?
>> - Need we differentiate ipv4 and ipv6 like TSO here?
Sure, thanks.
>> - And looks like this patch should be squash to following patches.
OK.
>>
>>>       }
>>>   
>>>       if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
>>> @@ -582,7 +583,8 @@ static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
>>>           (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>>>           (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
>>>           (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
>>> -        (1ULL << VIRTIO_NET_F_GUEST_UFO);
>>> +        (1ULL << VIRTIO_NET_F_GUEST_UFO)  |
>>> +        (1ULL << VIRTIO_NET_F_GUEST_RSC);
>> Looks like this is unnecessary since we won't set peer offload based on
>> GUEST_RSC.
there is an exclusive check when handling set feature command from 
control queue, so looks it will broke the check if don't include this bit.
>>
>>>   
>>>       return guest_offloads_mask & features;
>>>   }
>>> @@ -1089,7 +1091,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>>>       return 0;
>>>   }
>>>   
>>> -static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>> +static ssize_t virtio_net_do_receive(NetClientState *nc,
>>> +                                     const uint8_t *buf, size_t size)
>>>   {
>>>       VirtIONet *n = qemu_get_nic_opaque(nc);
>>>       VirtIONetQueue *q = virtio_net_get_subqueue(nc);
>>> @@ -1685,6 +1688,26 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
>>>       return 0;
>>>   }
>>>   
>>> +
>>> +static ssize_t virtio_net_rsc_receive(NetClientState *nc,
>>> +                                      const uint8_t *buf, size_t size)
>>> +{
>>> +    return virtio_net_do_receive(nc, buf, size);
>>> +}
>>> +
>>> +static ssize_t virtio_net_receive(NetClientState *nc,
>>> +                                  const uint8_t *buf, size_t size)
>>> +{
>>> +    VirtIONet *n;
>>> +
>>> +    n = qemu_get_nic_opaque(nc);
>>> +    if (n->curr_guest_offloads & VIRTIO_NET_F_GUEST_RSC) {
>>> +        return virtio_net_rsc_receive(nc, buf, size);
>>> +    } else {
>>> +        return virtio_net_do_receive(nc, buf, size);
>>> +    }
>>> +}
>> The changes here looks odd since it did nothing. Like I've mentioned,
>> better merge the patch into following ones.
OK.
>>
>>> +
>>>   static NetClientInfo net_virtio_info = {
>>>       .type = NET_CLIENT_OPTIONS_KIND_NIC,
>>>       .size = sizeof(NICState),
>>> @@ -1909,6 +1932,8 @@ static Property virtio_net_properties[] = {
>>>                          TX_TIMER_INTERVAL),
>>>       DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
>>>       DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
>>> +    DEFINE_PROP_BIT("guest_rsc", VirtIONet, host_features,
>>> +                    VIRTIO_NET_F_GUEST_RSC, true),
>> Need to compat the bit for old machine type to unbreak migration I believe?
> And definitely disable by default.
There maybe some windows specific details about this, i'll discuss with 
Yan and update.
>
>> Btw, also need a patch for virtio spec.
Sure.
>>
>> Thanks
>>
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   
>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>>> index a78f33e..5b95762 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -55,6 +55,7 @@
>>>   #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
>>>   					 * Steering */
>>>   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>>> +#define VIRTIO_NET_F_GUEST_RSC  24      /* Guest can coalesce tcp packets */
>>>   
>>>   #ifndef VIRTIO_NET_NO_LEGACY
>>>   #define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
diff mbox

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5798f87..bd91a4b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -537,6 +537,7 @@  static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
+        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_RSC);
     }
 
     if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
@@ -582,7 +583,8 @@  static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
         (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
         (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
         (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
-        (1ULL << VIRTIO_NET_F_GUEST_UFO);
+        (1ULL << VIRTIO_NET_F_GUEST_UFO)  |
+        (1ULL << VIRTIO_NET_F_GUEST_RSC);
 
     return guest_offloads_mask & features;
 }
@@ -1089,7 +1091,8 @@  static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
     return 0;
 }
 
-static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+static ssize_t virtio_net_do_receive(NetClientState *nc,
+                                     const uint8_t *buf, size_t size)
 {
     VirtIONet *n = qemu_get_nic_opaque(nc);
     VirtIONetQueue *q = virtio_net_get_subqueue(nc);
@@ -1685,6 +1688,26 @@  static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+
+static ssize_t virtio_net_rsc_receive(NetClientState *nc,
+                                      const uint8_t *buf, size_t size)
+{
+    return virtio_net_do_receive(nc, buf, size);
+}
+
+static ssize_t virtio_net_receive(NetClientState *nc,
+                                  const uint8_t *buf, size_t size)
+{
+    VirtIONet *n;
+
+    n = qemu_get_nic_opaque(nc);
+    if (n->curr_guest_offloads & VIRTIO_NET_F_GUEST_RSC) {
+        return virtio_net_rsc_receive(nc, buf, size);
+    } else {
+        return virtio_net_do_receive(nc, buf, size);
+    }
+}
+
 static NetClientInfo net_virtio_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
@@ -1909,6 +1932,8 @@  static Property virtio_net_properties[] = {
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
+    DEFINE_PROP_BIT("guest_rsc", VirtIONet, host_features,
+                    VIRTIO_NET_F_GUEST_RSC, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index a78f33e..5b95762 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -55,6 +55,7 @@ 
 #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
+#define VIRTIO_NET_F_GUEST_RSC  24      /* Guest can coalesce tcp packets */
 
 #ifndef VIRTIO_NET_NO_LEGACY
 #define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */