| Message ID | 20211224111148.345438-5-nikita.lapshin@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Add 'no-ram' and 'ram-only' cpabilities | expand |
24.12.2021 14:11, Nikita Lapshin wrote: > If this capability is enabled migration stream > will have RAM section only. > > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> > --- > migration/migration.c | 20 +++++++++++++++++++- > migration/migration.h | 1 + > migration/savevm.c | 11 ++++++++++- > qapi/migration.json | 7 +++++-- > 4 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 006447d00a..4d7ef9d8dc 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1257,6 +1257,14 @@ static bool migrate_caps_check(bool *cap_list, > return false; > } > > + if (cap_list[MIGRATION_CAPABILITY_NO_RAM] && > + cap_list[MIGRATION_CAPABILITY_RAM_ONLY]) { > + error_setg(errp, "ram-only and no-ram aren't " > + "compatible with each other"); > + > + return false; > + } > + > return true; > } > > @@ -2619,6 +2627,15 @@ bool migrate_no_ram(void) > return s->enabled_capabilities[MIGRATION_CAPABILITY_NO_RAM]; > } > > +bool migrate_ram_only(void) > +{ > + MigrationState *s; > + > + s = migrate_get_current(); > + > + return s->enabled_capabilities[MIGRATION_CAPABILITY_RAM_ONLY]; > +} > + > /* migration thread support */ > /* > * Something bad happened to the RP stream, mark an error > @@ -3973,7 +3990,8 @@ static void *bg_migration_thread(void *opaque) > * save their state to channel-buffer along with devices. > */ > cpu_synchronize_all_states(); > - if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) { > + if (!migrate_ram_only() && > + qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) { Here... [*] > goto fail; > } > /* > diff --git a/migration/migration.h b/migration/migration.h > index 43f7bf8eba..d5a077e00d 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -359,6 +359,7 @@ bool migrate_use_events(void); > bool migrate_postcopy_blocktime(void); > bool migrate_background_snapshot(void); > bool migrate_no_ram(void); > +bool migrate_ram_only(void); > > /* Sending on the return path - generic and then for each message type */ > void migrate_send_rp_shut(MigrationIncomingState *mis, > diff --git a/migration/savevm.c b/migration/savevm.c > index ba644083ab..e41ca76000 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -249,6 +249,7 @@ typedef struct SaveStateEntry { > void *opaque; > CompatEntry *compat; > int is_iterable; > + bool is_ram; > } SaveStateEntry; > > typedef struct SaveState { > @@ -802,6 +803,10 @@ int register_savevm_live(const char *idstr, > se->is_iterable = 1; > } > > + if (!strcmp("ram", idstr)) { > + se->is_ram = true; > + } > + > pstrcat(se->idstr, sizeof(se->idstr), idstr); > > if (instance_id == VMSTATE_INSTANCE_ID_ANY) { > @@ -949,6 +954,10 @@ static bool should_skip(SaveStateEntry *se) > return true; > } > > + if (migrate_ram_only() && !se->is_ram) { > + return true; > + } > + > return false; > } > > @@ -1486,7 +1495,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only, > } > } > > - if (iterable_only) { > + if (iterable_only || migrate_ram_only()) { > goto flush; > } [*] ... and here you care to avoid calling same qemu_savevm_state_complete_precopy_non_iterable() Seems better to check migrate_ram_only at start of qemu_savevm_state_complete_precopy_non_iterable() and do early return from it. > > diff --git a/qapi/migration.json b/qapi/migration.json > index d53956852c..626fc59d14 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -454,6 +454,8 @@ > # > # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) > # > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) > +# > # Features: > # @unstable: Members @x-colo and @x-ignore-shared are experimental. > # > @@ -467,7 +469,7 @@ > 'block', 'return-path', 'pause-before-switchover', 'multifd', > 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', > { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, > - 'validate-uuid', 'background-snapshot', 'no-ram'] } > + 'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] } > ## > # @MigrationCapabilityStatus: > # > @@ -521,7 +523,8 @@ > # {"state": true, "capability": "events"}, > # {"state": false, "capability": "postcopy-ram"}, > # {"state": false, "capability": "x-colo"}, > -# {"state": false, "capability": "no-ram"} > +# {"state": false, "capability": "no-ram"}, > +# {"state": false, "capability": "ram-only"} > # ]} > # > ## >
Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes: > If this capability is enabled migration stream > will have RAM section only. > > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> [...] > diff --git a/qapi/migration.json b/qapi/migration.json > index d53956852c..626fc59d14 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -454,6 +454,8 @@ > # > # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) > # > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) > +# What happens when I ask for 'no-ram': true, 'ram-only': true? > # Features: > # @unstable: Members @x-colo and @x-ignore-shared are experimental. > # > @@ -467,7 +469,7 @@ > 'block', 'return-path', 'pause-before-switchover', 'multifd', > 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', > { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, > - 'validate-uuid', 'background-snapshot', 'no-ram'] } > + 'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] } > ## > # @MigrationCapabilityStatus: > # > @@ -521,7 +523,8 @@ > # {"state": true, "capability": "events"}, > # {"state": false, "capability": "postcopy-ram"}, > # {"state": false, "capability": "x-colo"}, > -# {"state": false, "capability": "no-ram"} > +# {"state": false, "capability": "no-ram"}, > +# {"state": false, "capability": "ram-only"} > # ]} > # > ##
On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote: > Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes: > > > If this capability is enabled migration stream > > will have RAM section only. > > > > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> > > [...] > > > diff --git a/qapi/migration.json b/qapi/migration.json > > index d53956852c..626fc59d14 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -454,6 +454,8 @@ > > # > > # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) > > # > > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) > > +# > > What happens when I ask for 'no-ram': true, 'ram-only': true? So IIUC no-ram=false, ram-only=false => RAM + vmstate no-ram=true, ram-only=false => vmstate no-ram=false, ram-only=true => RAM no-ram=true, ram-only=true => nothing to send ? I find that the fact that one flag is a negative request and the other flag is a positive request to be confusing. If we must have two flags then could we at least use the same style for both. ie either @no-ram @no-vmstate Or @ram-only @vmstate-only Since the code enforces these flags are mutually exclusive though, it might point towards being handled by a enum { 'enum': 'MigrationStreamContent', 'data': ['both', 'ram', 'vmstate'] } none of these approaches are especially future proof if we ever need fine grained control over sending a sub-set of the non-RAM vmstate. Not sure if that matters in the end. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote: >> Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes: >> >> > If this capability is enabled migration stream >> > will have RAM section only. >> > >> > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> >> >> [...] >> >> > diff --git a/qapi/migration.json b/qapi/migration.json >> > index d53956852c..626fc59d14 100644 >> > --- a/qapi/migration.json >> > +++ b/qapi/migration.json >> > @@ -454,6 +454,8 @@ >> > # >> > # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) >> > # >> > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) >> > +# >> >> What happens when I ask for 'no-ram': true, 'ram-only': true? > > So IIUC > > no-ram=false, ram-only=false => RAM + vmstate > no-ram=true, ram-only=false => vmstate > no-ram=false, ram-only=true => RAM > no-ram=true, ram-only=true => nothing to send ? > > I find that the fact that one flag is a negative request and > the other flag is a positive request to be confusing. Me too. > If we must have two flags then could we at least use the same > style for both. ie either > > @no-ram > @no-vmstate > > Or > > @ram-only > @vmstate-only I strongly prefer "positive" names for booleans, avoiding double negation. > Since the code enforces these flags are mutually exclusive > though, it might point towards being handled by a enum > > { 'enum': 'MigrationStreamContent', > 'data': ['both', 'ram', 'vmstate'] } Enumerating the combinations that are actually valid is often nicer than a set of flags that look independent, but aren't. MigrationCapability can only do flags. For an enum, we'd have to use MigrationParameters & friends. For an example, check out @multifd-compression there. > none of these approaches are especially future proof if we ever > need fine grained control over sending a sub-set of the non-RAM > vmstate. Not sure if that matters in the end. > > > Regards, > Daniel
On 1/14/22 14:22, Markus Armbruster wrote: > Nikita Lapshin<nikita.lapshin@virtuozzo.com> writes: > >> If this capability is enabled migration stream >> will have RAM section only. >> >> Signed-off-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com> > [...] > >> diff --git a/qapi/migration.json b/qapi/migration.json >> index d53956852c..626fc59d14 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -454,6 +454,8 @@ >> # >> # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) >> # >> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) >> +# > What happens when I ask for 'no-ram': true, 'ram-only': true? migrate_caps_check() will return false and print error message that these capabilities are not compatible. > >> # Features: >> # @unstable: Members @x-colo and @x-ignore-shared are experimental. >> # >> @@ -467,7 +469,7 @@ >> 'block', 'return-path', 'pause-before-switchover', 'multifd', >> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', >> { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, >> - 'validate-uuid', 'background-snapshot', 'no-ram'] } >> + 'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] } >> ## >> # @MigrationCapabilityStatus: >> # >> @@ -521,7 +523,8 @@ >> # {"state": true, "capability": "events"}, >> # {"state": false, "capability": "postcopy-ram"}, >> # {"state": false, "capability": "x-colo"}, >> -# {"state": false, "capability": "no-ram"} >> +# {"state": false, "capability": "no-ram"}, >> +# {"state": false, "capability": "ram-only"} >> # ]} >> # >> ##
14.01.2022 18:55, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote: >>> Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes: >>> >>>> If this capability is enabled migration stream >>>> will have RAM section only. >>>> >>>> Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> >>> >>> [...] >>> >>>> diff --git a/qapi/migration.json b/qapi/migration.json >>>> index d53956852c..626fc59d14 100644 >>>> --- a/qapi/migration.json >>>> +++ b/qapi/migration.json >>>> @@ -454,6 +454,8 @@ >>>> # >>>> # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) >>>> # >>>> +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) >>>> +# >>> >>> What happens when I ask for 'no-ram': true, 'ram-only': true? >> >> So IIUC >> >> no-ram=false, ram-only=false => RAM + vmstate >> no-ram=true, ram-only=false => vmstate >> no-ram=false, ram-only=true => RAM >> no-ram=true, ram-only=true => nothing to send ? >> >> I find that the fact that one flag is a negative request and >> the other flag is a positive request to be confusing. > > Me too. > >> If we must have two flags then could we at least use the same >> style for both. ie either >> >> @no-ram >> @no-vmstate >> >> Or >> >> @ram-only >> @vmstate-only > > I strongly prefer "positive" names for booleans, avoiding double > negation. > >> Since the code enforces these flags are mutually exclusive >> though, it might point towards being handled by a enum >> >> { 'enum': 'MigrationStreamContent', >> 'data': ['both', 'ram', 'vmstate'] } > > Enumerating the combinations that are actually valid is often nicer than > a set of flags that look independent, but aren't. > > MigrationCapability can only do flags. For an enum, we'd have to use > MigrationParameters & friends. For an example, check out > @multifd-compression there. > Remember, that we also have block-dirty-bitmaps in migration stream. And we also may have block devices data (should it be deprecated?). So, what about an optional migration parameter stream-content, which would be a *list* of MigrationStreamContent enum values? Than we can deprecate dirty-bitmaps and block capabilities in favor of new migration parameter. { 'enum': 'MigrationStreamContent', 'data': ['block-dirty-bitmpas', 'block', 'ram', 'vmstate'] } ... Migration parameters: ... 'stream-content': ['MigrationStreamContent']
diff --git a/migration/migration.c b/migration/migration.c index 006447d00a..4d7ef9d8dc 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1257,6 +1257,14 @@ static bool migrate_caps_check(bool *cap_list, return false; } + if (cap_list[MIGRATION_CAPABILITY_NO_RAM] && + cap_list[MIGRATION_CAPABILITY_RAM_ONLY]) { + error_setg(errp, "ram-only and no-ram aren't " + "compatible with each other"); + + return false; + } + return true; } @@ -2619,6 +2627,15 @@ bool migrate_no_ram(void) return s->enabled_capabilities[MIGRATION_CAPABILITY_NO_RAM]; } +bool migrate_ram_only(void) +{ + MigrationState *s; + + s = migrate_get_current(); + + return s->enabled_capabilities[MIGRATION_CAPABILITY_RAM_ONLY]; +} + /* migration thread support */ /* * Something bad happened to the RP stream, mark an error @@ -3973,7 +3990,8 @@ static void *bg_migration_thread(void *opaque) * save their state to channel-buffer along with devices. */ cpu_synchronize_all_states(); - if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) { + if (!migrate_ram_only() && + qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) { goto fail; } /* diff --git a/migration/migration.h b/migration/migration.h index 43f7bf8eba..d5a077e00d 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -359,6 +359,7 @@ bool migrate_use_events(void); bool migrate_postcopy_blocktime(void); bool migrate_background_snapshot(void); bool migrate_no_ram(void); +bool migrate_ram_only(void); /* Sending on the return path - generic and then for each message type */ void migrate_send_rp_shut(MigrationIncomingState *mis, diff --git a/migration/savevm.c b/migration/savevm.c index ba644083ab..e41ca76000 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -249,6 +249,7 @@ typedef struct SaveStateEntry { void *opaque; CompatEntry *compat; int is_iterable; + bool is_ram; } SaveStateEntry; typedef struct SaveState { @@ -802,6 +803,10 @@ int register_savevm_live(const char *idstr, se->is_iterable = 1; } + if (!strcmp("ram", idstr)) { + se->is_ram = true; + } + pstrcat(se->idstr, sizeof(se->idstr), idstr); if (instance_id == VMSTATE_INSTANCE_ID_ANY) { @@ -949,6 +954,10 @@ static bool should_skip(SaveStateEntry *se) return true; } + if (migrate_ram_only() && !se->is_ram) { + return true; + } + return false; } @@ -1486,7 +1495,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only, } } - if (iterable_only) { + if (iterable_only || migrate_ram_only()) { goto flush; } diff --git a/qapi/migration.json b/qapi/migration.json index d53956852c..626fc59d14 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -454,6 +454,8 @@ # # @no-ram: If enabled, migration stream won't contain any ram in it. (since 7.0) # +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0) +# # Features: # @unstable: Members @x-colo and @x-ignore-shared are experimental. # @@ -467,7 +469,7 @@ 'block', 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, - 'validate-uuid', 'background-snapshot', 'no-ram'] } + 'validate-uuid', 'background-snapshot', 'no-ram', 'ram-only'] } ## # @MigrationCapabilityStatus: # @@ -521,7 +523,8 @@ # {"state": true, "capability": "events"}, # {"state": false, "capability": "postcopy-ram"}, # {"state": false, "capability": "x-colo"}, -# {"state": false, "capability": "no-ram"} +# {"state": false, "capability": "no-ram"}, +# {"state": false, "capability": "ram-only"} # ]} # ##
If this capability is enabled migration stream will have RAM section only. Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com> --- migration/migration.c | 20 +++++++++++++++++++- migration/migration.h | 1 + migration/savevm.c | 11 ++++++++++- qapi/migration.json | 7 +++++-- 4 files changed, 35 insertions(+), 4 deletions(-)