diff mbox series

[v2,4/4] virtio-net: block migration if RSS feature negotiated

Message ID 20200309083438.2389-5-yuri.benditovich@daynix.com (mailing list archive)
State New, archived
Headers show
Series reference implementation of RSS | expand

Commit Message

Yuri Benditovich March 9, 2020, 8:34 a.m. UTC 
Block migration for reference implementation of
RSS feature in QEMU. When we add support for RSS
on backend side, we'll implement migration of
current RSS settings.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/net/virtio-net.c            | 18 ++++++++++++++++++
 include/hw/virtio/virtio-net.h |  1 +
 2 files changed, 19 insertions(+)

Comments

Jason Wang March 10, 2020, 3:12 a.m. UTC | #1 
On 2020/3/9 下午4:34, Yuri Benditovich wrote:
> Block migration for reference implementation of
> RSS feature in QEMU. When we add support for RSS
> on backend side, we'll implement migration of
> current RSS settings.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>   hw/net/virtio-net.c            | 18 ++++++++++++++++++
>   include/hw/virtio/virtio-net.h |  1 +
>   2 files changed, 19 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index abc41fdb16..943d1931a2 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -37,6 +37,7 @@
>   #include "qapi/qapi-events-migration.h"
>   #include "hw/virtio/virtio-access.h"
>   #include "migration/misc.h"
> +#include "migration/blocker.h"
>   #include "standard-headers/linux/ethtool.h"
>   #include "sysemu/sysemu.h"
>   #include "trace.h"
> @@ -627,6 +628,12 @@ static void virtio_net_reset(VirtIODevice *vdev)
>       n->announce_timer.round = 0;
>       n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>   
> +    if (n->migration_blocker) {
> +        migrate_del_blocker(n->migration_blocker);
> +        error_free(n->migration_blocker);
> +        n->migration_blocker = NULL;
> +    }
> +
>       /* Flush any MAC and VLAN filter table state */
>       n->mac_table.in_use = 0;
>       n->mac_table.first_multi = 0;
> @@ -1003,6 +1010,17 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>           vhost_net_ack_features(get_vhost_net(nc->peer), features);
>       }
>   
> +    if (virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
> +        if (!n->migration_blocker) {
> +            error_setg(&n->migration_blocker, "virtio-net: RSS negotiated");
> +            migrate_add_blocker(n->migration_blocker, &err);
> +            if (err) {
> +                error_report_err(err);
> +                err = NULL;
> +            }
> +        }
> +    }


It looks to me we should add the blocker unconditionally based on 
virtio_host_has_feature() instead of depending the negotiated feature here.

Thanks


> +
>       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
>           memset(n->vlans, 0, MAX_VLAN >> 3);
>       } else {
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 45670dd054..fba768ba9b 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -180,6 +180,7 @@ struct VirtIONet {
>       virtio_net_conf net_conf;
>       NICConf nic_conf;
>       DeviceState *qdev;
> +    Error *migration_blocker;
>       int multiqueue;
>       uint16_t max_queues;
>       uint16_t curr_queues;
Michael S. Tsirkin March 10, 2020, 6:17 a.m. UTC | #2 
On Tue, Mar 10, 2020 at 11:12:05AM +0800, Jason Wang wrote:
> 
> On 2020/3/9 下午4:34, Yuri Benditovich wrote:
> > Block migration for reference implementation of
> > RSS feature in QEMU. When we add support for RSS
> > on backend side, we'll implement migration of
> > current RSS settings.
> > 
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >   hw/net/virtio-net.c            | 18 ++++++++++++++++++
> >   include/hw/virtio/virtio-net.h |  1 +
> >   2 files changed, 19 insertions(+)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index abc41fdb16..943d1931a2 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -37,6 +37,7 @@
> >   #include "qapi/qapi-events-migration.h"
> >   #include "hw/virtio/virtio-access.h"
> >   #include "migration/misc.h"
> > +#include "migration/blocker.h"
> >   #include "standard-headers/linux/ethtool.h"
> >   #include "sysemu/sysemu.h"
> >   #include "trace.h"
> > @@ -627,6 +628,12 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >       n->announce_timer.round = 0;
> >       n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> > +    if (n->migration_blocker) {
> > +        migrate_del_blocker(n->migration_blocker);
> > +        error_free(n->migration_blocker);
> > +        n->migration_blocker = NULL;
> > +    }
> > +
> >       /* Flush any MAC and VLAN filter table state */
> >       n->mac_table.in_use = 0;
> >       n->mac_table.first_multi = 0;
> > @@ -1003,6 +1010,17 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> >           vhost_net_ack_features(get_vhost_net(nc->peer), features);
> >       }
> > +    if (virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
> > +        if (!n->migration_blocker) {
> > +            error_setg(&n->migration_blocker, "virtio-net: RSS negotiated");
> > +            migrate_add_blocker(n->migration_blocker, &err);
> > +            if (err) {
> > +                error_report_err(err);
> > +                err = NULL;
> > +            }
> > +        }
> > +    }
> 
> 
> It looks to me we should add the blocker unconditionally based on
> virtio_host_has_feature() instead of depending the negotiated feature here.
> 
> Thanks

I agree, this is a stopgap measure anyway.  If this is merged in its
current form, there should also be a big TODO here that we must fix this
ASAP, and maybe a warning produced.


> 
> > +
> >       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> >           memset(n->vlans, 0, MAX_VLAN >> 3);
> >       } else {
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 45670dd054..fba768ba9b 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -180,6 +180,7 @@ struct VirtIONet {
> >       virtio_net_conf net_conf;
> >       NICConf nic_conf;
> >       DeviceState *qdev;
> > +    Error *migration_blocker;
> >       int multiqueue;
> >       uint16_t max_queues;
> >       uint16_t curr_queues;
Yuri Benditovich March 10, 2020, 10:26 a.m. UTC | #3 
IMO, this does not depend on features of vhost as soon as we're not able to
provide parameters to it.


On Tue, Mar 10, 2020 at 8:17 AM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Mar 10, 2020 at 11:12:05AM +0800, Jason Wang wrote:
> >
> > On 2020/3/9 下午4:34, Yuri Benditovich wrote:
> > > Block migration for reference implementation of
> > > RSS feature in QEMU. When we add support for RSS
> > > on backend side, we'll implement migration of
> > > current RSS settings.
> > >
> > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > ---
> > >   hw/net/virtio-net.c            | 18 ++++++++++++++++++
> > >   include/hw/virtio/virtio-net.h |  1 +
> > >   2 files changed, 19 insertions(+)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index abc41fdb16..943d1931a2 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -37,6 +37,7 @@
> > >   #include "qapi/qapi-events-migration.h"
> > >   #include "hw/virtio/virtio-access.h"
> > >   #include "migration/misc.h"
> > > +#include "migration/blocker.h"
> > >   #include "standard-headers/linux/ethtool.h"
> > >   #include "sysemu/sysemu.h"
> > >   #include "trace.h"
> > > @@ -627,6 +628,12 @@ static void virtio_net_reset(VirtIODevice *vdev)
> > >       n->announce_timer.round = 0;
> > >       n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> > > +    if (n->migration_blocker) {
> > > +        migrate_del_blocker(n->migration_blocker);
> > > +        error_free(n->migration_blocker);
> > > +        n->migration_blocker = NULL;
> > > +    }
> > > +
> > >       /* Flush any MAC and VLAN filter table state */
> > >       n->mac_table.in_use = 0;
> > >       n->mac_table.first_multi = 0;
> > > @@ -1003,6 +1010,17 @@ static void
> virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> > >           vhost_net_ack_features(get_vhost_net(nc->peer), features);
> > >       }
> > > +    if (virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
> > > +        if (!n->migration_blocker) {
> > > +            error_setg(&n->migration_blocker, "virtio-net: RSS
> negotiated");
> > > +            migrate_add_blocker(n->migration_blocker, &err);
> > > +            if (err) {
> > > +                error_report_err(err);
> > > +                err = NULL;
> > > +            }
> > > +        }
> > > +    }
> >
> >
> > It looks to me we should add the blocker unconditionally based on
> > virtio_host_has_feature() instead of depending the negotiated feature
> here.
> >
> > Thanks
>
> I agree, this is a stopgap measure anyway.  If this is merged in its
> current form, there should also be a big TODO here that we must fix this
> ASAP, and maybe a warning produced.
>
>
> >
> > > +
> > >       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> > >           memset(n->vlans, 0, MAX_VLAN >> 3);
> > >       } else {
> > > diff --git a/include/hw/virtio/virtio-net.h
> b/include/hw/virtio/virtio-net.h
> > > index 45670dd054..fba768ba9b 100644
> > > --- a/include/hw/virtio/virtio-net.h
> > > +++ b/include/hw/virtio/virtio-net.h
> > > @@ -180,6 +180,7 @@ struct VirtIONet {
> > >       virtio_net_conf net_conf;
> > >       NICConf nic_conf;
> > >       DeviceState *qdev;
> > > +    Error *migration_blocker;
> > >       int multiqueue;
> > >       uint16_t max_queues;
> > >       uint16_t curr_queues;
>
>
diff mbox series

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index abc41fdb16..943d1931a2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -37,6 +37,7 @@ 
 #include "qapi/qapi-events-migration.h"
 #include "hw/virtio/virtio-access.h"
 #include "migration/misc.h"
+#include "migration/blocker.h"
 #include "standard-headers/linux/ethtool.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
@@ -627,6 +628,12 @@  static void virtio_net_reset(VirtIODevice *vdev)
     n->announce_timer.round = 0;
     n->status &= ~VIRTIO_NET_S_ANNOUNCE;
 
+    if (n->migration_blocker) {
+        migrate_del_blocker(n->migration_blocker);
+        error_free(n->migration_blocker);
+        n->migration_blocker = NULL;
+    }
+
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
     n->mac_table.first_multi = 0;
@@ -1003,6 +1010,17 @@  static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
         vhost_net_ack_features(get_vhost_net(nc->peer), features);
     }
 
+    if (virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
+        if (!n->migration_blocker) {
+            error_setg(&n->migration_blocker, "virtio-net: RSS negotiated");
+            migrate_add_blocker(n->migration_blocker, &err);
+            if (err) {
+                error_report_err(err);
+                err = NULL;
+            }
+        }
+    }
+
     if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
         memset(n->vlans, 0, MAX_VLAN >> 3);
     } else {
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 45670dd054..fba768ba9b 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -180,6 +180,7 @@  struct VirtIONet {
     virtio_net_conf net_conf;
     NICConf nic_conf;
     DeviceState *qdev;
+    Error *migration_blocker;
     int multiqueue;
     uint16_t max_queues;
     uint16_t curr_queues;