| Message ID | 20220621140507.1246-6-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Eliminate multifd flush | expand |
Hello Juan, On Tue, 2022-06-21 at 16:05 +0200, Juan Quintela wrote: > We need to add a new flag to mean to sync at that point. > Notice that we still synchronize at the end of setup and at the end of > complete stages. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 2 +- > migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > 2 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 3f79df0b70..6627787fc2 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > DEFAULT_MIGRATE_ANNOUNCE_STEP), > /* We will change to false when we introduce the new mechanism */ > DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > - multifd_sync_each_iteration, true), > + multifd_sync_each_iteration, false), > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > diff --git a/migration/ram.c b/migration/ram.c > index 2c7289edad..6792986565 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -81,6 +81,7 @@ > #define RAM_SAVE_FLAG_XBZRLE 0x40 > /* 0x80 is reserved in migration.h start with 0x100 next */ > #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > XBZRLECacheStats xbzrle_counters; > > @@ -1482,6 +1483,7 @@ retry: > * associated with the search process. > * > * Returns: > + * <0: An error happened > * 0: no page found, give up > * 1: no page found, retry > * 2: page found > @@ -1510,6 +1512,13 @@ static int find_dirty_block(RAMState *rs, > PageSearchStatus *pss) > pss->page = 0; > pss->block = QLIST_NEXT_RCU(pss->block, next); > if (!pss->block) { > + if (!migrate_multifd_sync_each_iteration()) { > + int ret = multifd_send_sync_main(rs->f); > + if (ret < 0) { > + return ret; > + } > + qemu_put_be64(rs->f, RAM_SAVE_FLAG_MULTIFD_SYNC); > + } > /* > * If memory migration starts over, we will meet a dirtied page > * which may still exists in compression threads's ring, so we > @@ -2273,7 +2282,8 @@ static int ram_find_and_save_block(RAMState *rs) > if (!get_queued_page(rs, &pss)) { > /* priority queue empty, so just search for something dirty */ > int res = find_dirty_block(rs, &pss); > - if (res == 0) { > + if (res <= 0) { > + pages = res; > break; > } else if (res == 1) > continue; > @@ -2943,11 +2953,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > ram_control_before_iterate(f, RAM_CONTROL_SETUP); > ram_control_after_iterate(f, RAM_CONTROL_SETUP); > > - if (migrate_multifd_sync_each_iteration()) { > - ret = multifd_send_sync_main(f); > - if (ret < 0) { > - return ret; > - } (1) IIUC, the above lines were changed in 2/5 to be reverted now. Is that correct? was it expected? > + ret = multifd_send_sync_main(f); > + if (ret < 0) { > + return ret; > + } > + > + if (!migrate_multifd_sync_each_iteration()) { > + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); (2) I have done some testing with this patchset (because of MSG_ZEROCOPY) and it seems this part here is breaking migration from this build to 'older' builds (same commits except for this patchset): qemu-system-x86_64: Unknown combination of migration flags: 0x200 qemu-system-x86_64: error while loading state section id 2(ram) qemu-system-x86_64: load of migration failed: Invalid argument Which makes sense, since there is no RAM_SAVE_FLAG_MULTIFD_SYNC in older versions. Is this expected / desired ? Strange enough, it seems to be breaking even with this set in the sending part: --global migration.multifd-sync-each-iteration=on Was the idea of this config to allow migration to older qemu builds? > } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > qemu_fflush(f); > @@ -3127,13 +3139,14 @@ static int ram_save_complete(QEMUFile *f, void > *opaque) > return ret; > } > > - if (migrate_multifd_sync_each_iteration()) { > - ret = multifd_send_sync_main(rs->f); > - if (ret < 0) { > - return ret; > - } > + ret = multifd_send_sync_main(rs->f); > + if (ret < 0) { > + return ret; > } (3) Same as (1) > > + if (migrate_multifd_sync_each_iteration()) { > + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); > + } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > qemu_fflush(f); > > @@ -3800,7 +3813,9 @@ int ram_load_postcopy(QEMUFile *f) > } > decompress_data_with_multi_threads(f, page_buffer, len); > break; > - > + case RAM_SAVE_FLAG_MULTIFD_SYNC: > + multifd_recv_sync_main(); > + break; > case RAM_SAVE_FLAG_EOS: > /* normal exit */ > if (migrate_multifd_sync_each_iteration()) { > @@ -4079,6 +4094,9 @@ static int ram_load_precopy(QEMUFile *f) > break; > } > break; > + case RAM_SAVE_FLAG_MULTIFD_SYNC: > + multifd_recv_sync_main(); > + break; > case RAM_SAVE_FLAG_EOS: > /* normal exit */ > if (migrate_multifd_sync_each_iteration()) {
Leonardo Brás <leobras@redhat.com> wrote: > Hello Juan, > > On Tue, 2022-06-21 at 16:05 +0200, Juan Quintela wrote: >> We need to add a new flag to mean to sync at that point. >> Notice that we still synchronize at the end of setup and at the end of >> complete stages. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/migration.c | 2 +- >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ >> 2 files changed, 31 insertions(+), 13 deletions(-) >> >> @@ -2943,11 +2953,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> ram_control_before_iterate(f, RAM_CONTROL_SETUP); >> ram_control_after_iterate(f, RAM_CONTROL_SETUP); >> >> - if (migrate_multifd_sync_each_iteration()) { >> - ret = multifd_send_sync_main(f); >> - if (ret < 0) { >> - return ret; >> - } > > (1) IIUC, the above lines were changed in 2/5 to be reverted now. > Is that correct? was it expected? Yeap. The problem here is that (withouth this patchset) we synchrconize in three places: - ram_save_setup() - ram_save_iterate() - ram_save_complete() And we want to change it to: - ram_save_setup() - ram_save_complete() - And each time that we pass through the end of memory. (much less times than calls to ram_save_iterate). In the three places we send: RAM_SAVE_FLAG_EOS And that is what cause the synchronization. As we can't piggyback on RAM_SAVE_FLAG_EOS anymore, I added a new flag to synchronize it. The problem is that now (on setup and complete) we need to synchronize independently if we do the older way or the new one. On iterate we only synchronize on the old code, and on new code only when we reach the end of the memory. I *thought* it was clear this way, but I can do without the change if people think it is easier. >> + ret = multifd_send_sync_main(f); >> + if (ret < 0) { >> + return ret; >> + } >> + >> + if (!migrate_multifd_sync_each_iteration()) { >> + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); > > (2) I have done some testing with this patchset (because of MSG_ZEROCOPY) and it > seems this part here is breaking migration from this build to 'older' builds > (same commits except for this patchset): > > qemu-system-x86_64: Unknown combination of migration flags: 0x200 > qemu-system-x86_64: error while loading state section id 2(ram) > qemu-system-x86_64: load of migration failed: Invalid argument You can't do that O:-) (TM), that is the whole point that I added the multifd-sync-each-iteration property. It is true for old machine types, it is false for new machine types. If you try to play with that property, it is better than you know what you are doing (TM). > Which makes sense, since there is no RAM_SAVE_FLAG_MULTIFD_SYNC in older > versions. Is this expected / desired ? See previous paragraph. > Strange enough, it seems to be breaking even with this set in the sending part: > --global migration.multifd-sync-each-iteration=on > > Was the idea of this config to allow migration to older qemu builds? If you set it to on, it should work against and old qemu. By default it is set to on for old machine types, and only to on for new machine types. So you should have it right if you use new machine types. (If you use "pc" or "q35" machine types, you should now what you are doing for migrating machines. We do this kind of change all the time). >> } >> qemu_put_be64(f, RAM_SAVE_FLAG_EOS); >> qemu_fflush(f); >> @@ -3127,13 +3139,14 @@ static int ram_save_complete(QEMUFile *f, void >> *opaque) >> return ret; >> } >> >> - if (migrate_multifd_sync_each_iteration()) { >> - ret = multifd_send_sync_main(rs->f); >> - if (ret < 0) { >> - return ret; >> - } >> + ret = multifd_send_sync_main(rs->f); >> + if (ret < 0) { >> + return ret; >> } > > (3) Same as (1) Yeap, this is on purpose. If you feel that it is clearer the other way, I can change the patchset. Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > We need to add a new flag to mean to sync at that point. > Notice that we still synchronize at the end of setup and at the end of > complete stages. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 2 +- > migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > 2 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 3f79df0b70..6627787fc2 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > DEFAULT_MIGRATE_ANNOUNCE_STEP), > /* We will change to false when we introduce the new mechanism */ > DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > - multifd_sync_each_iteration, true), > + multifd_sync_each_iteration, false), > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > diff --git a/migration/ram.c b/migration/ram.c > index 2c7289edad..6792986565 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -81,6 +81,7 @@ > #define RAM_SAVE_FLAG_XBZRLE 0x40 > /* 0x80 is reserved in migration.h start with 0x100 next */ > #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 Note this is the very last usable flag! We could do with avoiding using them as flags where we dont need to. > XBZRLECacheStats xbzrle_counters; > > @@ -1482,6 +1483,7 @@ retry: > * associated with the search process. > * > * Returns: > + * <0: An error happened > * 0: no page found, give up > * 1: no page found, retry > * 2: page found > @@ -1510,6 +1512,13 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > pss->page = 0; > pss->block = QLIST_NEXT_RCU(pss->block, next); > if (!pss->block) { > + if (!migrate_multifd_sync_each_iteration()) { > + int ret = multifd_send_sync_main(rs->f); > + if (ret < 0) { > + return ret; > + } > + qemu_put_be64(rs->f, RAM_SAVE_FLAG_MULTIFD_SYNC); > + } > /* > * If memory migration starts over, we will meet a dirtied page > * which may still exists in compression threads's ring, so we > @@ -2273,7 +2282,8 @@ static int ram_find_and_save_block(RAMState *rs) > if (!get_queued_page(rs, &pss)) { > /* priority queue empty, so just search for something dirty */ > int res = find_dirty_block(rs, &pss); > - if (res == 0) { > + if (res <= 0) { > + pages = res; > break; > } else if (res == 1) > continue; > @@ -2943,11 +2953,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > ram_control_before_iterate(f, RAM_CONTROL_SETUP); > ram_control_after_iterate(f, RAM_CONTROL_SETUP); > > - if (migrate_multifd_sync_each_iteration()) { > - ret = multifd_send_sync_main(f); > - if (ret < 0) { > - return ret; > - } > + ret = multifd_send_sync_main(f); > + if (ret < 0) { > + return ret; > + } > + > + if (!migrate_multifd_sync_each_iteration()) { > + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); > } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > qemu_fflush(f); > @@ -3127,13 +3139,14 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > return ret; > } > > - if (migrate_multifd_sync_each_iteration()) { > - ret = multifd_send_sync_main(rs->f); > - if (ret < 0) { > - return ret; > - } It feels like you could have done that in the previous patch. Anyway, Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > + ret = multifd_send_sync_main(rs->f); > + if (ret < 0) { > + return ret; > } > > + if (migrate_multifd_sync_each_iteration()) { > + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); > + } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > qemu_fflush(f); > > @@ -3800,7 +3813,9 @@ int ram_load_postcopy(QEMUFile *f) > } > decompress_data_with_multi_threads(f, page_buffer, len); > break; > - > + case RAM_SAVE_FLAG_MULTIFD_SYNC: > + multifd_recv_sync_main(); > + break; > case RAM_SAVE_FLAG_EOS: > /* normal exit */ > if (migrate_multifd_sync_each_iteration()) { > @@ -4079,6 +4094,9 @@ static int ram_load_precopy(QEMUFile *f) > break; > } > break; > + case RAM_SAVE_FLAG_MULTIFD_SYNC: > + multifd_recv_sync_main(); > + break; > case RAM_SAVE_FLAG_EOS: > /* normal exit */ > if (migrate_multifd_sync_each_iteration()) { > -- > 2.34.1 >
On Tue, Jul 05, 2022 at 02:56:35PM +0100, Dr. David Alan Gilbert wrote: > * Juan Quintela (quintela@redhat.com) wrote: > > We need to add a new flag to mean to sync at that point. > > Notice that we still synchronize at the end of setup and at the end of > > complete stages. > > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > migration/migration.c | 2 +- > > migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > > 2 files changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index 3f79df0b70..6627787fc2 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > > DEFAULT_MIGRATE_ANNOUNCE_STEP), > > /* We will change to false when we introduce the new mechanism */ > > DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > > - multifd_sync_each_iteration, true), > > + multifd_sync_each_iteration, false), > > > > /* Migration capabilities */ > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > diff --git a/migration/ram.c b/migration/ram.c > > index 2c7289edad..6792986565 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -81,6 +81,7 @@ > > #define RAM_SAVE_FLAG_XBZRLE 0x40 > > /* 0x80 is reserved in migration.h start with 0x100 next */ > > #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > > +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > Note this is the very last usable flag! > We could do with avoiding using them as flags where we dont need to. Before it is too late, shouldn't we do #define RAM_SAVE_FLAG_BIGGER_FLAGS 0x200 to indicate that this will be followed by another uint64 value giving us another 64 flags to play with ? With regards, Daniel
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> We need to add a new flag to mean to sync at that point. >> Notice that we still synchronize at the end of setup and at the end of >> complete stages. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/migration.c | 2 +- >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ >> 2 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 3f79df0b70..6627787fc2 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { >> DEFAULT_MIGRATE_ANNOUNCE_STEP), >> /* We will change to false when we introduce the new mechanism */ >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, >> - multifd_sync_each_iteration, true), >> + multifd_sync_each_iteration, false), >> >> /* Migration capabilities */ >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> diff --git a/migration/ram.c b/migration/ram.c >> index 2c7289edad..6792986565 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -81,6 +81,7 @@ >> #define RAM_SAVE_FLAG_XBZRLE 0x40 >> /* 0x80 is reserved in migration.h start with 0x100 next */ >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > Note this is the very last usable flag! We can recover two flags right now: RAM_SAVE_FLAG_FULL is not used anymore. 0x80 is free since years ago. Once multifd is default, there are some other that could go. Later, Juan. > We could do with avoiding using them as flags where we dont need to. I can't really think on another way to do it. The other thing that I can do is just reuse one of the flags that don't make sense for multifd (RAM_SAVE_FLAG_ZERO after zero pages patch, RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). It looks worse to me. Later, Juan. > It feels like you could have done that in the previous patch. > Anyway, > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Thanks, Juan.
Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Jul 05, 2022 at 02:56:35PM +0100, Dr. David Alan Gilbert wrote: >> * Juan Quintela (quintela@redhat.com) wrote: >> > We need to add a new flag to mean to sync at that point. >> > Notice that we still synchronize at the end of setup and at the end of >> > complete stages. >> > >> > Signed-off-by: Juan Quintela <quintela@redhat.com> >> > --- >> > migration/migration.c | 2 +- >> > migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ >> > 2 files changed, 31 insertions(+), 13 deletions(-) >> > >> > diff --git a/migration/migration.c b/migration/migration.c >> > index 3f79df0b70..6627787fc2 100644 >> > --- a/migration/migration.c >> > +++ b/migration/migration.c >> > @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { >> > DEFAULT_MIGRATE_ANNOUNCE_STEP), >> > /* We will change to false when we introduce the new mechanism */ >> > DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, >> > - multifd_sync_each_iteration, true), >> > + multifd_sync_each_iteration, false), >> > >> > /* Migration capabilities */ >> > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> > diff --git a/migration/ram.c b/migration/ram.c >> > index 2c7289edad..6792986565 100644 >> > --- a/migration/ram.c >> > +++ b/migration/ram.c >> > @@ -81,6 +81,7 @@ >> > #define RAM_SAVE_FLAG_XBZRLE 0x40 >> > /* 0x80 is reserved in migration.h start with 0x100 next */ >> > #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 >> > +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 >> >> Note this is the very last usable flag! >> We could do with avoiding using them as flags where we dont need to. > > Before it is too late, shouldn't we do > > #define RAM_SAVE_FLAG_BIGGER_FLAGS 0x200 > > to indicate that this will be followed by another uint64 value > giving us another 64 flags to play with ? Dunno. We can recover 2 bits already as I told on the previous answer. And another two/three once that we move to multifd, so we should be ok (famous last words). Once told that, putting a comment saying what is the biggest possible value looks like a good idea. Later, Juan.
On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> We need to add a new flag to mean to sync at that point. > >> Notice that we still synchronize at the end of setup and at the end of > >> complete stages. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/migration.c | 2 +- > >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > >> 2 files changed, 31 insertions(+), 13 deletions(-) > >> > >> diff --git a/migration/migration.c b/migration/migration.c > >> index 3f79df0b70..6627787fc2 100644 > >> --- a/migration/migration.c > >> +++ b/migration/migration.c > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > >> DEFAULT_MIGRATE_ANNOUNCE_STEP), > >> /* We will change to false when we introduce the new mechanism */ > >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > >> - multifd_sync_each_iteration, true), > >> + multifd_sync_each_iteration, false), > >> > >> /* Migration capabilities */ > >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > >> diff --git a/migration/ram.c b/migration/ram.c > >> index 2c7289edad..6792986565 100644 > >> --- a/migration/ram.c > >> +++ b/migration/ram.c > >> @@ -81,6 +81,7 @@ > >> #define RAM_SAVE_FLAG_XBZRLE 0x40 > >> /* 0x80 is reserved in migration.h start with 0x100 next */ > >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > > > Note this is the very last usable flag! > > We can recover two flags right now: > > RAM_SAVE_FLAG_FULL is not used anymore. > 0x80 is free since years ago. > > Once multifd is default, there are some other that could go. Non-multifd migration isn't likely to go away any time soon, given distros desire to support migration between QEMU's with quite significantly different versions. So feels like quite a long time before we might reclaim more flags. > > We could do with avoiding using them as flags where we dont need to. > > I can't really think on another way to do it. The other thing that I > can do is just reuse one of the flags that don't make sense for multifd > (RAM_SAVE_FLAG_ZERO after zero pages patch, > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). Re-using flags based on use context differences feels like a recipe to confuse people. With regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote: > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > * Juan Quintela (quintela@redhat.com) wrote: > > >> We need to add a new flag to mean to sync at that point. > > >> Notice that we still synchronize at the end of setup and at the end of > > >> complete stages. > > >> > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > >> --- > > >> migration/migration.c | 2 +- > > >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > > >> 2 files changed, 31 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/migration/migration.c b/migration/migration.c > > >> index 3f79df0b70..6627787fc2 100644 > > >> --- a/migration/migration.c > > >> +++ b/migration/migration.c > > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > > >> DEFAULT_MIGRATE_ANNOUNCE_STEP), > > >> /* We will change to false when we introduce the new mechanism */ > > >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > > >> - multifd_sync_each_iteration, true), > > >> + multifd_sync_each_iteration, false), > > >> > > >> /* Migration capabilities */ > > >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > >> diff --git a/migration/ram.c b/migration/ram.c > > >> index 2c7289edad..6792986565 100644 > > >> --- a/migration/ram.c > > >> +++ b/migration/ram.c > > >> @@ -81,6 +81,7 @@ > > >> #define RAM_SAVE_FLAG_XBZRLE 0x40 > > >> /* 0x80 is reserved in migration.h start with 0x100 next */ > > >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > > > > > Note this is the very last usable flag! > > > > We can recover two flags right now: > > > > RAM_SAVE_FLAG_FULL is not used anymore. > > 0x80 is free since years ago. > > > > Once multifd is default, there are some other that could go. I have suggested that a few times in the past. > Non-multifd migration isn't likely to go away any time soon, given > distros desire to support migration between QEMU's with quite > significantly different versions. So feels like quite a long time > before we might reclaim more flags. > > > > We could do with avoiding using them as flags where we dont need to. > > > > I can't really think on another way to do it. The other thing that I > > can do is just reuse one of the flags that don't make sense for multifd > > (RAM_SAVE_FLAG_ZERO after zero pages patch, > > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). > > Re-using flags based on use context differences feels like a recipe > to confuse people. Note that most of these things aren't really 'flags'; in the sense that only a few of them are actually combinable; so we should start using combinations to mean things new. Dave > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Tue, Jul 05, 2022 at 06:13:40PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote: > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > > * Juan Quintela (quintela@redhat.com) wrote: > > > >> We need to add a new flag to mean to sync at that point. > > > >> Notice that we still synchronize at the end of setup and at the end of > > > >> complete stages. > > > >> > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > > >> --- > > > >> migration/migration.c | 2 +- > > > >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > > > >> 2 files changed, 31 insertions(+), 13 deletions(-) > > > >> > > > >> diff --git a/migration/migration.c b/migration/migration.c > > > >> index 3f79df0b70..6627787fc2 100644 > > > >> --- a/migration/migration.c > > > >> +++ b/migration/migration.c > > > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > > > >> DEFAULT_MIGRATE_ANNOUNCE_STEP), > > > >> /* We will change to false when we introduce the new mechanism */ > > > >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > > > >> - multifd_sync_each_iteration, true), > > > >> + multifd_sync_each_iteration, false), > > > >> > > > >> /* Migration capabilities */ > > > >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > > >> diff --git a/migration/ram.c b/migration/ram.c > > > >> index 2c7289edad..6792986565 100644 > > > >> --- a/migration/ram.c > > > >> +++ b/migration/ram.c > > > >> @@ -81,6 +81,7 @@ > > > >> #define RAM_SAVE_FLAG_XBZRLE 0x40 > > > >> /* 0x80 is reserved in migration.h start with 0x100 next */ > > > >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > > > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > > > > > > > Note this is the very last usable flag! > > > > > > We can recover two flags right now: > > > > > > RAM_SAVE_FLAG_FULL is not used anymore. > > > 0x80 is free since years ago. > > > > > > Once multifd is default, there are some other that could go. > > I have suggested that a few times in the past. > > > Non-multifd migration isn't likely to go away any time soon, given > > distros desire to support migration between QEMU's with quite > > significantly different versions. So feels like quite a long time > > before we might reclaim more flags. > > > > > > We could do with avoiding using them as flags where we dont need to. > > > > > > I can't really think on another way to do it. The other thing that I > > > can do is just reuse one of the flags that don't make sense for multifd > > > (RAM_SAVE_FLAG_ZERO after zero pages patch, > > > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). > > > > Re-using flags based on use context differences feels like a recipe > > to confuse people. > > Note that most of these things aren't really 'flags'; in the sense that > only a few of them are actually combinable; so we should start using > combinations to mean things new. IOW, treat the field as an enum of valid values instead, and just define enum entries for the few valid combinations, giving us many more values to play with ? With regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Tue, Jul 05, 2022 at 06:13:40PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote: > > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > > > > * Juan Quintela (quintela@redhat.com) wrote: > > > > >> We need to add a new flag to mean to sync at that point. > > > > >> Notice that we still synchronize at the end of setup and at the end of > > > > >> complete stages. > > > > >> > > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > >> --- > > > > >> migration/migration.c | 2 +- > > > > >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ > > > > >> 2 files changed, 31 insertions(+), 13 deletions(-) > > > > >> > > > > >> diff --git a/migration/migration.c b/migration/migration.c > > > > >> index 3f79df0b70..6627787fc2 100644 > > > > >> --- a/migration/migration.c > > > > >> +++ b/migration/migration.c > > > > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { > > > > >> DEFAULT_MIGRATE_ANNOUNCE_STEP), > > > > >> /* We will change to false when we introduce the new mechanism */ > > > > >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, > > > > >> - multifd_sync_each_iteration, true), > > > > >> + multifd_sync_each_iteration, false), > > > > >> > > > > >> /* Migration capabilities */ > > > > >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > > > >> diff --git a/migration/ram.c b/migration/ram.c > > > > >> index 2c7289edad..6792986565 100644 > > > > >> --- a/migration/ram.c > > > > >> +++ b/migration/ram.c > > > > >> @@ -81,6 +81,7 @@ > > > > >> #define RAM_SAVE_FLAG_XBZRLE 0x40 > > > > >> /* 0x80 is reserved in migration.h start with 0x100 next */ > > > > >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 > > > > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 > > > > > > > > > > Note this is the very last usable flag! > > > > > > > > We can recover two flags right now: > > > > > > > > RAM_SAVE_FLAG_FULL is not used anymore. > > > > 0x80 is free since years ago. > > > > > > > > Once multifd is default, there are some other that could go. > > > > I have suggested that a few times in the past. > > > > > Non-multifd migration isn't likely to go away any time soon, given > > > distros desire to support migration between QEMU's with quite > > > significantly different versions. So feels like quite a long time > > > before we might reclaim more flags. > > > > > > > > We could do with avoiding using them as flags where we dont need to. > > > > > > > > I can't really think on another way to do it. The other thing that I > > > > can do is just reuse one of the flags that don't make sense for multifd > > > > (RAM_SAVE_FLAG_ZERO after zero pages patch, > > > > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). > > > > > > Re-using flags based on use context differences feels like a recipe > > > to confuse people. > > > > Note that most of these things aren't really 'flags'; in the sense that > > only a few of them are actually combinable; so we should start using > > combinations to mean things new. > > IOW, treat the field as an enum of valid values instead, and just > define enum entries for the few valid combinations, giving us many > more values to play with ? Right; some care needs to be taken with the ones that were interpreted as flags; but since you're not going to send the new values to an old qemu, you've got quite a bit of flexibility. Dave > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: >> On Tue, Jul 05, 2022 at 06:13:40PM +0100, Dr. David Alan Gilbert wrote: >> > * Daniel P. Berrangé (berrange@redhat.com) wrote: >> > > On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote: >> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > > > > * Juan Quintela (quintela@redhat.com) wrote: >> > > > >> We need to add a new flag to mean to sync at that point. >> > > > >> Notice that we still synchronize at the end of setup and at the end of >> > > > >> complete stages. >> > > > >> >> > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> > > > >> --- >> > > > >> migration/migration.c | 2 +- >> > > > >> migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ >> > > > >> 2 files changed, 31 insertions(+), 13 deletions(-) >> > > > >> >> > > > >> diff --git a/migration/migration.c b/migration/migration.c >> > > > >> index 3f79df0b70..6627787fc2 100644 >> > > > >> --- a/migration/migration.c >> > > > >> +++ b/migration/migration.c >> > > > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { >> > > > >> DEFAULT_MIGRATE_ANNOUNCE_STEP), >> > > > >> /* We will change to false when we introduce the new mechanism */ >> > > > >> DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, >> > > > >> - multifd_sync_each_iteration, true), >> > > > >> + multifd_sync_each_iteration, false), >> > > > >> >> > > > >> /* Migration capabilities */ >> > > > >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> > > > >> diff --git a/migration/ram.c b/migration/ram.c >> > > > >> index 2c7289edad..6792986565 100644 >> > > > >> --- a/migration/ram.c >> > > > >> +++ b/migration/ram.c >> > > > >> @@ -81,6 +81,7 @@ >> > > > >> #define RAM_SAVE_FLAG_XBZRLE 0x40 >> > > > >> /* 0x80 is reserved in migration.h start with 0x100 next */ >> > > > >> #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 >> > > > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 >> > > > > >> > > > > Note this is the very last usable flag! >> > > > >> > > > We can recover two flags right now: >> > > > >> > > > RAM_SAVE_FLAG_FULL is not used anymore. >> > > > 0x80 is free since years ago. >> > > > >> > > > Once multifd is default, there are some other that could go. >> > >> > I have suggested that a few times in the past. >> > >> > > Non-multifd migration isn't likely to go away any time soon, given >> > > distros desire to support migration between QEMU's with quite >> > > significantly different versions. So feels like quite a long time >> > > before we might reclaim more flags. >> > > >> > > > > We could do with avoiding using them as flags where we dont need to. >> > > > >> > > > I can't really think on another way to do it. The other thing that I >> > > > can do is just reuse one of the flags that don't make sense for multifd >> > > > (RAM_SAVE_FLAG_ZERO after zero pages patch, >> > > > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE). >> > > >> > > Re-using flags based on use context differences feels like a recipe >> > > to confuse people. >> > >> > Note that most of these things aren't really 'flags'; in the sense that >> > only a few of them are actually combinable; so we should start using >> > combinations to mean things new. >> >> IOW, treat the field as an enum of valid values instead, and just >> define enum entries for the few valid combinations, giving us many >> more values to play with ? > > Right; some care needs to be taken with the ones that were interpreted > as flags; but since you're not going to send the new values to an old > qemu, you've got quite a bit of flexibility. Rigth now no combinations are allowed, so we are free to play with that combination thing. Reception side code is: switch (flags & ~RAM_SAVE_FLAG_CONTINUE) { case RAM_SAVE_FLAG_MEM_SIZE: .... break; case RAM_SAVE_FLAG_ZERO: ... break; case RAM_SAVE_FLAG_PAGE: .... break; case RAM_SAVE_FLAG_COMPRESS_PAGE: .... break; case RAM_SAVE_FLAG_XBZRLE: .... break; case RAM_SAVE_FLAG_MULTIFD_SYNC: ... break; case RAM_SAVE_FLAG_EOS: .... break; default: if (flags & RAM_SAVE_FLAG_HOOK) { ..... } } So the only value that is a flag is the CONTINUE one, there are not other combinations with other flags. Yes, the RAM_SAVE_FLAG_HOOK is as weird as it can be. Later, Juan.
diff --git a/migration/migration.c b/migration/migration.c index 3f79df0b70..6627787fc2 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -4283,7 +4283,7 @@ static Property migration_properties[] = { DEFAULT_MIGRATE_ANNOUNCE_STEP), /* We will change to false when we introduce the new mechanism */ DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState, - multifd_sync_each_iteration, true), + multifd_sync_each_iteration, false), /* Migration capabilities */ DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), diff --git a/migration/ram.c b/migration/ram.c index 2c7289edad..6792986565 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -81,6 +81,7 @@ #define RAM_SAVE_FLAG_XBZRLE 0x40 /* 0x80 is reserved in migration.h start with 0x100 next */ #define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100 +#define RAM_SAVE_FLAG_MULTIFD_SYNC 0x200 XBZRLECacheStats xbzrle_counters; @@ -1482,6 +1483,7 @@ retry: * associated with the search process. * * Returns: + * <0: An error happened * 0: no page found, give up * 1: no page found, retry * 2: page found @@ -1510,6 +1512,13 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) pss->page = 0; pss->block = QLIST_NEXT_RCU(pss->block, next); if (!pss->block) { + if (!migrate_multifd_sync_each_iteration()) { + int ret = multifd_send_sync_main(rs->f); + if (ret < 0) { + return ret; + } + qemu_put_be64(rs->f, RAM_SAVE_FLAG_MULTIFD_SYNC); + } /* * If memory migration starts over, we will meet a dirtied page * which may still exists in compression threads's ring, so we @@ -2273,7 +2282,8 @@ static int ram_find_and_save_block(RAMState *rs) if (!get_queued_page(rs, &pss)) { /* priority queue empty, so just search for something dirty */ int res = find_dirty_block(rs, &pss); - if (res == 0) { + if (res <= 0) { + pages = res; break; } else if (res == 1) continue; @@ -2943,11 +2953,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque) ram_control_before_iterate(f, RAM_CONTROL_SETUP); ram_control_after_iterate(f, RAM_CONTROL_SETUP); - if (migrate_multifd_sync_each_iteration()) { - ret = multifd_send_sync_main(f); - if (ret < 0) { - return ret; - } + ret = multifd_send_sync_main(f); + if (ret < 0) { + return ret; + } + + if (!migrate_multifd_sync_each_iteration()) { + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); } qemu_put_be64(f, RAM_SAVE_FLAG_EOS); qemu_fflush(f); @@ -3127,13 +3139,14 @@ static int ram_save_complete(QEMUFile *f, void *opaque) return ret; } - if (migrate_multifd_sync_each_iteration()) { - ret = multifd_send_sync_main(rs->f); - if (ret < 0) { - return ret; - } + ret = multifd_send_sync_main(rs->f); + if (ret < 0) { + return ret; } + if (migrate_multifd_sync_each_iteration()) { + qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_SYNC); + } qemu_put_be64(f, RAM_SAVE_FLAG_EOS); qemu_fflush(f); @@ -3800,7 +3813,9 @@ int ram_load_postcopy(QEMUFile *f) } decompress_data_with_multi_threads(f, page_buffer, len); break; - + case RAM_SAVE_FLAG_MULTIFD_SYNC: + multifd_recv_sync_main(); + break; case RAM_SAVE_FLAG_EOS: /* normal exit */ if (migrate_multifd_sync_each_iteration()) { @@ -4079,6 +4094,9 @@ static int ram_load_precopy(QEMUFile *f) break; } break; + case RAM_SAVE_FLAG_MULTIFD_SYNC: + multifd_recv_sync_main(); + break; case RAM_SAVE_FLAG_EOS: /* normal exit */ if (migrate_multifd_sync_each_iteration()) {
We need to add a new flag to mean to sync at that point. Notice that we still synchronize at the end of setup and at the end of complete stages. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/migration.c | 2 +- migration/ram.c | 42 ++++++++++++++++++++++++++++++------------ 2 files changed, 31 insertions(+), 13 deletions(-)