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, address@hidden wrote:
> > > From: Wei Xu <address@hidden>
> > >
> > > 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 <address@hidden>
> > > ---
> > >   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 */