[PATCH V3] migration: simplify notifiers

Steve Sistare posted 1 patch 11 months ago
Failed in applying to current master (apply log)
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Leonardo Bras <leobras@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
hw/net/virtio-net.c      |  6 +++---
hw/vfio/migration.c      |  8 ++++----
include/migration/misc.h |  6 ++++--
migration/migration.c    | 22 ++++++++++++++++------
net/vhost-vdpa.c         |  7 ++++---
ui/spice-core.c          |  3 +--
6 files changed, 32 insertions(+), 20 deletions(-)
[PATCH V3] migration: simplify notifiers
Posted by Steve Sistare 11 months ago
Pass the callback function to add_migration_state_change_notifier so
that migration can initialize the notifier on add and clear it on
delete, which simplifies the call sites.  Shorten the function names
so the extra arg can be added more legibly.  Hide the global notifier
list in a new function migration_call_notifiers, and make it externally
visible so future live update code can call it.

No functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/net/virtio-net.c      |  6 +++---
 hw/vfio/migration.c      |  8 ++++----
 include/migration/misc.h |  6 ++++--
 migration/migration.c    | 22 ++++++++++++++++------
 net/vhost-vdpa.c         |  7 ++++---
 ui/spice-core.c          |  3 +--
 6 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6df6b73..c4dc795 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3605,8 +3605,8 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         n->primary_listener.hide_device = failover_hide_primary_device;
         qatomic_set(&n->failover_primary_hidden, true);
         device_listener_register(&n->primary_listener);
-        n->migration_state.notify = virtio_net_migration_state_notifier;
-        add_migration_state_change_notifier(&n->migration_state);
+        migration_add_notifier(&n->migration_state,
+                               virtio_net_migration_state_notifier);
         n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY);
     }
 
@@ -3769,7 +3769,7 @@ static void virtio_net_device_unrealize(DeviceState *dev)
     if (n->failover) {
         qobject_unref(n->primary_opts);
         device_listener_unregister(&n->primary_listener);
-        remove_migration_state_change_notifier(&n->migration_state);
+        migration_remove_notifier(&n->migration_state);
     } else {
         assert(n->primary_opts == NULL);
     }
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index c4656bb..8af0294 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -619,9 +619,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
     migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
                                                            vfio_vmstate_change,
                                                            vbasedev);
-    migration->migration_state.notify = vfio_migration_state_notifier;
-    add_migration_state_change_notifier(&migration->migration_state);
-
+    migration_add_notifier(&migration->migration_state,
+                           vfio_migration_state_notifier);
+    
     return 0;
 }
 
@@ -670,7 +670,7 @@ void vfio_migration_exit(VFIODevice *vbasedev)
     if (vbasedev->migration) {
         VFIOMigration *migration = vbasedev->migration;
 
-        remove_migration_state_change_notifier(&migration->migration_state);
+        migration_remove_notifier(&migration->migration_state);
         qemu_del_vm_change_state_handler(migration->vm_state);
         unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
         vfio_migration_free(vbasedev);
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 5ebe13b..0987eb1 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -59,8 +59,10 @@ void migration_object_init(void);
 void migration_shutdown(void);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
-void add_migration_state_change_notifier(Notifier *notify);
-void remove_migration_state_change_notifier(Notifier *notify);
+void migration_add_notifier(Notifier *notify,
+                            void (*func)(Notifier *notifier, void *data));
+void migration_remove_notifier(Notifier *notify);
+void migration_call_notifiers(MigrationState *s);
 bool migration_in_setup(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index 5103e2f..17b4b47 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1178,7 +1178,7 @@ static void migrate_fd_cleanup(MigrationState *s)
         /* It is used on info migrate.  We can't free it */
         error_report_err(error_copy(s->error));
     }
-    notifier_list_notify(&migration_state_notifiers, s);
+    migration_call_notifiers(s);
     block_cleanup_parameters();
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
@@ -1273,14 +1273,24 @@ static void migrate_fd_cancel(MigrationState *s)
     }
 }
 
-void add_migration_state_change_notifier(Notifier *notify)
+void migration_add_notifier(Notifier *notify,
+                            void (*func)(Notifier *notifier, void *data))
 {
+    notify->notify = func;
     notifier_list_add(&migration_state_notifiers, notify);
 }
 
-void remove_migration_state_change_notifier(Notifier *notify)
+void migration_remove_notifier(Notifier *notify)
 {
-    notifier_remove(notify);
+    if (notify->notify) {
+        notifier_remove(notify);
+        notify->notify = NULL;
+    }
+}
+
+void migration_call_notifiers(MigrationState *s)
+{
+    notifier_list_notify(&migration_state_notifiers, s);
 }
 
 bool migration_in_setup(MigrationState *s)
@@ -2191,7 +2201,7 @@ static int postcopy_start(MigrationState *ms)
      * spice needs to trigger a transition now
      */
     ms->postcopy_after_devices = true;
-    notifier_list_notify(&migration_state_notifiers, ms);
+    migration_call_notifiers(ms);
 
     ms->downtime =  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - time_at_stop;
 
@@ -3219,7 +3229,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         rate_limit = migrate_max_bandwidth();
 
         /* Notify before starting migration thread */
-        notifier_list_notify(&migration_state_notifiers, s);
+        migration_call_notifiers(s);
     }
 
     migration_rate_set(rate_limit);
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 37cdc84..1a4608b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -297,7 +297,8 @@ static void vhost_vdpa_net_data_start_first(VhostVDPAState *s)
 {
     struct vhost_vdpa *v = &s->vhost_vdpa;
 
-    add_migration_state_change_notifier(&s->migration_state);
+    migration_add_notifier(&s->migration_state,
+                           vdpa_net_migration_state_notifier);
     if (v->shadow_vqs_enabled) {
         v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
                                            v->iova_range.last);
@@ -341,7 +342,7 @@ static void vhost_vdpa_net_client_stop(NetClientState *nc)
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
     if (s->vhost_vdpa.index == 0) {
-        remove_migration_state_change_notifier(&s->migration_state);
+        migration_remove_notifier(&s->migration_state);
     }
 
     dev = s->vhost_vdpa.dev;
@@ -818,7 +819,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     s->vhost_vdpa.device_fd = vdpa_device_fd;
     s->vhost_vdpa.index = queue_pair_index;
     s->always_svq = svq;
-    s->migration_state.notify = vdpa_net_migration_state_notifier;
+    s->migration_state.notify = NULL;
     s->vhost_vdpa.shadow_vqs_enabled = svq;
     s->vhost_vdpa.iova_range = iova_range;
     s->vhost_vdpa.shadow_data = svq;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 52a5938..db21db2 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -821,8 +821,7 @@ static void qemu_spice_init(void)
     };
     using_spice = 1;
 
-    migration_state.notify = migration_state_notifier;
-    add_migration_state_change_notifier(&migration_state);
+    migration_add_notifier(&migration_state, migration_state_notifier);
     spice_migrate.base.sif = &migrate_interface.base;
     qemu_spice.add_interface(&spice_migrate.base);
 
-- 
1.8.3.1
Re: [PATCH V3] migration: simplify notifiers
Posted by Juan Quintela 6 months, 2 weeks ago
Steve Sistare <steven.sistare@oracle.com> wrote:
> Pass the callback function to add_migration_state_change_notifier so
> that migration can initialize the notifier on add and clear it on
> delete, which simplifies the call sites.  Shorten the function names
> so the extra arg can be added more legibly.  Hide the global notifier
> list in a new function migration_call_notifiers, and make it externally
> visible so future live update code can call it.
>
> No functional change.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.
Re: [PATCH V3] migration: simplify notifiers
Posted by Michael Galaxy 9 months, 3 weeks ago
On 6/7/23 09:42, Steve Sistare wrote:
> Pass the callback function to add_migration_state_change_notifier so
> that migration can initialize the notifier on add and clear it on
> delete, which simplifies the call sites.  Shorten the function names
> so the extra arg can be added more legibly.  Hide the global notifier
> list in a new function migration_call_notifiers, and make it externally
> visible so future live update code can call it.
Tested-by: Michael Galaxy <mgalaxy@akamai.com>
Reviewed-by: Michael Galaxy <mgalaxy@akamai.com>
> No functional change.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/net/virtio-net.c      |  6 +++---
>   hw/vfio/migration.c      |  8 ++++----
>   include/migration/misc.h |  6 ++++--
>   migration/migration.c    | 22 ++++++++++++++++------
>   net/vhost-vdpa.c         |  7 ++++---
>   ui/spice-core.c          |  3 +--
>   6 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6df6b73..c4dc795 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3605,8 +3605,8 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>           n->primary_listener.hide_device = failover_hide_primary_device;
>           qatomic_set(&n->failover_primary_hidden, true);
>           device_listener_register(&n->primary_listener);
> -        n->migration_state.notify = virtio_net_migration_state_notifier;
> -        add_migration_state_change_notifier(&n->migration_state);
> +        migration_add_notifier(&n->migration_state,
> +                               virtio_net_migration_state_notifier);
>           n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY);
>       }
>   
> @@ -3769,7 +3769,7 @@ static void virtio_net_device_unrealize(DeviceState *dev)
>       if (n->failover) {
>           qobject_unref(n->primary_opts);
>           device_listener_unregister(&n->primary_listener);
> -        remove_migration_state_change_notifier(&n->migration_state);
> +        migration_remove_notifier(&n->migration_state);
>       } else {
>           assert(n->primary_opts == NULL);
>       }
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index c4656bb..8af0294 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -619,9 +619,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
>       migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
>                                                              vfio_vmstate_change,
>                                                              vbasedev);
> -    migration->migration_state.notify = vfio_migration_state_notifier;
> -    add_migration_state_change_notifier(&migration->migration_state);
> -
> +    migration_add_notifier(&migration->migration_state,
> +                           vfio_migration_state_notifier);
> +
>       return 0;
>   }
>   
> @@ -670,7 +670,7 @@ void vfio_migration_exit(VFIODevice *vbasedev)
>       if (vbasedev->migration) {
>           VFIOMigration *migration = vbasedev->migration;
>   
> -        remove_migration_state_change_notifier(&migration->migration_state);
> +        migration_remove_notifier(&migration->migration_state);
>           qemu_del_vm_change_state_handler(migration->vm_state);
>           unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
>           vfio_migration_free(vbasedev);
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 5ebe13b..0987eb1 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -59,8 +59,10 @@ void migration_object_init(void);
>   void migration_shutdown(void);
>   bool migration_is_idle(void);
>   bool migration_is_active(MigrationState *);
> -void add_migration_state_change_notifier(Notifier *notify);
> -void remove_migration_state_change_notifier(Notifier *notify);
> +void migration_add_notifier(Notifier *notify,
> +                            void (*func)(Notifier *notifier, void *data));
> +void migration_remove_notifier(Notifier *notify);
> +void migration_call_notifiers(MigrationState *s);
>   bool migration_in_setup(MigrationState *);
>   bool migration_has_finished(MigrationState *);
>   bool migration_has_failed(MigrationState *);
> diff --git a/migration/migration.c b/migration/migration.c
> index 5103e2f..17b4b47 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1178,7 +1178,7 @@ static void migrate_fd_cleanup(MigrationState *s)
>           /* It is used on info migrate.  We can't free it */
>           error_report_err(error_copy(s->error));
>       }
> -    notifier_list_notify(&migration_state_notifiers, s);
> +    migration_call_notifiers(s);
>       block_cleanup_parameters();
>       yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>   }
> @@ -1273,14 +1273,24 @@ static void migrate_fd_cancel(MigrationState *s)
>       }
>   }
>   
> -void add_migration_state_change_notifier(Notifier *notify)
> +void migration_add_notifier(Notifier *notify,
> +                            void (*func)(Notifier *notifier, void *data))
>   {
> +    notify->notify = func;
>       notifier_list_add(&migration_state_notifiers, notify);
>   }
>   
> -void remove_migration_state_change_notifier(Notifier *notify)
> +void migration_remove_notifier(Notifier *notify)
>   {
> -    notifier_remove(notify);
> +    if (notify->notify) {
> +        notifier_remove(notify);
> +        notify->notify = NULL;
> +    }
> +}
> +
> +void migration_call_notifiers(MigrationState *s)
> +{
> +    notifier_list_notify(&migration_state_notifiers, s);
>   }
>   
>   bool migration_in_setup(MigrationState *s)
> @@ -2191,7 +2201,7 @@ static int postcopy_start(MigrationState *ms)
>        * spice needs to trigger a transition now
>        */
>       ms->postcopy_after_devices = true;
> -    notifier_list_notify(&migration_state_notifiers, ms);
> +    migration_call_notifiers(ms);
>   
>       ms->downtime =  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - time_at_stop;
>   
> @@ -3219,7 +3229,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>           rate_limit = migrate_max_bandwidth();
>   
>           /* Notify before starting migration thread */
> -        notifier_list_notify(&migration_state_notifiers, s);
> +        migration_call_notifiers(s);
>       }
>   
>       migration_rate_set(rate_limit);
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 37cdc84..1a4608b 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -297,7 +297,8 @@ static void vhost_vdpa_net_data_start_first(VhostVDPAState *s)
>   {
>       struct vhost_vdpa *v = &s->vhost_vdpa;
>   
> -    add_migration_state_change_notifier(&s->migration_state);
> +    migration_add_notifier(&s->migration_state,
> +                           vdpa_net_migration_state_notifier);
>       if (v->shadow_vqs_enabled) {
>           v->iova_tree = vhost_iova_tree_new(v->iova_range.first,
>                                              v->iova_range.last);
> @@ -341,7 +342,7 @@ static void vhost_vdpa_net_client_stop(NetClientState *nc)
>       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>   
>       if (s->vhost_vdpa.index == 0) {
> -        remove_migration_state_change_notifier(&s->migration_state);
> +        migration_remove_notifier(&s->migration_state);
>       }
>   
>       dev = s->vhost_vdpa.dev;
> @@ -818,7 +819,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>       s->vhost_vdpa.device_fd = vdpa_device_fd;
>       s->vhost_vdpa.index = queue_pair_index;
>       s->always_svq = svq;
> -    s->migration_state.notify = vdpa_net_migration_state_notifier;
> +    s->migration_state.notify = NULL;
>       s->vhost_vdpa.shadow_vqs_enabled = svq;
>       s->vhost_vdpa.iova_range = iova_range;
>       s->vhost_vdpa.shadow_data = svq;
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 52a5938..db21db2 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -821,8 +821,7 @@ static void qemu_spice_init(void)
>       };
>       using_spice = 1;
>   
> -    migration_state.notify = migration_state_notifier;
> -    add_migration_state_change_notifier(&migration_state);
> +    migration_add_notifier(&migration_state, migration_state_notifier);
>       spice_migrate.base.sif = &migrate_interface.base;
>       qemu_spice.add_interface(&spice_migrate.base);
>
Re: [PATCH V3] migration: simplify notifiers
Posted by Peter Xu 11 months ago
On Wed, Jun 07, 2023 at 07:42:34AM -0700, Steve Sistare wrote:
> Pass the callback function to add_migration_state_change_notifier so
> that migration can initialize the notifier on add and clear it on
> delete, which simplifies the call sites.  Shorten the function names
> so the extra arg can be added more legibly.  Hide the global notifier
> list in a new function migration_call_notifiers, and make it externally
> visible so future live update code can call it.
> 
> No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu