The existing notifiers should only apply to normal mode.
No functional change.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
hw/net/virtio-net.c | 4 ++++
hw/vfio/migration.c | 3 +++
include/migration/misc.h | 1 +
migration/migration.c | 5 +++++
net/vhost-vdpa.c | 4 ++++
ui/spice-core.c | 2 +-
6 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 80c56f0..0fa083d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3532,6 +3532,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
static void virtio_net_migration_state_notifier(Notifier *notifier, void *data)
{
MigrationState *s = data;
+
+ if (migrate_mode_of(s) != MIG_MODE_NORMAL) {
+ return;
+ }
VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
virtio_net_handle_migration_primary(n, s);
}
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 28d422b..814132a 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -763,6 +763,9 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data)
migration_state);
VFIODevice *vbasedev = migration->vbasedev;
+ if (migrate_mode_of(s) != MIG_MODE_NORMAL) {
+ return;
+ }
trace_vfio_migration_state_notifier(vbasedev->name,
MigrationStatus_str(s->state));
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 1bc8902..901d117 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -61,6 +61,7 @@ void migration_object_init(void);
void migration_shutdown(void);
bool migration_is_idle(void);
bool migration_is_active(MigrationState *);
+MigMode migrate_mode_of(MigrationState *);
void migration_add_notifier(Notifier *notify,
void (*func)(Notifier *notifier, void *data));
void migration_remove_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index 2cc7e2a..d5bfe70 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1559,6 +1559,11 @@ bool migration_is_active(MigrationState *s)
s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
}
+MigMode migrate_mode_of(MigrationState *s)
+{
+ return s->parameters.mode;
+}
+
int migrate_init(MigrationState *s, Error **errp)
{
int ret;
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index d0614d7..80acc4b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -336,6 +336,10 @@ static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data)
VhostVDPAState *s = container_of(notifier, VhostVDPAState,
migration_state);
+ if (migrate_mode_of(migration) != MIG_MODE_NORMAL) {
+ return;
+ }
+
if (migration_in_setup(migration)) {
vhost_vdpa_net_log_global_enable(s, true);
} else if (migration_has_failed(migration)) {
diff --git a/ui/spice-core.c b/ui/spice-core.c
index db21db2..0a04eb0 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -572,7 +572,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
{
MigrationState *s = data;
- if (!spice_have_target_host) {
+ if (!spice_have_target_host || migrate_mode_of(s) != MIG_MODE_NORMAL) {
return;
}
--
1.8.3.1On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote: > The existing notifiers should only apply to normal mode. > > No functional change. Instead of adding such check in every notifier, why not make CPR a separate list of notifiers? Just like the blocker lists. Aside of this patch, I just started to look at this "notifier" code, I really don't think we should pass in MigrationState* into the notifiers. IIUC we only need the "state" as an enum. Then with two separate registers, the device code knows the migration mode. What do you think? -- Peter Xu
On 1/10/2024 2:09 AM, Peter Xu wrote:
> On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote:
>> The existing notifiers should only apply to normal mode.
>>
>> No functional change.
>
> Instead of adding such check in every notifier, why not make CPR a separate
> list of notifiers? Just like the blocker lists.
Sure. I proposed minimal changes in this current series, but extending the
api to take migration mode would be nicer.
> Aside of this patch, I just started to look at this "notifier" code, I
> really don't think we should pass in MigrationState* into the notifiers.
> IIUC we only need the "state" as an enum. Then with two separate
> registers, the device code knows the migration mode.
>
> What do you think?
If we pass state, the notifier must either compare to enum values such as
MIGRATION_STATUS_COMPLETED instead of calling migration_has_finished(s), or
we must define new accessors such as migration_state_is_finished(state).
IMO passing MigrationState is the best approach.
MigrationState is an incomplete type in most notifiers, and the client can
pass it to a limited set of accessors to get more information -- exactly what
we want to hide migration internals. However, we could further limit the
allowed accessors, eg move these to a new file "include/migration/notifier.h".
----------------------------------------
#include "qemu/notify.h"
void migration_add_notifier(Notifier *notify,
void (*func)(Notifier *notifier, void *data));
void migration_remove_notifier(Notifier *notify);
bool migration_is_active(MigrationState *);
bool migration_in_setup(MigrationState *);
bool migration_has_finished(MigrationState *);
bool migration_has_failed(MigrationState *);
-----------------------------------------------
- Steve
On Wed, Jan 10, 2024 at 01:08:01PM -0500, Steven Sistare wrote: > On 1/10/2024 2:09 AM, Peter Xu wrote: > > On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote: > >> The existing notifiers should only apply to normal mode. > >> > >> No functional change. > > > > Instead of adding such check in every notifier, why not make CPR a separate > > list of notifiers? Just like the blocker lists. > > Sure. I proposed minimal changes in this current series, but extending the > api to take migration mode would be nicer. > > > Aside of this patch, I just started to look at this "notifier" code, I > > really don't think we should pass in MigrationState* into the notifiers. > > IIUC we only need the "state" as an enum. Then with two separate > > registers, the device code knows the migration mode. > > > > What do you think? > > If we pass state, the notifier must either compare to enum values such as > MIGRATION_STATUS_COMPLETED instead of calling migration_has_finished(s), or > we must define new accessors such as migration_state_is_finished(state). > > IMO passing MigrationState is the best approach. > MigrationState is an incomplete type in most notifiers, and the client can > pass it to a limited set of accessors to get more information -- exactly what > we want to hide migration internals. However, we could further limit the > allowed accessors, eg move these to a new file "include/migration/notifier.h". > > ---------------------------------------- > #include "qemu/notify.h" > void migration_add_notifier(Notifier *notify, > void (*func)(Notifier *notifier, void *data)); > void migration_remove_notifier(Notifier *notify); > bool migration_is_active(MigrationState *); > bool migration_in_setup(MigrationState *); > bool migration_has_finished(MigrationState *); > bool migration_has_failed(MigrationState *); > ----------------------------------------------- Yes this also sounds good. Thanks, -- Peter Xu
© 2016 - 2025 Red Hat, Inc.