| Message ID | 20230213091548.76444-2-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Eliminate multifd flush | expand |
Juan Quintela <quintela@redhat.com> writes: > We used to synchronize all channels at the end of each RAM section > sent. That is not needed, so preparing to only synchronize once every > full round in latests patches. > > Notice that we initialize the property as true. We will change the > default when we introduce the new mechanism. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > Rename each-iteration to after-each-section > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > qapi/migration.json | 10 +++++++++- > migration/migration.h | 1 + > hw/core/machine.c | 1 + > migration/migration.c | 15 +++++++++++++-- > 4 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/qapi/migration.json b/qapi/migration.json > index c84fa10e86..2907241b9c 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -478,6 +478,13 @@ > # should not affect the correctness of postcopy migration. > # (since 7.1) > # > +# @multifd-sync-after-each-section: Synchronize channels after each > +# section is sent. What does it mean to synchronize channels? When would I want to, and why? > +# We used to do > +# that in the past, but it is > +# suboptimal. This isn't particularly helpful, I'm afraid. > +# Default value is true until all code is in. As far as I can tell, it's actually *unused* for now, and a later patch will put it to use ... > +# (since 8.0) > +# > # Features: > # @unstable: Members @x-colo and @x-ignore-shared are experimental. > # > @@ -492,7 +499,8 @@ > 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', > { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, > 'validate-uuid', 'background-snapshot', > - 'zero-copy-send', 'postcopy-preempt'] } > + 'zero-copy-send', 'postcopy-preempt', > + 'multifd-sync-after-each-section'] } > > ## > # @MigrationCapabilityStatus: > diff --git a/migration/migration.h b/migration/migration.h > index 2da2f8a164..cf84520196 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -424,6 +424,7 @@ int migrate_multifd_channels(void); > MultiFDCompression migrate_multifd_compression(void); > int migrate_multifd_zlib_level(void); > int migrate_multifd_zstd_level(void); > +bool migrate_multifd_sync_after_each_section(void); > > #ifdef CONFIG_LINUX > bool migrate_use_zero_copy_send(void); > diff --git a/hw/core/machine.c b/hw/core/machine.c > index f73fc4c45c..dc86849402 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -54,6 +54,7 @@ const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1); > GlobalProperty hw_compat_7_0[] = { > { "arm-gicv3-common", "force-8-bit-prio", "on" }, > { "nvme-ns", "eui64-default", "on"}, > + { "migration", "multifd-sync-after-each-section", "on"}, > }; > const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0); > > diff --git a/migration/migration.c b/migration/migration.c > index 90fca70cb7..406c27bc82 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -167,7 +167,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot, > MIGRATION_CAPABILITY_XBZRLE, > MIGRATION_CAPABILITY_X_COLO, > MIGRATION_CAPABILITY_VALIDATE_UUID, > - MIGRATION_CAPABILITY_ZERO_COPY_SEND); > + MIGRATION_CAPABILITY_ZERO_COPY_SEND, > + MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION); > > /* When we add fault tolerance, we could have several > migrations at once. For now we don't need to add > @@ -2701,6 +2702,15 @@ bool migrate_use_multifd(void) > return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD]; > } > > +bool migrate_multifd_sync_after_each_section(void) > +{ > + MigrationState *s = migrate_get_current(); > + > + return true; > + // We will change this when code gets in. > + return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; ... here. No warning about unreachable code? Checking... nope, gcc seems to not to care. > +} > + > bool migrate_pause_before_switchover(void) > { > MigrationState *s; > @@ -4535,7 +4545,8 @@ static Property migration_properties[] = { > DEFINE_PROP_MIG_CAP("x-zero-copy-send", > MIGRATION_CAPABILITY_ZERO_COPY_SEND), > #endif > - > + DEFINE_PROP_MIG_CAP("multifd-sync-after-each-section", > + MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION), > DEFINE_PROP_END_OF_LIST(), > };
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> We used to synchronize all channels at the end of each RAM section >> sent. That is not needed, so preparing to only synchronize once every >> full round in latests patches. >> >> Notice that we initialize the property as true. We will change the >> default when we introduce the new mechanism. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> --- >> >> Rename each-iteration to after-each-section >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> qapi/migration.json | 10 +++++++++- >> migration/migration.h | 1 + >> hw/core/machine.c | 1 + >> migration/migration.c | 15 +++++++++++++-- >> 4 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/qapi/migration.json b/qapi/migration.json >> index c84fa10e86..2907241b9c 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -478,6 +478,13 @@ >> # should not affect the correctness of postcopy migration. >> # (since 7.1) >> # >> +# @multifd-sync-after-each-section: Synchronize channels after each >> +# section is sent. > > What does it mean to synchronize channels? > > When would I want to, and why? > >> +# We used to do >> +# that in the past, but it is >> +# suboptimal. > > This isn't particularly helpful, I'm afraid. > >> +# Default value is true until all code is in. > > As far as I can tell, it's actually *unused* for now, and a later patch > will put it to use ... We (well, libvert preffers) want capabilities to be false by default. When I introduce a new capability/parameter: - Patch1: I introduce the capability/parameter, it does nothing yet. - Patch2: I conditionalize the old code on this capability. Default value is true (old code). - Patch3: I introduce the new code to implement the feature. At this point I change the default. Depending on complexity, Patch2 and 3 can be a series, but you get the idea O:-) >> +# (since 8.0) Retry. What about: # @multifd-sync-after-each-section: flush each channel after each # section sent. This assures that # we can't mix pages from one # iteration through the dirty bitmap # with pages for the following # iteration. We really only need to # do this flush after we have go # trhough all the dirty bitmap. For # historical reasons, we do that after # each section. This is suboptimal # (we flush too many times). # Default value is true until the code # to implement it is in tree. # (since 8.0) Better? >> +bool migrate_multifd_sync_after_each_section(void) >> +{ >> + MigrationState *s = migrate_get_current(); >> + >> + return true; >> + // We will change this when code gets in. >> + return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; > > ... here. > > No warning about unreachable code? Checking... nope, gcc seems to not > to care. Yeap. Gcc thinks this is ok. In others try's I have done: return true || s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; If you preffer I can change to this, not strong opinions. Later, Juan.
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> We used to synchronize all channels at the end of each RAM section >>> sent. That is not needed, so preparing to only synchronize once every >>> full round in latests patches. >>> >>> Notice that we initialize the property as true. We will change the >>> default when we introduce the new mechanism. >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> >>> --- >>> >>> Rename each-iteration to after-each-section >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> qapi/migration.json | 10 +++++++++- >>> migration/migration.h | 1 + >>> hw/core/machine.c | 1 + >>> migration/migration.c | 15 +++++++++++++-- >>> 4 files changed, 24 insertions(+), 3 deletions(-) >>> >>> diff --git a/qapi/migration.json b/qapi/migration.json >>> index c84fa10e86..2907241b9c 100644 >>> --- a/qapi/migration.json >>> +++ b/qapi/migration.json >>> @@ -478,6 +478,13 @@ >>> # should not affect the correctness of postcopy migration. >>> # (since 7.1) >>> # >>> +# @multifd-sync-after-each-section: Synchronize channels after each >>> +# section is sent. >> >> What does it mean to synchronize channels? >> >> When would I want to, and why? >> >>> +# We used to do >>> +# that in the past, but it is >>> +# suboptimal. >> >> This isn't particularly helpful, I'm afraid. >> >>> +# Default value is true until all code is in. >> >> As far as I can tell, it's actually *unused* for now, and a later patch >> will put it to use ... > > We (well, libvert preffers) want capabilities to be false by default. > When I introduce a new capability/parameter: > - Patch1: I introduce the capability/parameter, it does nothing yet. > - Patch2: I conditionalize the old code on this capability. > Default value is true (old code). > - Patch3: I introduce the new code to implement the feature. > At this point I change the default. > > Depending on complexity, Patch2 and 3 can be a series, but you get the > idea O:-) I'm fine with this approach, as long as commit messages and comments reflect reality :) >>> +# (since 8.0) > > Retry. What about: > > # @multifd-sync-after-each-section: flush each channel after each > # section sent. This assures that > # we can't mix pages from one > # iteration through the dirty bitmap > # with pages for the following > # iteration. We really only need to > # do this flush after we have go > # trhough all the dirty bitmap. For s/trhough/through/ > # historical reasons, we do that after > # each section. This is suboptimal > # (we flush too many times). > # Default value is true until the code > # to implement it is in tree. > # (since 8.0) > > > Better? Yes, except the comment suggests value false does something, which isn't true, yet. Possible solutions: 1. Accept only configurations that work as advertized: Patch1: add code to reject value false with a suitable "not implemented" error message. Since the behavior is temporary within a single series, documenting this feels optional. Patch2: no change. Patch3: drop the code rejecting false. 2. Document configurations that don't yet work as advertized: Patch1: doc comment states the capability is not yet implemented. Patch2: no change. Patch3: drop the comment. No need to mess with documenting temporary default true in either case. >>> +bool migrate_multifd_sync_after_each_section(void) >>> +{ >>> + MigrationState *s = migrate_get_current(); >>> + >>> + return true; >>> + // We will change this when code gets in. >>> + return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; >> >> ... here. >> >> No warning about unreachable code? Checking... nope, gcc seems to not >> to care. > > Yeap. Gcc thinks this is ok. > In others try's I have done: > > return true || s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; > > If you preffer I can change to this, not strong opinions. Matter of taste, you pick what you like best. I'd simply start with return true; /* TODO implement */ and replace it with the real expression when its callers are ready for it.
diff --git a/qapi/migration.json b/qapi/migration.json index c84fa10e86..2907241b9c 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -478,6 +478,13 @@ # should not affect the correctness of postcopy migration. # (since 7.1) # +# @multifd-sync-after-each-section: Synchronize channels after each +# section is sent. We used to do +# that in the past, but it is +# suboptimal. +# Default value is true until all code is in. +# (since 8.0) +# # Features: # @unstable: Members @x-colo and @x-ignore-shared are experimental. # @@ -492,7 +499,8 @@ 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, 'validate-uuid', 'background-snapshot', - 'zero-copy-send', 'postcopy-preempt'] } + 'zero-copy-send', 'postcopy-preempt', + 'multifd-sync-after-each-section'] } ## # @MigrationCapabilityStatus: diff --git a/migration/migration.h b/migration/migration.h index 2da2f8a164..cf84520196 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -424,6 +424,7 @@ int migrate_multifd_channels(void); MultiFDCompression migrate_multifd_compression(void); int migrate_multifd_zlib_level(void); int migrate_multifd_zstd_level(void); +bool migrate_multifd_sync_after_each_section(void); #ifdef CONFIG_LINUX bool migrate_use_zero_copy_send(void); diff --git a/hw/core/machine.c b/hw/core/machine.c index f73fc4c45c..dc86849402 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -54,6 +54,7 @@ const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1); GlobalProperty hw_compat_7_0[] = { { "arm-gicv3-common", "force-8-bit-prio", "on" }, { "nvme-ns", "eui64-default", "on"}, + { "migration", "multifd-sync-after-each-section", "on"}, }; const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0); diff --git a/migration/migration.c b/migration/migration.c index 90fca70cb7..406c27bc82 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -167,7 +167,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot, MIGRATION_CAPABILITY_XBZRLE, MIGRATION_CAPABILITY_X_COLO, MIGRATION_CAPABILITY_VALIDATE_UUID, - MIGRATION_CAPABILITY_ZERO_COPY_SEND); + MIGRATION_CAPABILITY_ZERO_COPY_SEND, + MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION); /* When we add fault tolerance, we could have several migrations at once. For now we don't need to add @@ -2701,6 +2702,15 @@ bool migrate_use_multifd(void) return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD]; } +bool migrate_multifd_sync_after_each_section(void) +{ + MigrationState *s = migrate_get_current(); + + return true; + // We will change this when code gets in. + return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION]; +} + bool migrate_pause_before_switchover(void) { MigrationState *s; @@ -4535,7 +4545,8 @@ static Property migration_properties[] = { DEFINE_PROP_MIG_CAP("x-zero-copy-send", MIGRATION_CAPABILITY_ZERO_COPY_SEND), #endif - + DEFINE_PROP_MIG_CAP("multifd-sync-after-each-section", + MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION), DEFINE_PROP_END_OF_LIST(), };