Re: [PATCH V4] migration: simplify notifiers

[Steven Sistare <steven.sistare@oracle.com>]

I rebased the V3 patch and added the new maintainers and reviewers.
Peter has already reviewed it.

- Steve

On 10/18/2023 9:40 AM, 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>
> Tested-by: Michael Galaxy <mgalaxy@akamai.com>
> Reviewed-by: Michael Galaxy <mgalaxy@akamai.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/net/virtio-net.c      |  6 +++---
>  hw/vfio/migration.c      |  6 +++---
>  include/migration/misc.h |  6 ++++--
>  migration/migration.c    | 22 ++++++++++++++++------
>  net/vhost-vdpa.c         |  7 ++++---
>  ui/spice-core.c          |  3 +--
>  6 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 29e33ea..b85c794 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3624,8 +3624,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);
>      }
>  
> @@ -3788,7 +3788,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 0aaad65..28d422b 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -872,8 +872,8 @@ static int vfio_migration_init(VFIODevice *vbasedev)
>                       NULL;
>      migration->vm_state = qdev_add_vm_change_state_handler_full(
>          vbasedev->dev, vfio_vmstate_change, prepare_cb, 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;
>  }
> @@ -882,7 +882,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
>  {
>      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 7dcc0b5..673ac49 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -60,8 +60,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 65f9791..29a69bf 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1204,7 +1204,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);
>  }
> @@ -1308,14 +1308,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)
> @@ -2231,7 +2241,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       * 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;
>  
> @@ -3265,7 +3275,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 939c984..0f2e6fc 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -339,7 +339,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);
> @@ -399,7 +400,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;
> @@ -1456,7 +1457,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);
>