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

[Wei Xu]

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, w...@redhat.com wrote:

From: Wei Xu 

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 
---
  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(, VIRTIO_NET_F_GUEST_TSO4);
  virtio_clear_feature(, VIRTIO_NET_F_GUEST_TSO6);
  virtio_clear_feature(, VIRTIO_NET_F_GUEST_ECN);
+virtio_clear_feature(, 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 */

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

[Michael S. Tsirkin]

On Tue, Apr 05, 2016 at 10:05:17AM +0800, Jason Wang wrote:
> 
> 
> On 04/04/2016 03:25 AM, w...@redhat.com wrote:
> > From: Wei Xu 
> >
> > 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 
> > ---
> >  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(, VIRTIO_NET_F_GUEST_TSO4);
> >  virtio_clear_feature(, VIRTIO_NET_F_GUEST_TSO6);
> >  virtio_clear_feature(, VIRTIO_NET_F_GUEST_ECN);
> > +virtio_clear_feature(, 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_MQ22  /* 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 */

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

[Jason Wang]

On 04/04/2016 03:25 AM, w...@redhat.com wrote:
> From: Wei Xu 
>
> 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 
> ---
>  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(, VIRTIO_NET_F_GUEST_TSO4);
>  virtio_clear_feature(, VIRTIO_NET_F_GUEST_TSO6);
>  virtio_clear_feature(, VIRTIO_NET_F_GUEST_ECN);
> +virtio_clear_feature(, 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 */

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

[wexu]

From: Wei Xu 

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 
---
 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(, VIRTIO_NET_F_GUEST_TSO4);
 virtio_clear_feature(, VIRTIO_NET_F_GUEST_TSO6);
 virtio_clear_feature(, VIRTIO_NET_F_GUEST_ECN);
+virtio_clear_feature(, 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_MQ22  /* 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 */
-- 
2.5.0