* [PATCH V1 1/4] migration: mode parameter
2023-10-19 20:47 [PATCH V1 0/4] Live Update reboot mode Steve Sistare
@ 2023-10-19 20:47 ` Steve Sistare
2023-10-20 9:29 ` Juan Quintela
2023-10-20 22:14 ` Steven Sistare
2023-10-19 20:47 ` [PATCH V1 2/4] migration: per-mode blockers Steve Sistare
` (3 subsequent siblings)
4 siblings, 2 replies; 29+ messages in thread
From: Steve Sistare @ 2023-10-19 20:47 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras,
Steve Sistare
Create a mode migration parameter that can be used to select alternate
migration algorithms. The default mode is normal, representing the
current migration algorithm, and does not need to be explicitly set.
No functional change until a new mode is added, except that the mode is
shown by the 'info migrate' command.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
hw/core/qdev-properties-system.c | 12 ++++++++++++
include/hw/qdev-properties-system.h | 4 ++++
include/migration/misc.h | 1 +
migration/migration-hmp-cmds.c | 8 ++++++++
migration/options.c | 21 +++++++++++++++++++++
migration/options.h | 1 +
qapi/migration.json | 27 ++++++++++++++++++++++++---
7 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 6883406..c6fd430 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -673,6 +673,18 @@ const PropertyInfo qdev_prop_multifd_compression = {
.set_default_value = qdev_propinfo_set_default_value_enum,
};
+/* --- MigMode --- */
+
+const PropertyInfo qdev_prop_mig_mode = {
+ .name = "MigMode",
+ .description = "mig_mode values, "
+ "normal/exec",
+ .enum_table = &MigMode_lookup,
+ .get = qdev_propinfo_get_enum,
+ .set = qdev_propinfo_set_enum,
+ .set_default_value = qdev_propinfo_set_default_value_enum,
+};
+
/* --- Reserved Region --- */
/*
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 0ac327a..1418801 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -7,6 +7,7 @@ extern const PropertyInfo qdev_prop_chr;
extern const PropertyInfo qdev_prop_macaddr;
extern const PropertyInfo qdev_prop_reserved_region;
extern const PropertyInfo qdev_prop_multifd_compression;
+extern const PropertyInfo qdev_prop_mig_mode;
extern const PropertyInfo qdev_prop_losttickpolicy;
extern const PropertyInfo qdev_prop_blockdev_on_error;
extern const PropertyInfo qdev_prop_bios_chs_trans;
@@ -41,6 +42,9 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
#define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compression, \
MultiFDCompression)
+#define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
+ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
+ MigMode)
#define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
LostTickPolicy)
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 673ac49..1bc8902 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -15,6 +15,7 @@
#define MIGRATION_MISC_H
#include "qemu/notify.h"
+#include "qapi/qapi-types-migration.h"
#include "qapi/qapi-types-net.h"
/* migration/ram.c */
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a82597f..d8ad429 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -274,6 +274,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%s: %" PRIu64 " ms\n",
MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
params->announce_step);
+ assert(params->has_mode);
+ monitor_printf(mon, "%s: %s\n",
+ MigrationParameter_str(MIGRATION_PARAMETER_MODE),
+ qapi_enum_lookup(&MigMode_lookup, params->mode));
assert(params->has_compress_level);
monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
@@ -514,6 +518,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
}
switch (val) {
+ case MIGRATION_PARAMETER_MODE:
+ p->has_mode = true;
+ visit_type_MigMode(v, param, &p->mode, &err);
+ break;
case MIGRATION_PARAMETER_COMPRESS_LEVEL:
p->has_compress_level = true;
visit_type_uint8(v, param, &p->compress_level, &err);
diff --git a/migration/options.c b/migration/options.c
index 42fb818..4f26515 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -101,6 +101,9 @@ Property migration_properties[] = {
preempt_pre_7_2, false),
/* Migration parameters */
+ DEFINE_PROP_MIG_MODE("mode", MigrationState,
+ parameters.mode,
+ MIG_MODE_NORMAL),
DEFINE_PROP_UINT8("x-compress-level", MigrationState,
parameters.compress_level,
DEFAULT_MIGRATE_COMPRESS_LEVEL),
@@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
return s->parameters.xbzrle_cache_size;
}
+MigMode migrate_mode(void)
+{
+ MigrationState *s = migrate_get_current();
+
+ return s->parameters.mode;
+}
+
/* parameter setters */
void migrate_set_block_incremental(bool value)
@@ -911,6 +921,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
/* TODO use QAPI_CLONE() instead of duplicating it inline */
params = g_malloc0(sizeof(*params));
+ params->has_mode = true;
+ params->mode = s->parameters.mode;
params->has_compress_level = true;
params->compress_level = s->parameters.compress_level;
params->has_compress_threads = true;
@@ -985,6 +997,7 @@ void migrate_params_init(MigrationParameters *params)
params->tls_creds = g_strdup("");
/* Set has_* up only for parameter checks */
+ params->has_mode = true;
params->has_compress_level = true;
params->has_compress_threads = true;
params->has_compress_wait_thread = true;
@@ -1206,6 +1219,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
/* TODO use QAPI_CLONE() instead of duplicating it inline */
+ if (params->has_mode) {
+ dest->mode = params->mode;
+ }
+
if (params->has_compress_level) {
dest->compress_level = params->compress_level;
}
@@ -1315,6 +1332,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
/* TODO use QAPI_CLONE() instead of duplicating it inline */
+ if (params->has_mode) {
+ s->parameters.mode = params->mode;
+ }
+
if (params->has_compress_level) {
s->parameters.compress_level = params->compress_level;
}
diff --git a/migration/options.h b/migration/options.h
index 237f2d6..d9ec873 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -92,6 +92,7 @@ const char *migrate_tls_authz(void);
const char *migrate_tls_creds(void);
const char *migrate_tls_hostname(void);
uint64_t migrate_xbzrle_cache_size(void);
+MigMode migrate_mode(void);
/* parameters setters */
diff --git a/qapi/migration.json b/qapi/migration.json
index db3df12..184fb78 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -616,6 +616,15 @@
{ 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
##
+# @MigMode:
+#
+# @normal: the original form of migration. (since 8.2)
+#
+##
+{ 'enum': 'MigMode',
+ 'data': [ 'normal' ] }
+
+##
# @BitmapMigrationBitmapAliasTransform:
#
# @persistent: If present, the bitmap will be made persistent or
@@ -675,6 +684,9 @@
#
# Migration parameters enumeration
#
+# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
+# (Since 8.2)
+#
# @announce-initial: Initial delay (in milliseconds) before sending
# the first announce (Since 4.0)
#
@@ -841,7 +853,8 @@
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
- 'data': ['announce-initial', 'announce-max',
+ 'data': ['mode',
+ 'announce-initial', 'announce-max',
'announce-rounds', 'announce-step',
'compress-level', 'compress-threads', 'decompress-threads',
'compress-wait-thread', 'throttle-trigger-threshold',
@@ -862,6 +875,9 @@
##
# @MigrateSetParameters:
#
+# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
+# (Since 8.2)
+#
# @announce-initial: Initial delay (in milliseconds) before sending
# the first announce (Since 4.0)
#
@@ -1020,7 +1036,8 @@
# Since: 2.4
##
{ 'struct': 'MigrateSetParameters',
- 'data': { '*announce-initial': 'size',
+ 'data': { '*mode': 'MigMode',
+ '*announce-initial': 'size',
'*announce-max': 'size',
'*announce-rounds': 'size',
'*announce-step': 'size',
@@ -1074,6 +1091,9 @@
#
# The optional members aren't actually optional.
#
+# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
+# (Since 8.2)
+#
# @announce-initial: Initial delay (in milliseconds) before sending
# the first announce (Since 4.0)
#
@@ -1231,7 +1251,8 @@
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
- 'data': { '*announce-initial': 'size',
+ 'data': { '*mode': 'MigMode',
+ '*announce-initial': 'size',
'*announce-max': 'size',
'*announce-rounds': 'size',
'*announce-step': 'size',
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH V1 1/4] migration: mode parameter
2023-10-19 20:47 ` [PATCH V1 1/4] migration: mode parameter Steve Sistare
@ 2023-10-20 9:29 ` Juan Quintela
2023-10-20 14:08 ` Steven Sistare
2023-10-20 22:14 ` Steven Sistare
1 sibling, 1 reply; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 9:29 UTC (permalink / raw)
To: Steve Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steve Sistare <steven.sistare@oracle.com> wrote:
> Create a mode migration parameter that can be used to select alternate
> migration algorithms. The default mode is normal, representing the
> current migration algorithm, and does not need to be explicitly set.
>
> No functional change until a new mode is added, except that the mode is
> shown by the 'info migrate' command.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
[... qdev definition ...]
Looks legit, but I am not an expert here.
> @@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
> return s->parameters.xbzrle_cache_size;
> }
>
> +MigMode migrate_mode(void)
> +{
> + MigrationState *s = migrate_get_current();
> +
> + return s->parameters.mode;
> +}
> +
Inside parameters, I try to get the functions sorted by name. the same
for options.h
> diff --git a/qapi/migration.json b/qapi/migration.json
> index db3df12..184fb78 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -616,6 +616,15 @@
> { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>
> ##
> +# @MigMode:
> +#
> +# @normal: the original form of migration. (since 8.2)
> +#
> +##
> +{ 'enum': 'MigMode',
> + 'data': [ 'normal' ] }
> +
> +##
Here you only have normal, but in qdev you also have exec.
> # @BitmapMigrationBitmapAliasTransform:
> #
> # @persistent: If present, the bitmap will be made persistent or
> @@ -675,6 +684,9 @@
> #
> # Migration parameters enumeration
> #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +# (Since 8.2)
> +#
You normally put comments and values at the end of the comments and
sections. Your sshould be last.
Feel free to use a single line in the json. More than one value for
line make it a bit more compress, but makes changes more complicated.
Rest of the patch feels right.
Thanks, Juan.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 1/4] migration: mode parameter
2023-10-20 9:29 ` Juan Quintela
@ 2023-10-20 14:08 ` Steven Sistare
2023-10-20 19:38 ` Juan Quintela
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-20 14:08 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
On 10/20/2023 5:29 AM, Juan Quintela wrote:
> Steve Sistare <steven.sistare@oracle.com> wrote:
>> Create a mode migration parameter that can be used to select alternate
>> migration algorithms. The default mode is normal, representing the
>> current migration algorithm, and does not need to be explicitly set.
>>
>> No functional change until a new mode is added, except that the mode is
>> shown by the 'info migrate' command.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>
> [... qdev definition ...]
>
> Looks legit, but I am not an expert here.
Nor I, but I copied a similar definition line for line, see
qdev_prop_blockdev_on_error
DEFINE_PROP_BLOCKDEV_ON_ERROR
However, I now see I am missing:
QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));
I can ask Daniel Berrange to review this part if you prefer.
>> @@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
>> return s->parameters.xbzrle_cache_size;
>> }
>>
>> +MigMode migrate_mode(void)
>> +{
>> + MigrationState *s = migrate_get_current();
>> +
>> + return s->parameters.mode;
>> +}
>> +
>
> Inside parameters, I try to get the functions sorted by name. the same
> for options.h
Sure, will do.
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index db3df12..184fb78 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -616,6 +616,15 @@
>> { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>>
>> ##
>> +# @MigMode:
>> +#
>> +# @normal: the original form of migration. (since 8.2)
>> +#
>> +##
>> +{ 'enum': 'MigMode',
>> + 'data': [ 'normal' ] }
>> +
>> +##
>
> Here you only have normal, but in qdev you also have exec.
Good eye. I will remove exec from .description in this patch, and add
cpr-reboot to it in patch 4.
>> # @BitmapMigrationBitmapAliasTransform:
>> #
>> # @persistent: If present, the bitmap will be made persistent or
>> @@ -675,6 +684,9 @@
>> #
>> # Migration parameters enumeration
>> #
>> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>> +# (Since 8.2)
>> +#
>
> You normally put comments and values at the end of the comments and
> sections. Your sshould be last.
Do you mean, add the mode parameter at the end of the existing parameters,
after vcpu-dirty-limit?
> Feel free to use a single line in the json. More than one value for
> line make it a bit more compress, but makes changes more complicated.
Like this?
{ 'enum': 'MigrationParameter',
'data': ['announce-initial', 'announce-max',
...
'vcpu-dirty-limit',
'mode'] }
- Steve
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 1/4] migration: mode parameter
2023-10-20 14:08 ` Steven Sistare
@ 2023-10-20 19:38 ` Juan Quintela
0 siblings, 0 replies; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 19:38 UTC (permalink / raw)
To: Steven Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steven Sistare <steven.sistare@oracle.com> wrote:
> On 10/20/2023 5:29 AM, Juan Quintela wrote:
>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>> Create a mode migration parameter that can be used to select alternate
>>> migration algorithms. The default mode is normal, representing the
>>> current migration algorithm, and does not need to be explicitly set.
>>>
>>> No functional change until a new mode is added, except that the mode is
>>> shown by the 'info migrate' command.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>
>> [... qdev definition ...]
>>
>> Looks legit, but I am not an expert here.
>
> Nor I, but I copied a similar definition line for line, see
> qdev_prop_blockdev_on_error
> DEFINE_PROP_BLOCKDEV_ON_ERROR
>
> However, I now see I am missing:
> QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));
>
> I can ask Daniel Berrange to review this part if you prefer.
I reviewed it, but I agree that looks legit to me O:-)
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index db3df12..184fb78 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -616,6 +616,15 @@
>>> { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>>>
>>> ##
>>> +# @MigMode:
>>> +#
>>> +# @normal: the original form of migration. (since 8.2)
>>> +#
>>> +##
>>> +{ 'enum': 'MigMode',
>>> + 'data': [ 'normal' ] }
>>> +
>>> +##
>>
>> Here you only have normal, but in qdev you also have exec.
>
> Good eye. I will remove exec from .description in this patch, and add
> cpr-reboot to it in patch 4.
Thanks.
>>> # @BitmapMigrationBitmapAliasTransform:
>>> #
>>> # @persistent: If present, the bitmap will be made persistent or
>>> @@ -675,6 +684,9 @@
>>> #
>>> # Migration parameters enumeration
>>> #
>>> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>>> +# (Since 8.2)
>>> +#
>>
>> You normally put comments and values at the end of the comments and
>> sections. Your sshould be last.
>
> Do you mean, add the mode parameter at the end of the existing parameters,
> after vcpu-dirty-limit?
>
>> Feel free to use a single line in the json. More than one value for
>> line make it a bit more compress, but makes changes more complicated.
>
> Like this?
>
> { 'enum': 'MigrationParameter',
> 'data': ['announce-initial', 'announce-max',
> ...
> 'vcpu-dirty-limit',
> 'mode'] }
Exactwly. Same for the comments at the end of the list.
> - Steve
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 1/4] migration: mode parameter
2023-10-19 20:47 ` [PATCH V1 1/4] migration: mode parameter Steve Sistare
2023-10-20 9:29 ` Juan Quintela
@ 2023-10-20 22:14 ` Steven Sistare
1 sibling, 0 replies; 29+ messages in thread
From: Steven Sistare @ 2023-10-20 22:14 UTC (permalink / raw)
To: qemu-devel, Daniel P. Berrange
Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
Hi Daniel, does the addition of MigMode in qdev below look OK to you?
It exactly mirrors qdev_prop_blockdev_on_error + DEFINE_PROP_BLOCKDEV_ON_ERROR.
I realize I need to add:
QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));
and I need to delete "exec" from the .description.
I will cc you when I submit V2 of the patch.
- Steve
On 10/19/2023 4:47 PM, Steve Sistare wrote:
> Create a mode migration parameter that can be used to select alternate
> migration algorithms. The default mode is normal, representing the
> current migration algorithm, and does not need to be explicitly set.
>
> No functional change until a new mode is added, except that the mode is
> shown by the 'info migrate' command.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> hw/core/qdev-properties-system.c | 12 ++++++++++++
> include/hw/qdev-properties-system.h | 4 ++++
> include/migration/misc.h | 1 +
> migration/migration-hmp-cmds.c | 8 ++++++++
> migration/options.c | 21 +++++++++++++++++++++
> migration/options.h | 1 +
> qapi/migration.json | 27 ++++++++++++++++++++++++---
> 7 files changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 6883406..c6fd430 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -673,6 +673,18 @@ const PropertyInfo qdev_prop_multifd_compression = {
> .set_default_value = qdev_propinfo_set_default_value_enum,
> };
>
> +/* --- MigMode --- */
> +
> +const PropertyInfo qdev_prop_mig_mode = {
> + .name = "MigMode",
> + .description = "mig_mode values, "
> + "normal/exec",
> + .enum_table = &MigMode_lookup,
> + .get = qdev_propinfo_get_enum,
> + .set = qdev_propinfo_set_enum,
> + .set_default_value = qdev_propinfo_set_default_value_enum,
> +};
> +
> /* --- Reserved Region --- */
>
> /*
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index 0ac327a..1418801 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -7,6 +7,7 @@ extern const PropertyInfo qdev_prop_chr;
> extern const PropertyInfo qdev_prop_macaddr;
> extern const PropertyInfo qdev_prop_reserved_region;
> extern const PropertyInfo qdev_prop_multifd_compression;
> +extern const PropertyInfo qdev_prop_mig_mode;
> extern const PropertyInfo qdev_prop_losttickpolicy;
> extern const PropertyInfo qdev_prop_blockdev_on_error;
> extern const PropertyInfo qdev_prop_bios_chs_trans;
> @@ -41,6 +42,9 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compression, \
> MultiFDCompression)
> +#define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
> + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
> + MigMode)
> #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
> DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
> LostTickPolicy)
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 673ac49..1bc8902 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -15,6 +15,7 @@
> #define MIGRATION_MISC_H
>
> #include "qemu/notify.h"
> +#include "qapi/qapi-types-migration.h"
> #include "qapi/qapi-types-net.h"
>
> /* migration/ram.c */
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index a82597f..d8ad429 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -274,6 +274,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "%s: %" PRIu64 " ms\n",
> MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
> params->announce_step);
> + assert(params->has_mode);
> + monitor_printf(mon, "%s: %s\n",
> + MigrationParameter_str(MIGRATION_PARAMETER_MODE),
> + qapi_enum_lookup(&MigMode_lookup, params->mode));
> assert(params->has_compress_level);
> monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
> @@ -514,6 +518,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> }
>
> switch (val) {
> + case MIGRATION_PARAMETER_MODE:
> + p->has_mode = true;
> + visit_type_MigMode(v, param, &p->mode, &err);
> + break;
> case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> p->has_compress_level = true;
> visit_type_uint8(v, param, &p->compress_level, &err);
> diff --git a/migration/options.c b/migration/options.c
> index 42fb818..4f26515 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -101,6 +101,9 @@ Property migration_properties[] = {
> preempt_pre_7_2, false),
>
> /* Migration parameters */
> + DEFINE_PROP_MIG_MODE("mode", MigrationState,
> + parameters.mode,
> + MIG_MODE_NORMAL),
> DEFINE_PROP_UINT8("x-compress-level", MigrationState,
> parameters.compress_level,
> DEFAULT_MIGRATE_COMPRESS_LEVEL),
> @@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
> return s->parameters.xbzrle_cache_size;
> }
>
> +MigMode migrate_mode(void)
> +{
> + MigrationState *s = migrate_get_current();
> +
> + return s->parameters.mode;
> +}
> +
> /* parameter setters */
>
> void migrate_set_block_incremental(bool value)
> @@ -911,6 +921,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>
> /* TODO use QAPI_CLONE() instead of duplicating it inline */
> params = g_malloc0(sizeof(*params));
> + params->has_mode = true;
> + params->mode = s->parameters.mode;
> params->has_compress_level = true;
> params->compress_level = s->parameters.compress_level;
> params->has_compress_threads = true;
> @@ -985,6 +997,7 @@ void migrate_params_init(MigrationParameters *params)
> params->tls_creds = g_strdup("");
>
> /* Set has_* up only for parameter checks */
> + params->has_mode = true;
> params->has_compress_level = true;
> params->has_compress_threads = true;
> params->has_compress_wait_thread = true;
> @@ -1206,6 +1219,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>
> /* TODO use QAPI_CLONE() instead of duplicating it inline */
>
> + if (params->has_mode) {
> + dest->mode = params->mode;
> + }
> +
> if (params->has_compress_level) {
> dest->compress_level = params->compress_level;
> }
> @@ -1315,6 +1332,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>
> /* TODO use QAPI_CLONE() instead of duplicating it inline */
>
> + if (params->has_mode) {
> + s->parameters.mode = params->mode;
> + }
> +
> if (params->has_compress_level) {
> s->parameters.compress_level = params->compress_level;
> }
> diff --git a/migration/options.h b/migration/options.h
> index 237f2d6..d9ec873 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -92,6 +92,7 @@ const char *migrate_tls_authz(void);
> const char *migrate_tls_creds(void);
> const char *migrate_tls_hostname(void);
> uint64_t migrate_xbzrle_cache_size(void);
> +MigMode migrate_mode(void);
>
> /* parameters setters */
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index db3df12..184fb78 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -616,6 +616,15 @@
> { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>
> ##
> +# @MigMode:
> +#
> +# @normal: the original form of migration. (since 8.2)
> +#
> +##
> +{ 'enum': 'MigMode',
> + 'data': [ 'normal' ] }
> +
> +##
> # @BitmapMigrationBitmapAliasTransform:
> #
> # @persistent: If present, the bitmap will be made persistent or
> @@ -675,6 +684,9 @@
> #
> # Migration parameters enumeration
> #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +# (Since 8.2)
> +#
> # @announce-initial: Initial delay (in milliseconds) before sending
> # the first announce (Since 4.0)
> #
> @@ -841,7 +853,8 @@
> # Since: 2.4
> ##
> { 'enum': 'MigrationParameter',
> - 'data': ['announce-initial', 'announce-max',
> + 'data': ['mode',
> + 'announce-initial', 'announce-max',
> 'announce-rounds', 'announce-step',
> 'compress-level', 'compress-threads', 'decompress-threads',
> 'compress-wait-thread', 'throttle-trigger-threshold',
> @@ -862,6 +875,9 @@
> ##
> # @MigrateSetParameters:
> #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +# (Since 8.2)
> +#
> # @announce-initial: Initial delay (in milliseconds) before sending
> # the first announce (Since 4.0)
> #
> @@ -1020,7 +1036,8 @@
> # Since: 2.4
> ##
> { 'struct': 'MigrateSetParameters',
> - 'data': { '*announce-initial': 'size',
> + 'data': { '*mode': 'MigMode',
> + '*announce-initial': 'size',
> '*announce-max': 'size',
> '*announce-rounds': 'size',
> '*announce-step': 'size',
> @@ -1074,6 +1091,9 @@
> #
> # The optional members aren't actually optional.
> #
> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
> +# (Since 8.2)
> +#
> # @announce-initial: Initial delay (in milliseconds) before sending
> # the first announce (Since 4.0)
> #
> @@ -1231,7 +1251,8 @@
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> - 'data': { '*announce-initial': 'size',
> + 'data': { '*mode': 'MigMode',
> + '*announce-initial': 'size',
> '*announce-max': 'size',
> '*announce-rounds': 'size',
> '*announce-step': 'size',
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH V1 2/4] migration: per-mode blockers
2023-10-19 20:47 [PATCH V1 0/4] Live Update reboot mode Steve Sistare
2023-10-19 20:47 ` [PATCH V1 1/4] migration: mode parameter Steve Sistare
@ 2023-10-19 20:47 ` Steve Sistare
2023-10-20 9:36 ` Juan Quintela
2023-10-23 12:46 ` Daniel P. Berrangé
2023-10-19 20:47 ` [PATCH V1 3/4] cpr: relax some blockers Steve Sistare
` (2 subsequent siblings)
4 siblings, 2 replies; 29+ messages in thread
From: Steve Sistare @ 2023-10-19 20:47 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras,
Steve Sistare
Extend the blocker interface so that a blocker can be registered for
one or more migration modes. The existing interfaces register a
blocker for all modes, and the new interfaces take a varargs list
of modes.
Internally, maintain a separate blocker list per mode. The same Error
object may be added to multiple lists. When a block is deleted, it is
removed from every list, and the Error is freed.
No functional change until a new mode is added.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
include/migration/blocker.h | 44 +++++++++++++++++++--
migration/migration.c | 95 ++++++++++++++++++++++++++++++++++++++-------
stubs/migr-blocker.c | 10 +++++
3 files changed, 132 insertions(+), 17 deletions(-)
diff --git a/include/migration/blocker.h b/include/migration/blocker.h
index b048f30..a687ac0 100644
--- a/include/migration/blocker.h
+++ b/include/migration/blocker.h
@@ -14,8 +14,12 @@
#ifndef MIGRATION_BLOCKER_H
#define MIGRATION_BLOCKER_H
+#include "qapi/qapi-types-migration.h"
+
+#define MIG_MODE_ALL MIG_MODE__MAX
+
/**
- * @migrate_add_blocker - prevent migration from proceeding
+ * @migrate_add_blocker - prevent all modes of migration from proceeding
*
* @reasonp - address of an error to be returned whenever migration is attempted
*
@@ -30,8 +34,8 @@
int migrate_add_blocker(Error **reasonp, Error **errp);
/**
- * @migrate_add_blocker_internal - prevent migration from proceeding without
- * only-migrate implications
+ * @migrate_add_blocker_internal - prevent all modes of migration from
+ * proceeding, but ignore -only-migratable
*
* @reasonp - address of an error to be returned whenever migration is attempted
*
@@ -50,7 +54,7 @@ int migrate_add_blocker(Error **reasonp, Error **errp);
int migrate_add_blocker_internal(Error **reasonp, Error **errp);
/**
- * @migrate_del_blocker - remove a blocking error from migration and free it.
+ * @migrate_del_blocker - remove a migration blocker from all modes and free it.
*
* @reasonp - address of the error blocking migration
*
@@ -58,4 +62,36 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp);
*/
void migrate_del_blocker(Error **reasonp);
+/**
+ * @migrate_add_blocker_normal - prevent normal migration mode from proceeding
+ *
+ * @reasonp - address of an error to be returned whenever migration is attempted
+ *
+ * @errp - [out] The reason (if any) we cannot block migration right now.
+ *
+ * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
+ *
+ * *@reasonp is freed and set to NULL if failure is returned.
+ * On success, the caller must not free @reasonp, except by
+ * calling migrate_del_blocker.
+ */
+int migrate_add_blocker_normal(Error **reasonp, Error **errp);
+
+/**
+ * @migrate_add_blocker_modes - prevent some modes of migration from proceeding
+ *
+ * @reasonp - address of an error to be returned whenever migration is attempted
+ *
+ * @errp - [out] The reason (if any) we cannot block migration right now.
+ *
+ * @mode - one or more migration modes to be blocked. The list is terminated
+ * by -1 or MIG_MODE_ALL. For the latter, all modes are blocked.
+ *
+ * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
+ *
+ * *@reasonp is freed and set to NULL if failure is returned.
+ * On success, the caller must not free *@reasonp before the blocker is removed.
+ */
+int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...);
+
#endif
diff --git a/migration/migration.c b/migration/migration.c
index 67547eb..b8b54e6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -92,7 +92,7 @@ enum mig_rp_message_type {
static MigrationState *current_migration;
static MigrationIncomingState *current_incoming;
-static GSList *migration_blockers;
+static GSList *migration_blockers[MIG_MODE__MAX];
static bool migration_object_check(MigrationState *ms, Error **errp);
static int migration_maybe_pause(MigrationState *s,
@@ -1011,7 +1011,7 @@ static void fill_source_migration_info(MigrationInfo *info)
{
MigrationState *s = migrate_get_current();
int state = qatomic_read(&s->state);
- GSList *cur_blocker = migration_blockers;
+ GSList *cur_blocker = migration_blockers[migrate_mode()];
info->blocked_reasons = NULL;
@@ -1475,38 +1475,105 @@ int migrate_init(MigrationState *s, Error **errp)
return 0;
}
-int migrate_add_blocker_internal(Error **reasonp, Error **errp)
+static bool is_busy(Error **reasonp, Error **errp)
{
+ ERRP_GUARD();
+
/* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
error_propagate_prepend(errp, *reasonp,
"disallowing migration blocker "
"(migration/snapshot in progress) for: ");
*reasonp = NULL;
- return -EBUSY;
+ return true;
}
-
- migration_blockers = g_slist_prepend(migration_blockers, *reasonp);
- return 0;
+ return false;
}
-int migrate_add_blocker(Error **reasonp, Error **errp)
+static bool is_only_migratable(Error **reasonp, Error **errp, int modes)
{
- if (only_migratable) {
+ ERRP_GUARD();
+
+ if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
error_propagate_prepend(errp, *reasonp,
"disallowing migration blocker "
"(--only-migratable) for: ");
*reasonp = NULL;
+ return true;
+ }
+ return false;
+}
+
+static int get_modes(MigMode mode, va_list ap)
+{
+ int modes = 0;
+
+ while (mode != -1 && mode != MIG_MODE_ALL) {
+ assert(mode >= MIG_MODE_NORMAL && mode < MIG_MODE__MAX);
+ modes |= BIT(mode);
+ mode = va_arg(ap, MigMode);
+ }
+ if (mode == MIG_MODE_ALL) {
+ modes = BIT(MIG_MODE__MAX) - 1;
+ }
+ return modes;
+}
+
+static int add_blockers(Error **reasonp, Error **errp, int modes)
+{
+ for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
+ if (modes & BIT(mode)) {
+ migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
+ *reasonp);
+ }
+ }
+ return 0;
+}
+
+int migrate_add_blocker(Error **reasonp, Error **errp)
+{
+ return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_ALL);
+}
+
+int migrate_add_blocker_normal(Error **reasonp, Error **errp)
+{
+ return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_NORMAL, -1);
+}
+
+int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
+{
+ int modes;
+ va_list ap;
+
+ va_start(ap, mode);
+ modes = get_modes(mode, ap);
+ va_end(ap);
+
+ if (is_only_migratable(reasonp, errp, modes)) {
return -EACCES;
+ } else if (is_busy(reasonp, errp)) {
+ return -EBUSY;
}
+ return add_blockers(reasonp, errp, modes);
+}
- return migrate_add_blocker_internal(reasonp, errp);
+int migrate_add_blocker_internal(Error **reasonp, Error **errp)
+{
+ int modes = BIT(MIG_MODE__MAX) - 1;
+
+ if (is_busy(reasonp, errp)) {
+ return -EBUSY;
+ }
+ return add_blockers(reasonp, errp, modes);
}
void migrate_del_blocker(Error **reasonp)
{
if (*reasonp) {
- migration_blockers = g_slist_remove(migration_blockers, *reasonp);
+ for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
+ migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
+ *reasonp);
+ }
error_free(*reasonp);
*reasonp = NULL;
}
@@ -1602,12 +1669,14 @@ void qmp_migrate_pause(Error **errp)
bool migration_is_blocked(Error **errp)
{
+ GSList *blockers = migration_blockers[migrate_mode()];
+
if (qemu_savevm_state_blocked(errp)) {
return true;
}
- if (migration_blockers) {
- error_propagate(errp, error_copy(migration_blockers->data));
+ if (blockers) {
+ error_propagate(errp, error_copy(blockers->data));
return true;
}
diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
index 17a5dbf..11cbff2 100644
--- a/stubs/migr-blocker.c
+++ b/stubs/migr-blocker.c
@@ -6,6 +6,16 @@ int migrate_add_blocker(Error **reasonp, Error **errp)
return 0;
}
+int migrate_add_blocker_normal(Error **reasonp, Error **errp)
+{
+ return 0;
+}
+
+int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
+{
+ return 0;
+}
+
void migrate_del_blocker(Error **reasonp)
{
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH V1 2/4] migration: per-mode blockers
2023-10-19 20:47 ` [PATCH V1 2/4] migration: per-mode blockers Steve Sistare
@ 2023-10-20 9:36 ` Juan Quintela
2023-10-23 12:46 ` Daniel P. Berrangé
1 sibling, 0 replies; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 9:36 UTC (permalink / raw)
To: Steve Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steve Sistare <steven.sistare@oracle.com> wrote:
> Extend the blocker interface so that a blocker can be registered for
> one or more migration modes. The existing interfaces register a
> blocker for all modes, and the new interfaces take a varargs list
> of modes.
>
> Internally, maintain a separate blocker list per mode. The same Error
> object may be added to multiple lists. When a block is deleted, it is
> removed from every list, and the Error is freed.
>
> No functional change until a new mode is added.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 2/4] migration: per-mode blockers
2023-10-19 20:47 ` [PATCH V1 2/4] migration: per-mode blockers Steve Sistare
2023-10-20 9:36 ` Juan Quintela
@ 2023-10-23 12:46 ` Daniel P. Berrangé
2023-10-23 14:37 ` Steven Sistare
1 sibling, 1 reply; 29+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 12:46 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
On Thu, Oct 19, 2023 at 01:47:44PM -0700, Steve Sistare wrote:
> Extend the blocker interface so that a blocker can be registered for
> one or more migration modes. The existing interfaces register a
> blocker for all modes, and the new interfaces take a varargs list
> of modes.
>
> Internally, maintain a separate blocker list per mode. The same Error
> object may be added to multiple lists. When a block is deleted, it is
> removed from every list, and the Error is freed.
I'm not sure that assocating blockers with migration modes is
the optimal way to model this.
IIUC, some of the migration blockers exist because the feature
relies on state that only exists on the current host.
This isn't a problem with CPR since the migration is within
the same host. At the time though, these blockers should
likely be redundant for a normal migration that uses "localhost".
We can't express the distinction between localhost-migrate
and cross-host-migrate historically, but we should have done.
This new patch largely enables that I think which is good.
What I think this means is that we shouldn't tie blockers
to modes, but rather have different types of blockers as
a bit set
enum MigrationBlockerType {
MIGRATION_BLOCKER_LOCAL_HOST = (1 << 0),
MIGRATION_BLOCKER_CROSS_HOST = (1 << 1),
};
#define MIGRATION_BLOCKER_ALL 0xff
Cpr would check for blockers with MIGRATION_BLOCKER_LOCAL_HOST
set only.
Normal migration within localhost only would similarly only
check MIGRATION_BLOCKER_LOCAL_HOST
Normal migration between arbitrary host would check for
MIGRATION_BLOCKER_LOCAL_HOST and MIGRATION_BLOCKER_CROSS_HOST
>
> No functional change until a new mode is added.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> include/migration/blocker.h | 44 +++++++++++++++++++--
> migration/migration.c | 95 ++++++++++++++++++++++++++++++++++++++-------
> stubs/migr-blocker.c | 10 +++++
> 3 files changed, 132 insertions(+), 17 deletions(-)
>
> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
> index b048f30..a687ac0 100644
> --- a/include/migration/blocker.h
> +++ b/include/migration/blocker.h
> @@ -14,8 +14,12 @@
> #ifndef MIGRATION_BLOCKER_H
> #define MIGRATION_BLOCKER_H
>
> +#include "qapi/qapi-types-migration.h"
> +
> +#define MIG_MODE_ALL MIG_MODE__MAX
> +
> /**
> - * @migrate_add_blocker - prevent migration from proceeding
> + * @migrate_add_blocker - prevent all modes of migration from proceeding
> *
> * @reasonp - address of an error to be returned whenever migration is attempted
> *
> @@ -30,8 +34,8 @@
> int migrate_add_blocker(Error **reasonp, Error **errp);
>
> /**
> - * @migrate_add_blocker_internal - prevent migration from proceeding without
> - * only-migrate implications
> + * @migrate_add_blocker_internal - prevent all modes of migration from
> + * proceeding, but ignore -only-migratable
> *
> * @reasonp - address of an error to be returned whenever migration is attempted
> *
> @@ -50,7 +54,7 @@ int migrate_add_blocker(Error **reasonp, Error **errp);
> int migrate_add_blocker_internal(Error **reasonp, Error **errp);
>
> /**
> - * @migrate_del_blocker - remove a blocking error from migration and free it.
> + * @migrate_del_blocker - remove a migration blocker from all modes and free it.
> *
> * @reasonp - address of the error blocking migration
> *
> @@ -58,4 +62,36 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp);
> */
> void migrate_del_blocker(Error **reasonp);
>
> +/**
> + * @migrate_add_blocker_normal - prevent normal migration mode from proceeding
> + *
> + * @reasonp - address of an error to be returned whenever migration is attempted
> + *
> + * @errp - [out] The reason (if any) we cannot block migration right now.
> + *
> + * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
> + *
> + * *@reasonp is freed and set to NULL if failure is returned.
> + * On success, the caller must not free @reasonp, except by
> + * calling migrate_del_blocker.
> + */
> +int migrate_add_blocker_normal(Error **reasonp, Error **errp);
> +
> +/**
> + * @migrate_add_blocker_modes - prevent some modes of migration from proceeding
> + *
> + * @reasonp - address of an error to be returned whenever migration is attempted
> + *
> + * @errp - [out] The reason (if any) we cannot block migration right now.
> + *
> + * @mode - one or more migration modes to be blocked. The list is terminated
> + * by -1 or MIG_MODE_ALL. For the latter, all modes are blocked.
> + *
> + * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
> + *
> + * *@reasonp is freed and set to NULL if failure is returned.
> + * On success, the caller must not free *@reasonp before the blocker is removed.
> + */
> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...);
> +
> #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 67547eb..b8b54e6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -92,7 +92,7 @@ enum mig_rp_message_type {
> static MigrationState *current_migration;
> static MigrationIncomingState *current_incoming;
>
> -static GSList *migration_blockers;
> +static GSList *migration_blockers[MIG_MODE__MAX];
>
> static bool migration_object_check(MigrationState *ms, Error **errp);
> static int migration_maybe_pause(MigrationState *s,
> @@ -1011,7 +1011,7 @@ static void fill_source_migration_info(MigrationInfo *info)
> {
> MigrationState *s = migrate_get_current();
> int state = qatomic_read(&s->state);
> - GSList *cur_blocker = migration_blockers;
> + GSList *cur_blocker = migration_blockers[migrate_mode()];
>
> info->blocked_reasons = NULL;
>
> @@ -1475,38 +1475,105 @@ int migrate_init(MigrationState *s, Error **errp)
> return 0;
> }
>
> -int migrate_add_blocker_internal(Error **reasonp, Error **errp)
> +static bool is_busy(Error **reasonp, Error **errp)
> {
> + ERRP_GUARD();
> +
> /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
> error_propagate_prepend(errp, *reasonp,
> "disallowing migration blocker "
> "(migration/snapshot in progress) for: ");
> *reasonp = NULL;
> - return -EBUSY;
> + return true;
> }
> -
> - migration_blockers = g_slist_prepend(migration_blockers, *reasonp);
> - return 0;
> + return false;
> }
>
> -int migrate_add_blocker(Error **reasonp, Error **errp)
> +static bool is_only_migratable(Error **reasonp, Error **errp, int modes)
> {
> - if (only_migratable) {
> + ERRP_GUARD();
> +
> + if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
> error_propagate_prepend(errp, *reasonp,
> "disallowing migration blocker "
> "(--only-migratable) for: ");
> *reasonp = NULL;
> + return true;
> + }
> + return false;
> +}
> +
> +static int get_modes(MigMode mode, va_list ap)
> +{
> + int modes = 0;
> +
> + while (mode != -1 && mode != MIG_MODE_ALL) {
> + assert(mode >= MIG_MODE_NORMAL && mode < MIG_MODE__MAX);
> + modes |= BIT(mode);
> + mode = va_arg(ap, MigMode);
> + }
> + if (mode == MIG_MODE_ALL) {
> + modes = BIT(MIG_MODE__MAX) - 1;
> + }
> + return modes;
> +}
> +
> +static int add_blockers(Error **reasonp, Error **errp, int modes)
> +{
> + for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> + if (modes & BIT(mode)) {
> + migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
> + *reasonp);
> + }
> + }
> + return 0;
> +}
> +
> +int migrate_add_blocker(Error **reasonp, Error **errp)
> +{
> + return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_ALL);
> +}
> +
> +int migrate_add_blocker_normal(Error **reasonp, Error **errp)
> +{
> + return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_NORMAL, -1);
> +}
> +
> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
> +{
> + int modes;
> + va_list ap;
> +
> + va_start(ap, mode);
> + modes = get_modes(mode, ap);
> + va_end(ap);
> +
> + if (is_only_migratable(reasonp, errp, modes)) {
> return -EACCES;
> + } else if (is_busy(reasonp, errp)) {
> + return -EBUSY;
> }
> + return add_blockers(reasonp, errp, modes);
> +}
>
> - return migrate_add_blocker_internal(reasonp, errp);
> +int migrate_add_blocker_internal(Error **reasonp, Error **errp)
> +{
> + int modes = BIT(MIG_MODE__MAX) - 1;
> +
> + if (is_busy(reasonp, errp)) {
> + return -EBUSY;
> + }
> + return add_blockers(reasonp, errp, modes);
> }
>
> void migrate_del_blocker(Error **reasonp)
> {
> if (*reasonp) {
> - migration_blockers = g_slist_remove(migration_blockers, *reasonp);
> + for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> + migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
> + *reasonp);
> + }
> error_free(*reasonp);
> *reasonp = NULL;
> }
> @@ -1602,12 +1669,14 @@ void qmp_migrate_pause(Error **errp)
>
> bool migration_is_blocked(Error **errp)
> {
> + GSList *blockers = migration_blockers[migrate_mode()];
> +
> if (qemu_savevm_state_blocked(errp)) {
> return true;
> }
>
> - if (migration_blockers) {
> - error_propagate(errp, error_copy(migration_blockers->data));
> + if (blockers) {
> + error_propagate(errp, error_copy(blockers->data));
> return true;
> }
>
> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
> index 17a5dbf..11cbff2 100644
> --- a/stubs/migr-blocker.c
> +++ b/stubs/migr-blocker.c
> @@ -6,6 +6,16 @@ int migrate_add_blocker(Error **reasonp, Error **errp)
> return 0;
> }
>
> +int migrate_add_blocker_normal(Error **reasonp, Error **errp)
> +{
> + return 0;
> +}
> +
> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
> +{
> + return 0;
> +}
> +
> void migrate_del_blocker(Error **reasonp)
> {
> }
> --
> 1.8.3.1
>
>
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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 2/4] migration: per-mode blockers
2023-10-23 12:46 ` Daniel P. Berrangé
@ 2023-10-23 14:37 ` Steven Sistare
2023-10-23 15:02 ` Daniel P. Berrangé
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 14:37 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
On 10/23/2023 8:46 AM, Daniel P. Berrangé wrote:
> On Thu, Oct 19, 2023 at 01:47:44PM -0700, Steve Sistare wrote:
>> Extend the blocker interface so that a blocker can be registered for
>> one or more migration modes. The existing interfaces register a
>> blocker for all modes, and the new interfaces take a varargs list
>> of modes.
>>
>> Internally, maintain a separate blocker list per mode. The same Error
>> object may be added to multiple lists. When a block is deleted, it is
>> removed from every list, and the Error is freed.
>
> I'm not sure that assocating blockers with migration modes is
> the optimal way to model this.
>
> IIUC, some of the migration blockers exist because the feature
> relies on state that only exists on the current host.
>
> This isn't a problem with CPR since the migration is within
> the same host. At the time though, these blockers should
> likely be redundant for a normal migration that uses "localhost".
>
> We can't express the distinction between localhost-migrate
> and cross-host-migrate historically, but we should have done.
> This new patch largely enables that I think which is good.
>
> What I think this means is that we shouldn't tie blockers
> to modes, but rather have different types of blockers as
> a bit set
>
> enum MigrationBlockerType {
> MIGRATION_BLOCKER_LOCAL_HOST = (1 << 0),
> MIGRATION_BLOCKER_CROSS_HOST = (1 << 1),
> };
>
> #define MIGRATION_BLOCKER_ALL 0xff
>
>
> Cpr would check for blockers with MIGRATION_BLOCKER_LOCAL_HOST
> set only.
>
> Normal migration within localhost only would similarly only
> check MIGRATION_BLOCKER_LOCAL_HOST
>
> Normal migration between arbitrary host would check for
> MIGRATION_BLOCKER_LOCAL_HOST and MIGRATION_BLOCKER_CROSS_HOST
Or, we could define MIG_MODE_LOCAL to relax the blockers for local migrations.
The user would add mode explicitly to the migrate command, or we could
implicitly switch from normal mode to local mode if we infer that the src
and target are the same node. MIG_MODE_LOCAL and MIG_MODE_CPR_REBOOT would
relax the same blockers for now, but conceivably that could change.
When I add cpr-exec mode, it will have its own mode-specific blockers.
But, in your scheme, it could map to a new MigrationBlockerType.
I do prefer mode as the way of specifying the type of migration.
The question is whether we map mode directly to blockers, or map mode
plus other criteria such as locality to MigrationBlockerType(s) which
map to blockers.
One consideration is, how will the user specify the equivalent of only-migratable
on the command line? I was thinking of adding -only-migratable <mode1,mode2,...>
in a future patch, but if additional criteria maps to blockers, then we need
additional options or syntax.
- Steve
>> No functional change until a new mode is added.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> include/migration/blocker.h | 44 +++++++++++++++++++--
>> migration/migration.c | 95 ++++++++++++++++++++++++++++++++++++++-------
>> stubs/migr-blocker.c | 10 +++++
>> 3 files changed, 132 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>> index b048f30..a687ac0 100644
>> --- a/include/migration/blocker.h
>> +++ b/include/migration/blocker.h
>> @@ -14,8 +14,12 @@
>> #ifndef MIGRATION_BLOCKER_H
>> #define MIGRATION_BLOCKER_H
>>
>> +#include "qapi/qapi-types-migration.h"
>> +
>> +#define MIG_MODE_ALL MIG_MODE__MAX
>> +
>> /**
>> - * @migrate_add_blocker - prevent migration from proceeding
>> + * @migrate_add_blocker - prevent all modes of migration from proceeding
>> *
>> * @reasonp - address of an error to be returned whenever migration is attempted
>> *
>> @@ -30,8 +34,8 @@
>> int migrate_add_blocker(Error **reasonp, Error **errp);
>>
>> /**
>> - * @migrate_add_blocker_internal - prevent migration from proceeding without
>> - * only-migrate implications
>> + * @migrate_add_blocker_internal - prevent all modes of migration from
>> + * proceeding, but ignore -only-migratable
>> *
>> * @reasonp - address of an error to be returned whenever migration is attempted
>> *
>> @@ -50,7 +54,7 @@ int migrate_add_blocker(Error **reasonp, Error **errp);
>> int migrate_add_blocker_internal(Error **reasonp, Error **errp);
>>
>> /**
>> - * @migrate_del_blocker - remove a blocking error from migration and free it.
>> + * @migrate_del_blocker - remove a migration blocker from all modes and free it.
>> *
>> * @reasonp - address of the error blocking migration
>> *
>> @@ -58,4 +62,36 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp);
>> */
>> void migrate_del_blocker(Error **reasonp);
>>
>> +/**
>> + * @migrate_add_blocker_normal - prevent normal migration mode from proceeding
>> + *
>> + * @reasonp - address of an error to be returned whenever migration is attempted
>> + *
>> + * @errp - [out] The reason (if any) we cannot block migration right now.
>> + *
>> + * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
>> + *
>> + * *@reasonp is freed and set to NULL if failure is returned.
>> + * On success, the caller must not free @reasonp, except by
>> + * calling migrate_del_blocker.
>> + */
>> +int migrate_add_blocker_normal(Error **reasonp, Error **errp);
>> +
>> +/**
>> + * @migrate_add_blocker_modes - prevent some modes of migration from proceeding
>> + *
>> + * @reasonp - address of an error to be returned whenever migration is attempted
>> + *
>> + * @errp - [out] The reason (if any) we cannot block migration right now.
>> + *
>> + * @mode - one or more migration modes to be blocked. The list is terminated
>> + * by -1 or MIG_MODE_ALL. For the latter, all modes are blocked.
>> + *
>> + * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
>> + *
>> + * *@reasonp is freed and set to NULL if failure is returned.
>> + * On success, the caller must not free *@reasonp before the blocker is removed.
>> + */
>> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...);
>> +
>> #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 67547eb..b8b54e6 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -92,7 +92,7 @@ enum mig_rp_message_type {
>> static MigrationState *current_migration;
>> static MigrationIncomingState *current_incoming;
>>
>> -static GSList *migration_blockers;
>> +static GSList *migration_blockers[MIG_MODE__MAX];
>>
>> static bool migration_object_check(MigrationState *ms, Error **errp);
>> static int migration_maybe_pause(MigrationState *s,
>> @@ -1011,7 +1011,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>> {
>> MigrationState *s = migrate_get_current();
>> int state = qatomic_read(&s->state);
>> - GSList *cur_blocker = migration_blockers;
>> + GSList *cur_blocker = migration_blockers[migrate_mode()];
>>
>> info->blocked_reasons = NULL;
>>
>> @@ -1475,38 +1475,105 @@ int migrate_init(MigrationState *s, Error **errp)
>> return 0;
>> }
>>
>> -int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>> +static bool is_busy(Error **reasonp, Error **errp)
>> {
>> + ERRP_GUARD();
>> +
>> /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
>> if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
>> error_propagate_prepend(errp, *reasonp,
>> "disallowing migration blocker "
>> "(migration/snapshot in progress) for: ");
>> *reasonp = NULL;
>> - return -EBUSY;
>> + return true;
>> }
>> -
>> - migration_blockers = g_slist_prepend(migration_blockers, *reasonp);
>> - return 0;
>> + return false;
>> }
>>
>> -int migrate_add_blocker(Error **reasonp, Error **errp)
>> +static bool is_only_migratable(Error **reasonp, Error **errp, int modes)
>> {
>> - if (only_migratable) {
>> + ERRP_GUARD();
>> +
>> + if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>> error_propagate_prepend(errp, *reasonp,
>> "disallowing migration blocker "
>> "(--only-migratable) for: ");
>> *reasonp = NULL;
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +static int get_modes(MigMode mode, va_list ap)
>> +{
>> + int modes = 0;
>> +
>> + while (mode != -1 && mode != MIG_MODE_ALL) {
>> + assert(mode >= MIG_MODE_NORMAL && mode < MIG_MODE__MAX);
>> + modes |= BIT(mode);
>> + mode = va_arg(ap, MigMode);
>> + }
>> + if (mode == MIG_MODE_ALL) {
>> + modes = BIT(MIG_MODE__MAX) - 1;
>> + }
>> + return modes;
>> +}
>> +
>> +static int add_blockers(Error **reasonp, Error **errp, int modes)
>> +{
>> + for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> + if (modes & BIT(mode)) {
>> + migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>> + *reasonp);
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +int migrate_add_blocker(Error **reasonp, Error **errp)
>> +{
>> + return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_ALL);
>> +}
>> +
>> +int migrate_add_blocker_normal(Error **reasonp, Error **errp)
>> +{
>> + return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_NORMAL, -1);
>> +}
>> +
>> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
>> +{
>> + int modes;
>> + va_list ap;
>> +
>> + va_start(ap, mode);
>> + modes = get_modes(mode, ap);
>> + va_end(ap);
>> +
>> + if (is_only_migratable(reasonp, errp, modes)) {
>> return -EACCES;
>> + } else if (is_busy(reasonp, errp)) {
>> + return -EBUSY;
>> }
>> + return add_blockers(reasonp, errp, modes);
>> +}
>>
>> - return migrate_add_blocker_internal(reasonp, errp);
>> +int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>> +{
>> + int modes = BIT(MIG_MODE__MAX) - 1;
>> +
>> + if (is_busy(reasonp, errp)) {
>> + return -EBUSY;
>> + }
>> + return add_blockers(reasonp, errp, modes);
>> }
>>
>> void migrate_del_blocker(Error **reasonp)
>> {
>> if (*reasonp) {
>> - migration_blockers = g_slist_remove(migration_blockers, *reasonp);
>> + for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> + migration_blockers[mode] = g_slist_remove(migration_blockers[mode],
>> + *reasonp);
>> + }
>> error_free(*reasonp);
>> *reasonp = NULL;
>> }
>> @@ -1602,12 +1669,14 @@ void qmp_migrate_pause(Error **errp)
>>
>> bool migration_is_blocked(Error **errp)
>> {
>> + GSList *blockers = migration_blockers[migrate_mode()];
>> +
>> if (qemu_savevm_state_blocked(errp)) {
>> return true;
>> }
>>
>> - if (migration_blockers) {
>> - error_propagate(errp, error_copy(migration_blockers->data));
>> + if (blockers) {
>> + error_propagate(errp, error_copy(blockers->data));
>> return true;
>> }
>>
>> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
>> index 17a5dbf..11cbff2 100644
>> --- a/stubs/migr-blocker.c
>> +++ b/stubs/migr-blocker.c
>> @@ -6,6 +6,16 @@ int migrate_add_blocker(Error **reasonp, Error **errp)
>> return 0;
>> }
>>
>> +int migrate_add_blocker_normal(Error **reasonp, Error **errp)
>> +{
>> + return 0;
>> +}
>> +
>> +int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
>> +{
>> + return 0;
>> +}
>> +
>> void migrate_del_blocker(Error **reasonp)
>> {
>> }
>> --
>> 1.8.3.1
>>
>>
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 2/4] migration: per-mode blockers
2023-10-23 14:37 ` Steven Sistare
@ 2023-10-23 15:02 ` Daniel P. Berrangé
2023-10-23 18:29 ` Steven Sistare
0 siblings, 1 reply; 29+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 15:02 UTC (permalink / raw)
To: Steven Sistare
Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
On Mon, Oct 23, 2023 at 10:37:59AM -0400, Steven Sistare wrote:
> On 10/23/2023 8:46 AM, Daniel P. Berrangé wrote:
> > On Thu, Oct 19, 2023 at 01:47:44PM -0700, Steve Sistare wrote:
> >> Extend the blocker interface so that a blocker can be registered for
> >> one or more migration modes. The existing interfaces register a
> >> blocker for all modes, and the new interfaces take a varargs list
> >> of modes.
> >>
> >> Internally, maintain a separate blocker list per mode. The same Error
> >> object may be added to multiple lists. When a block is deleted, it is
> >> removed from every list, and the Error is freed.
> >
> > I'm not sure that assocating blockers with migration modes is
> > the optimal way to model this.
> >
> > IIUC, some of the migration blockers exist because the feature
> > relies on state that only exists on the current host.
> >
> > This isn't a problem with CPR since the migration is within
> > the same host. At the time though, these blockers should
> > likely be redundant for a normal migration that uses "localhost".
> >
> > We can't express the distinction between localhost-migrate
> > and cross-host-migrate historically, but we should have done.
> > This new patch largely enables that I think which is good.
> >
> > What I think this means is that we shouldn't tie blockers
> > to modes, but rather have different types of blockers as
> > a bit set
> >
> > enum MigrationBlockerType {
> > MIGRATION_BLOCKER_LOCAL_HOST = (1 << 0),
> > MIGRATION_BLOCKER_CROSS_HOST = (1 << 1),
> > };
> >
> > #define MIGRATION_BLOCKER_ALL 0xff
> >
> >
> > Cpr would check for blockers with MIGRATION_BLOCKER_LOCAL_HOST
> > set only.
> >
> > Normal migration within localhost only would similarly only
> > check MIGRATION_BLOCKER_LOCAL_HOST
> >
> > Normal migration between arbitrary host would check for
> > MIGRATION_BLOCKER_LOCAL_HOST and MIGRATION_BLOCKER_CROSS_HOST
>
> Or, we could define MIG_MODE_LOCAL to relax the blockers for local migrations.
> The user would add mode explicitly to the migrate command, or we could
> implicitly switch from normal mode to local mode if we infer that the src
> and target are the same node. MIG_MODE_LOCAL and MIG_MODE_CPR_REBOOT would
> relax the same blockers for now, but conceivably that could change.
>
> When I add cpr-exec mode, it will have its own mode-specific blockers.
> But, in your scheme, it could map to a new MigrationBlockerType.
Yes, there could be further types of blocker.
Do you have an example of something that would be a CPR blocker
only ?
I was thinking that migration blockers have a functional classification
which motivates their existance.
The different migration modes are describing particular usage
scenarios, and a given usage scenario will imply blockers for
one or more functional reasons.
> I do prefer mode as the way of specifying the type of migration.
Sure, I didn't mean to suggest "mode" as an input to 'migrate'
is bad. Just that I see migration blockers classification as
being distinct from the 'mode'. So a user could specify 'mode'
with 'migrate' and that ends up mapping to certain types of
blocker.
> The question is whether we map mode directly to blockers, or map mode
> plus other criteria such as locality to MigrationBlockerType(s) which
> map to blockers.
>
> One consideration is, how will the user specify the equivalent of only-migratable
> on the command line? I was thinking of adding -only-migratable <mode1,mode2,...>
> in a future patch, but if additional criteria maps to blockers, then we need
> additional options or syntax.
I guess I could see wanting to use --only-migratable to express that I
want a guest that can do a localhost-migration, and CPR, but don't
care about cross-host-migration, which would point towards blocker
types being exposed.
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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 2/4] migration: per-mode blockers
2023-10-23 15:02 ` Daniel P. Berrangé
@ 2023-10-23 18:29 ` Steven Sistare
0 siblings, 0 replies; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 18:29 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
On 10/23/2023 11:02 AM, Daniel P. Berrangé wrote:
> On Mon, Oct 23, 2023 at 10:37:59AM -0400, Steven Sistare wrote:
>> On 10/23/2023 8:46 AM, Daniel P. Berrangé wrote:
>>> On Thu, Oct 19, 2023 at 01:47:44PM -0700, Steve Sistare wrote:
>>>> Extend the blocker interface so that a blocker can be registered for
>>>> one or more migration modes. The existing interfaces register a
>>>> blocker for all modes, and the new interfaces take a varargs list
>>>> of modes.
>>>>
>>>> Internally, maintain a separate blocker list per mode. The same Error
>>>> object may be added to multiple lists. When a block is deleted, it is
>>>> removed from every list, and the Error is freed.
>>>
>>> I'm not sure that assocating blockers with migration modes is
>>> the optimal way to model this.
>>>
>>> IIUC, some of the migration blockers exist because the feature
>>> relies on state that only exists on the current host.
>>>
>>> This isn't a problem with CPR since the migration is within
>>> the same host. At the time though, these blockers should
>>> likely be redundant for a normal migration that uses "localhost".
>>>
>>> We can't express the distinction between localhost-migrate
>>> and cross-host-migrate historically, but we should have done.
>>> This new patch largely enables that I think which is good.
>>>
>>> What I think this means is that we shouldn't tie blockers
>>> to modes, but rather have different types of blockers as
>>> a bit set
>>>
>>> enum MigrationBlockerType {
>>> MIGRATION_BLOCKER_LOCAL_HOST = (1 << 0),
>>> MIGRATION_BLOCKER_CROSS_HOST = (1 << 1),
>>> };
>>>
>>> #define MIGRATION_BLOCKER_ALL 0xff
>>>
>>>
>>> Cpr would check for blockers with MIGRATION_BLOCKER_LOCAL_HOST
>>> set only.
>>>
>>> Normal migration within localhost only would similarly only
>>> check MIGRATION_BLOCKER_LOCAL_HOST
>>>
>>> Normal migration between arbitrary host would check for
>>> MIGRATION_BLOCKER_LOCAL_HOST and MIGRATION_BLOCKER_CROSS_HOST
>>
>> Or, we could define MIG_MODE_LOCAL to relax the blockers for local migrations.
>> The user would add mode explicitly to the migrate command, or we could
>> implicitly switch from normal mode to local mode if we infer that the src
>> and target are the same node. MIG_MODE_LOCAL and MIG_MODE_CPR_REBOOT would
>> relax the same blockers for now, but conceivably that could change.
>>
>> When I add cpr-exec mode, it will have its own mode-specific blockers.
>> But, in your scheme, it could map to a new MigrationBlockerType.
>
> Yes, there could be further types of blocker.
>
> Do you have an example of something that would be a CPR blocker
> only ?
For cpr-exec with vfio, all ram blocks must shared, so the same pinned
pages can be attached after exec. Secondary ram blocks, such as vga ram,
must be created with memfd.
There are misc others. You cannot mix replay and cpr, or colo and cpr.
> I was thinking that migration blockers have a functional classification
> which motivates their existance.
>
> The different migration modes are describing particular usage
> scenarios, and a given usage scenario will imply blockers for
> one or more functional reasons.
A "localhost" blocker reason is less useful and less clear-cut than it first
seemed. The blockdev blockers that I relaxed for reboot mode must still
block normal mode migration to a local host, with concurrent access by the
src and target VM's, because they do not support dirty bitmaps. In fact, I'm
not sure if any of the blockers would be relaxed for a localhost migration.
For cpr, blocks are flushed before qemu exits.
>> I do prefer mode as the way of specifying the type of migration.
>
> Sure, I didn't mean to suggest "mode" as an input to 'migrate'
> is bad. Just that I see migration blockers classification as
> being distinct from the 'mode'. So a user could specify 'mode'
> with 'migrate' and that ends up mapping to certain types of
> blocker.
>
>> The question is whether we map mode directly to blockers, or map mode
>> plus other criteria such as locality to MigrationBlockerType(s) which
>> map to blockers.
>>
>> One consideration is, how will the user specify the equivalent of only-migratable
>> on the command line? I was thinking of adding -only-migratable <mode1,mode2,...>
>> in a future patch, but if additional criteria maps to blockers, then we need
>> additional options or syntax.
>
> I guess I could see wanting to use --only-migratable to express that I
> want a guest that can do a localhost-migration, and CPR, but don't
> care about cross-host-migration, which would point towards blocker
> types being exposed.
Yes, but then users need to understand the additional concept of blocker type,
and know the mapping between mode and blocker type.
I was undecided before, but now I believe that mapping mode to a blocker type
does not add much value, and we should stick to blockers based on mode.
- Steve
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH V1 3/4] cpr: relax some blockers
2023-10-19 20:47 [PATCH V1 0/4] Live Update reboot mode Steve Sistare
2023-10-19 20:47 ` [PATCH V1 1/4] migration: mode parameter Steve Sistare
2023-10-19 20:47 ` [PATCH V1 2/4] migration: per-mode blockers Steve Sistare
@ 2023-10-19 20:47 ` Steve Sistare
2023-10-20 9:38 ` Juan Quintela
2023-10-23 12:36 ` Daniel P. Berrangé
2023-10-19 20:47 ` [PATCH V1 4/4] cpr: reboot mode Steve Sistare
2023-10-19 21:18 ` [PATCH V1 0/4] Live Update " Steven Sistare
4 siblings, 2 replies; 29+ messages in thread
From: Steve Sistare @ 2023-10-19 20:47 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras,
Steve Sistare
Some devices block migration because they rely on local state that
is not migrated to the target host, such as for local filesystems.
These need not block cpr, which will restart qemu on the same host.
Narrow the scope of these blockers so they only apply to normal mode.
They will not block cpr modes when they are added in subsequent patches.
No functional change until a new mode is added.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
backends/tpm/tpm_emulator.c | 2 +-
block/parallels.c | 2 +-
block/qcow.c | 2 +-
block/vdi.c | 2 +-
block/vhdx.c | 2 +-
block/vmdk.c | 2 +-
block/vpc.c | 2 +-
block/vvfat.c | 2 +-
hw/9pfs/9p.c | 2 +-
hw/scsi/vhost-scsi.c | 2 +-
hw/virtio/vhost.c | 2 +-
target/i386/nvmm/nvmm-all.c | 3 ++-
12 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index bf1a90f..ac66aee 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -534,7 +534,7 @@ static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
error_setg(&tpm_emu->migration_blocker,
"Migration disabled: TPM emulator does not support "
"migration");
- if (migrate_add_blocker(&tpm_emu->migration_blocker, &err) < 0) {
+ if (migrate_add_blocker_normal(&tpm_emu->migration_blocker, &err) < 0) {
error_report_err(err);
return -1;
}
diff --git a/block/parallels.c b/block/parallels.c
index 1697a2e..8a520db 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1369,7 +1369,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_get_device_or_node_name(bs));
bdrv_graph_rdunlock_main_loop();
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
error_setg(errp, "Migration blocker error");
goto fail;
diff --git a/block/qcow.c b/block/qcow.c
index fdd4c83..eab68e3 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -307,7 +307,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_get_device_or_node_name(bs));
bdrv_graph_rdunlock_main_loop();
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/vdi.c b/block/vdi.c
index fd7e365..c647d72 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -498,7 +498,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_get_device_or_node_name(bs));
bdrv_graph_rdunlock_main_loop();
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail_free_bmap;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index e37f8c0..a9d0874 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1096,7 +1096,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/vmdk.c b/block/vmdk.c
index 1335d39..85864b8 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1386,7 +1386,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/vpc.c b/block/vpc.c
index c30cf86..aa1a48a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -452,7 +452,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_get_device_or_node_name(bs));
bdrv_graph_rdunlock_main_loop();
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index 266e036..9d050ba 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1268,7 +1268,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
"The vvfat (rw) format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- ret = migrate_add_blocker(&s->migration_blocker, errp);
+ ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
goto fail;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index af636cf..369dfc8 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1501,7 +1501,7 @@ static void coroutine_fn v9fs_attach(void *opaque)
error_setg(&s->migration_blocker,
"Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
- err = migrate_add_blocker(&s->migration_blocker, NULL);
+ err = migrate_add_blocker_normal(&s->migration_blocker, NULL);
if (err < 0) {
clunk_fid(s, fid);
goto out;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 14e23cc..bf528d5 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -208,7 +208,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
"When external environment supports it (Orchestrator migrates "
"target SCSI device state or use shared storage over network), "
"set 'migratable' property to true to enable migration.");
- if (migrate_add_blocker(&vsc->migration_blocker, errp) < 0) {
+ if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
goto free_virtio;
}
}
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d737671..f5e9625 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1527,7 +1527,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
if (hdev->migration_blocker != NULL) {
- r = migrate_add_blocker(&hdev->migration_blocker, errp);
+ r = migrate_add_blocker_normal(&hdev->migration_blocker, errp);
if (r < 0) {
goto fail_busyloop;
}
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index 7d752bc..0cfcdac 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -929,7 +929,8 @@ nvmm_init_vcpu(CPUState *cpu)
error_setg(&nvmm_migration_blocker,
"NVMM: Migration not supported");
- if (migrate_add_blocker(&nvmm_migration_blocker, &local_error) < 0) {
+ ret = migrate_add_blocker_normal(&nvmm_migration_blocker, &local_error);
+ if (ret < 0) {
error_report_err(local_error);
return -EINVAL;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH V1 3/4] cpr: relax some blockers
2023-10-19 20:47 ` [PATCH V1 3/4] cpr: relax some blockers Steve Sistare
@ 2023-10-20 9:38 ` Juan Quintela
2023-10-23 15:25 ` Peter Xu
2023-10-23 12:36 ` Daniel P. Berrangé
1 sibling, 1 reply; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 9:38 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras, Kevin Wolf,
Stefan Hajnoczi
Steve Sistare <steven.sistare@oracle.com> wrote:
> Some devices block migration because they rely on local state that
> is not migrated to the target host, such as for local filesystems.
> These need not block cpr, which will restart qemu on the same host.
> Narrow the scope of these blockers so they only apply to normal mode.
> They will not block cpr modes when they are added in subsequent patches.
>
> No functional change until a new mode is added.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
They are all basically block devices support, would be great to have a
comment from someone from the block layer.
Later, Juan.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 3/4] cpr: relax some blockers
2023-10-20 9:38 ` Juan Quintela
@ 2023-10-23 15:25 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2023-10-23 15:25 UTC (permalink / raw)
To: Juan Quintela
Cc: Steve Sistare, qemu-devel, Fabiano Rosas, Leonardo Bras,
Kevin Wolf, Stefan Hajnoczi
On Fri, Oct 20, 2023 at 11:38:55AM +0200, Juan Quintela wrote:
> Steve Sistare <steven.sistare@oracle.com> wrote:
> > Some devices block migration because they rely on local state that
> > is not migrated to the target host, such as for local filesystems.
> > These need not block cpr, which will restart qemu on the same host.
> > Narrow the scope of these blockers so they only apply to normal mode.
> > They will not block cpr modes when they are added in subsequent patches.
> >
> > No functional change until a new mode is added.
> >
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> They are all basically block devices support, would be great to have a
> comment from someone from the block layer.
There're also non-block ones like vhost/9pfs/..
I agree with Daniel that we should split and allow module maintainers to
review. Maybe we can unify changes of the same module into one patch.
Even if so, some comments for each site on explaining why local migration
can skip the blocker would be greatly helpful.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 3/4] cpr: relax some blockers
2023-10-19 20:47 ` [PATCH V1 3/4] cpr: relax some blockers Steve Sistare
2023-10-20 9:38 ` Juan Quintela
@ 2023-10-23 12:36 ` Daniel P. Berrangé
1 sibling, 0 replies; 29+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 12:36 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
On Thu, Oct 19, 2023 at 01:47:45PM -0700, Steve Sistare wrote:
> Some devices block migration because they rely on local state that
> is not migrated to the target host, such as for local filesystems.
> These need not block cpr, which will restart qemu on the same host.
> Narrow the scope of these blockers so they only apply to normal mode.
> They will not block cpr modes when they are added in subsequent patches.
Looking at these changes, it is not entirely clear to me
why many of these features were blocked for migration
in the first place, and thus it is even less clear why
we should be OK to relax this for cpr.
I'd prefer to see some justification for each file,
explaining why the current blocker exists and why
it is OK to relax it for cpr.
>
> No functional change until a new mode is added.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> backends/tpm/tpm_emulator.c | 2 +-
> block/parallels.c | 2 +-
> block/qcow.c | 2 +-
> block/vdi.c | 2 +-
> block/vhdx.c | 2 +-
> block/vmdk.c | 2 +-
> block/vpc.c | 2 +-
> block/vvfat.c | 2 +-
> hw/9pfs/9p.c | 2 +-
> hw/scsi/vhost-scsi.c | 2 +-
> hw/virtio/vhost.c | 2 +-
> target/i386/nvmm/nvmm-all.c | 3 ++-
> 12 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index bf1a90f..ac66aee 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -534,7 +534,7 @@ static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
> error_setg(&tpm_emu->migration_blocker,
> "Migration disabled: TPM emulator does not support "
> "migration");
> - if (migrate_add_blocker(&tpm_emu->migration_blocker, &err) < 0) {
> + if (migrate_add_blocker_normal(&tpm_emu->migration_blocker, &err) < 0) {
> error_report_err(err);
> return -1;
> }
> diff --git a/block/parallels.c b/block/parallels.c
> index 1697a2e..8a520db 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1369,7 +1369,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> bdrv_get_device_or_node_name(bs));
> bdrv_graph_rdunlock_main_loop();
>
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> error_setg(errp, "Migration blocker error");
> goto fail;
> diff --git a/block/qcow.c b/block/qcow.c
> index fdd4c83..eab68e3 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -307,7 +307,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> bdrv_get_device_or_node_name(bs));
> bdrv_graph_rdunlock_main_loop();
>
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/vdi.c b/block/vdi.c
> index fd7e365..c647d72 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -498,7 +498,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> bdrv_get_device_or_node_name(bs));
> bdrv_graph_rdunlock_main_loop();
>
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail_free_bmap;
> }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index e37f8c0..a9d0874 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1096,7 +1096,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 1335d39..85864b8 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1386,7 +1386,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
> error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/vpc.c b/block/vpc.c
> index c30cf86..aa1a48a 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -452,7 +452,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
> bdrv_get_device_or_node_name(bs));
> bdrv_graph_rdunlock_main_loop();
>
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 266e036..9d050ba 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1268,7 +1268,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> "The vvfat (rw) format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - ret = migrate_add_blocker(&s->migration_blocker, errp);
> + ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
> if (ret < 0) {
> goto fail;
> }
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index af636cf..369dfc8 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1501,7 +1501,7 @@ static void coroutine_fn v9fs_attach(void *opaque)
> error_setg(&s->migration_blocker,
> "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
> s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
> - err = migrate_add_blocker(&s->migration_blocker, NULL);
> + err = migrate_add_blocker_normal(&s->migration_blocker, NULL);
> if (err < 0) {
> clunk_fid(s, fid);
> goto out;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 14e23cc..bf528d5 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -208,7 +208,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> "When external environment supports it (Orchestrator migrates "
> "target SCSI device state or use shared storage over network), "
> "set 'migratable' property to true to enable migration.");
> - if (migrate_add_blocker(&vsc->migration_blocker, errp) < 0) {
> + if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0) {
> goto free_virtio;
> }
> }
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index d737671..f5e9625 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1527,7 +1527,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> }
>
> if (hdev->migration_blocker != NULL) {
> - r = migrate_add_blocker(&hdev->migration_blocker, errp);
> + r = migrate_add_blocker_normal(&hdev->migration_blocker, errp);
> if (r < 0) {
> goto fail_busyloop;
> }
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 7d752bc..0cfcdac 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -929,7 +929,8 @@ nvmm_init_vcpu(CPUState *cpu)
> error_setg(&nvmm_migration_blocker,
> "NVMM: Migration not supported");
>
> - if (migrate_add_blocker(&nvmm_migration_blocker, &local_error) < 0) {
> + ret = migrate_add_blocker_normal(&nvmm_migration_blocker, &local_error);
> + if (ret < 0) {
> error_report_err(local_error);
> return -EINVAL;
> }
> --
> 1.8.3.1
>
>
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 :|
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH V1 4/4] cpr: reboot mode
2023-10-19 20:47 [PATCH V1 0/4] Live Update reboot mode Steve Sistare
` (2 preceding siblings ...)
2023-10-19 20:47 ` [PATCH V1 3/4] cpr: relax some blockers Steve Sistare
@ 2023-10-19 20:47 ` Steve Sistare
2023-10-20 9:45 ` Juan Quintela
2023-10-23 15:39 ` Peter Xu
2023-10-19 21:18 ` [PATCH V1 0/4] Live Update " Steven Sistare
4 siblings, 2 replies; 29+ messages in thread
From: Steve Sistare @ 2023-10-19 20:47 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras,
Steve Sistare
Add the cpr-reboot migration mode. Usage:
$ qemu-system-$arch -monitor stdio ...
QEMU 8.1.50 monitor - type 'help' for more information
(qemu) migrate_set_capability x-ignore-shared on
(qemu) migrate_set_parameter mode cpr-reboot
(qemu) migrate -d file:vm.state
(qemu) info status
VM status: paused (postmigrate)
(qemu) quit
$ qemu-system-$arch -monitor stdio -incoming defer ...
QEMU 8.1.50 monitor - type 'help' for more information
(qemu) migrate_set_capability x-ignore-shared on
(qemu) migrate_set_parameter mode cpr-reboot
(qemu) migrate_incoming file:vm.state
(qemu) info status
VM status: running
In this mode, the migrate command saves state to a file, allowing one
to quit qemu, reboot to an updated kernel, and restart an updated version
of qemu. The caller must specify a migration URI that writes to and reads
from a file. Unlike normal mode, the use of certain local storage options
does not block the migration, but the caller must not modify guest block
devices between the quit and restart. The guest RAM memory-backend must
be shared, and the @x-ignore-shared migration capability must be set,
to avoid saving RAM to the file. Guest RAM must be non-volatile across
reboot, such as by backing it with a dax device, but this is not enforced.
The restarted qemu arguments must match those used to initially start qemu,
plus the -incoming option.
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
qapi/migration.json | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 184fb78..2d862fa 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -620,9 +620,23 @@
#
# @normal: the original form of migration. (since 8.2)
#
+# @cpr-reboot: The migrate command saves state to a file, allowing one to
+# quit qemu, reboot to an updated kernel, and restart an updated
+# version of qemu. The caller must specify a migration URI
+# that writes to and reads from a file. Unlike normal mode,
+# the use of certain local storage options does not block the
+# migration, but the caller must not modify guest block devices
+# between the quit and restart. The guest RAM memory-backend
+# must be shared, and the @x-ignore-shared migration capability
+# must be set, to avoid saving it to the file. Guest RAM must
+# be non-volatile across reboot, such as by backing it with
+# a dax device, but this is not enforced. The restarted qemu
+# arguments must match those used to initially start qemu, plus
+# the -incoming option. (since 8.2)
+#
##
{ 'enum': 'MigMode',
- 'data': [ 'normal' ] }
+ 'data': [ 'normal', 'cpr-reboot' ] }
##
# @BitmapMigrationBitmapAliasTransform:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-19 20:47 ` [PATCH V1 4/4] cpr: reboot mode Steve Sistare
@ 2023-10-20 9:45 ` Juan Quintela
2023-10-20 14:09 ` Steven Sistare
2023-10-23 15:39 ` Peter Xu
1 sibling, 1 reply; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 9:45 UTC (permalink / raw)
To: Steve Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steve Sistare <steven.sistare@oracle.com> wrote:
> Add the cpr-reboot migration mode. Usage:
>
> $ qemu-system-$arch -monitor stdio ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate -d file:vm.state
> (qemu) info status
> VM status: paused (postmigrate)
> (qemu) quit
>
> $ qemu-system-$arch -monitor stdio -incoming defer ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate_incoming file:vm.state
> (qemu) info status
> VM status: running
>
> In this mode, the migrate command saves state to a file, allowing one
> to quit qemu, reboot to an updated kernel, and restart an updated version
> of qemu. The caller must specify a migration URI that writes to and reads
> from a file. Unlike normal mode, the use of certain local storage options
> does not block the migration, but the caller must not modify guest block
> devices between the quit and restart. The guest RAM memory-backend must
> be shared, and the @x-ignore-shared migration capability must be set,
> to avoid saving RAM to the file. Guest RAM must be non-volatile across
> reboot, such as by backing it with a dax device, but this is not enforced.
> The restarted qemu arguments must match those used to initially start qemu,
> plus the -incoming option.
Please, add this message to doc/<somewhere> instead (or additionally) to
the commit log.
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> qapi/migration.json | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 184fb78..2d862fa 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -620,9 +620,23 @@
> #
> # @normal: the original form of migration. (since 8.2)
> #
> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
> +# quit qemu, reboot to an updated kernel, and restart an updated
> +# version of qemu. The caller must specify a migration URI
> +# that writes to and reads from a file. Unlike normal mode,
> +# the use of certain local storage options does not block the
> +# migration, but the caller must not modify guest block devices
> +# between the quit and restart. The guest RAM memory-backend
> +# must be shared, and the @x-ignore-shared migration capability
> +# must be set, to avoid saving it to the file. Guest RAM must
> +# be non-volatile across reboot, such as by backing it with
> +# a dax device, but this is not enforced. The restarted qemu
> +# arguments must match those used to initially start qemu, plus
> +# the -incoming option. (since 8.2)
> +#
> ##
> { 'enum': 'MigMode',
> - 'data': [ 'normal' ] }
> + 'data': [ 'normal', 'cpr-reboot' ] }
>
> ##
> # @BitmapMigrationBitmapAliasTransform:
It only works with file backend, and we don't have any check for that.
Wondering how to add that check.
Additionally, you are not adding a migration test that does exactly what
you put there in the comment.
Thanks, Juan.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-20 9:45 ` Juan Quintela
@ 2023-10-20 14:09 ` Steven Sistare
2023-10-20 19:40 ` Juan Quintela
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-20 14:09 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
On 10/20/2023 5:45 AM, Juan Quintela wrote:
> Steve Sistare <steven.sistare@oracle.com> wrote:
>> Add the cpr-reboot migration mode. Usage:
>>
>> $ qemu-system-$arch -monitor stdio ...
>> QEMU 8.1.50 monitor - type 'help' for more information
>> (qemu) migrate_set_capability x-ignore-shared on
>> (qemu) migrate_set_parameter mode cpr-reboot
>> (qemu) migrate -d file:vm.state
>> (qemu) info status
>> VM status: paused (postmigrate)
>> (qemu) quit
>>
>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>> QEMU 8.1.50 monitor - type 'help' for more information
>> (qemu) migrate_set_capability x-ignore-shared on
>> (qemu) migrate_set_parameter mode cpr-reboot
>> (qemu) migrate_incoming file:vm.state
>> (qemu) info status
>> VM status: running
>>
>> In this mode, the migrate command saves state to a file, allowing one
>> to quit qemu, reboot to an updated kernel, and restart an updated version
>> of qemu. The caller must specify a migration URI that writes to and reads
>> from a file. Unlike normal mode, the use of certain local storage options
>> does not block the migration, but the caller must not modify guest block
>> devices between the quit and restart. The guest RAM memory-backend must
>> be shared, and the @x-ignore-shared migration capability must be set,
>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>> reboot, such as by backing it with a dax device, but this is not enforced.
>> The restarted qemu arguments must match those used to initially start qemu,
>> plus the -incoming option.
>
> Please, add this message to doc/<somewhere> instead (or additionally) to
> the commit log.
>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> qapi/migration.json | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 184fb78..2d862fa 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -620,9 +620,23 @@
>> #
>> # @normal: the original form of migration. (since 8.2)
>> #
>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>> +# quit qemu, reboot to an updated kernel, and restart an updated
>> +# version of qemu. The caller must specify a migration URI
>> +# that writes to and reads from a file. Unlike normal mode,
>> +# the use of certain local storage options does not block the
>> +# migration, but the caller must not modify guest block devices
>> +# between the quit and restart. The guest RAM memory-backend
>> +# must be shared, and the @x-ignore-shared migration capability
>> +# must be set, to avoid saving it to the file. Guest RAM must
>> +# be non-volatile across reboot, such as by backing it with
>> +# a dax device, but this is not enforced. The restarted qemu
>> +# arguments must match those used to initially start qemu, plus
>> +# the -incoming option. (since 8.2)
>> +#
>> ##
>> { 'enum': 'MigMode',
>> - 'data': [ 'normal' ] }
>> + 'data': [ 'normal', 'cpr-reboot' ] }
>>
>> ##
>> # @BitmapMigrationBitmapAliasTransform:
>
> It only works with file backend, and we don't have any check for that.
> Wondering how to add that check.
Actually, it works for other backends, but the ram contents are saved in the
state file, which is slower. I should spell that out in the json comment and
in the commit message.
> Additionally, you are not adding a migration test that does exactly what
> you put there in the comment.
I provide tests/avocado/cpr.py in the original long series. Would you
like me to add it to this series, or post it later? Would you prefer I
add a test to tests/qtest/migration-test.c?
- Steve
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-20 14:09 ` Steven Sistare
@ 2023-10-20 19:40 ` Juan Quintela
2023-10-23 18:39 ` Steven Sistare
0 siblings, 1 reply; 29+ messages in thread
From: Juan Quintela @ 2023-10-20 19:40 UTC (permalink / raw)
To: Steven Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steven Sistare <steven.sistare@oracle.com> wrote:
> On 10/20/2023 5:45 AM, Juan Quintela wrote:
>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>> Add the cpr-reboot migration mode. Usage:
>>>
>>> $ qemu-system-$arch -monitor stdio ...
>>> QEMU 8.1.50 monitor - type 'help' for more information
>>> (qemu) migrate_set_capability x-ignore-shared on
>>> (qemu) migrate_set_parameter mode cpr-reboot
>>> (qemu) migrate -d file:vm.state
>>> (qemu) info status
>>> VM status: paused (postmigrate)
>>> (qemu) quit
>>>
>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>> QEMU 8.1.50 monitor - type 'help' for more information
>>> (qemu) migrate_set_capability x-ignore-shared on
>>> (qemu) migrate_set_parameter mode cpr-reboot
>>> (qemu) migrate_incoming file:vm.state
>>> (qemu) info status
>>> VM status: running
>>>
>>> In this mode, the migrate command saves state to a file, allowing one
>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>> of qemu. The caller must specify a migration URI that writes to and reads
>>> from a file. Unlike normal mode, the use of certain local storage options
>>> does not block the migration, but the caller must not modify guest block
>>> devices between the quit and restart. The guest RAM memory-backend must
>>> be shared, and the @x-ignore-shared migration capability must be set,
>>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>> The restarted qemu arguments must match those used to initially start qemu,
>>> plus the -incoming option.
>>
>> Please, add this message to doc/<somewhere> instead (or additionally) to
>> the commit log.
>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>> qapi/migration.json | 16 +++++++++++++++-
>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 184fb78..2d862fa 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -620,9 +620,23 @@
>>> #
>>> # @normal: the original form of migration. (since 8.2)
>>> #
>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>> +# quit qemu, reboot to an updated kernel, and restart an updated
>>> +# version of qemu. The caller must specify a migration URI
>>> +# that writes to and reads from a file. Unlike normal mode,
>>> +# the use of certain local storage options does not block the
>>> +# migration, but the caller must not modify guest block devices
>>> +# between the quit and restart. The guest RAM memory-backend
>>> +# must be shared, and the @x-ignore-shared migration capability
>>> +# must be set, to avoid saving it to the file. Guest RAM must
>>> +# be non-volatile across reboot, such as by backing it with
>>> +# a dax device, but this is not enforced. The restarted qemu
>>> +# arguments must match those used to initially start qemu, plus
>>> +# the -incoming option. (since 8.2)
>>> +#
>>> ##
>>> { 'enum': 'MigMode',
>>> - 'data': [ 'normal' ] }
>>> + 'data': [ 'normal', 'cpr-reboot' ] }
>>>
>>> ##
>>> # @BitmapMigrationBitmapAliasTransform:
>>
>> It only works with file backend, and we don't have any check for that.
>> Wondering how to add that check.
>
> Actually, it works for other backends, but the ram contents are saved in the
> state file, which is slower. I should spell that out in the json comment and
> in the commit message.
Thanks.
>
>> Additionally, you are not adding a migration test that does exactly what
>> you put there in the comment.
>
> I provide tests/avocado/cpr.py in the original long series. Would you
> like me to add it to this series, or post it later? Would you prefer I
> add a test to tests/qtest/migration-test.c?
test/qtest/migration-test.c
please.
Something simple like what you say in the commit should be a good start.
Thanks, Juan.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-20 19:40 ` Juan Quintela
@ 2023-10-23 18:39 ` Steven Sistare
2023-10-24 11:13 ` Juan Quintela
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 18:39 UTC (permalink / raw)
To: quintela; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
On 10/20/2023 3:40 PM, Juan Quintela wrote:
> Steven Sistare <steven.sistare@oracle.com> wrote:
>> On 10/20/2023 5:45 AM, Juan Quintela wrote:
>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>> Add the cpr-reboot migration mode. Usage:
>>>>
>>>> $ qemu-system-$arch -monitor stdio ...
>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>> (qemu) migrate -d file:vm.state
>>>> (qemu) info status
>>>> VM status: paused (postmigrate)
>>>> (qemu) quit
>>>>
>>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>> (qemu) migrate_incoming file:vm.state
>>>> (qemu) info status
>>>> VM status: running
>>>>
>>>> In this mode, the migrate command saves state to a file, allowing one
>>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>>> of qemu. The caller must specify a migration URI that writes to and reads
>>>> from a file. Unlike normal mode, the use of certain local storage options
>>>> does not block the migration, but the caller must not modify guest block
>>>> devices between the quit and restart. The guest RAM memory-backend must
>>>> be shared, and the @x-ignore-shared migration capability must be set,
>>>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>>> The restarted qemu arguments must match those used to initially start qemu,
>>>> plus the -incoming option.
>>>
>>> Please, add this message to doc/<somewhere> instead (or additionally) to
>>> the commit log.
>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>> qapi/migration.json | 16 +++++++++++++++-
>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 184fb78..2d862fa 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -620,9 +620,23 @@
>>>> #
>>>> # @normal: the original form of migration. (since 8.2)
>>>> #
>>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>>> +# quit qemu, reboot to an updated kernel, and restart an updated
>>>> +# version of qemu. The caller must specify a migration URI
>>>> +# that writes to and reads from a file. Unlike normal mode,
>>>> +# the use of certain local storage options does not block the
>>>> +# migration, but the caller must not modify guest block devices
>>>> +# between the quit and restart. The guest RAM memory-backend
>>>> +# must be shared, and the @x-ignore-shared migration capability
>>>> +# must be set, to avoid saving it to the file. Guest RAM must
>>>> +# be non-volatile across reboot, such as by backing it with
>>>> +# a dax device, but this is not enforced. The restarted qemu
>>>> +# arguments must match those used to initially start qemu, plus
>>>> +# the -incoming option. (since 8.2)
>>>> +#
>>>> ##
>>>> { 'enum': 'MigMode',
>>>> - 'data': [ 'normal' ] }
>>>> + 'data': [ 'normal', 'cpr-reboot' ] }
>>>>
>>>> ##
>>>> # @BitmapMigrationBitmapAliasTransform:
>>>
>>> It only works with file backend, and we don't have any check for that.
>>> Wondering how to add that check.
>>
>> Actually, it works for other backends, but the ram contents are saved in the
>> state file, which is slower. I should spell that out in the json comment and
>> in the commit message.
>
> Thanks.
>>
>>> Additionally, you are not adding a migration test that does exactly what
>>> you put there in the comment.
>>
>> I provide tests/avocado/cpr.py in the original long series. Would you
>> like me to add it to this series, or post it later? Would you prefer I
>> add a test to tests/qtest/migration-test.c?
>
> test/qtest/migration-test.c
>
> please.
>
> Something simple like what you say in the commit should be a good start.
I wrote a new test which I will submit with V2 of this series. Turned out to be
easy after Fabiano provided test_file_common.
+static void *test_mode_reboot_start(QTestState *from, QTestState *to)
+{
+ migrate_set_parameter_str(from, "mode", "cpr-reboot");
+ migrate_set_parameter_str(to, "mode", "cpr-reboot");
+
+ migrate_set_capability(from, "x-ignore-shared", true);
+ migrate_set_capability(to, "x-ignore-shared", true);
+
+ return NULL;
+}
+
+static void test_mode_reboot(void)
+{
+ g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
+ FILE_TEST_FILENAME);
+ MigrateCommon args = {
+ .start.use_shmem = true,
+ .connect_uri = uri,
+ .listen_uri = "defer",
+ .start_hook = test_mode_reboot_start
+ };
+
+ test_file_common(&args, true);
+}
- Steve
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-23 18:39 ` Steven Sistare
@ 2023-10-24 11:13 ` Juan Quintela
0 siblings, 0 replies; 29+ messages in thread
From: Juan Quintela @ 2023-10-24 11:13 UTC (permalink / raw)
To: Steven Sistare; +Cc: qemu-devel, Peter Xu, Fabiano Rosas, Leonardo Bras
Steven Sistare <steven.sistare@oracle.com> wrote:
> On 10/20/2023 3:40 PM, Juan Quintela wrote:
>> Steven Sistare <steven.sistare@oracle.com> wrote:
>>> On 10/20/2023 5:45 AM, Juan Quintela wrote:
>>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>> Add the cpr-reboot migration mode. Usage:
>>>>>
>>>>> $ qemu-system-$arch -monitor stdio ...
>>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>>> (qemu) migrate -d file:vm.state
>>>>> (qemu) info status
>>>>> VM status: paused (postmigrate)
>>>>> (qemu) quit
>>>>>
>>>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>>> (qemu) migrate_incoming file:vm.state
>>>>> (qemu) info status
>>>>> VM status: running
>>>>>
>>>>> In this mode, the migrate command saves state to a file, allowing one
>>>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>>>> of qemu. The caller must specify a migration URI that writes to and reads
>>>>> from a file. Unlike normal mode, the use of certain local storage options
>>>>> does not block the migration, but the caller must not modify guest block
>>>>> devices between the quit and restart. The guest RAM memory-backend must
>>>>> be shared, and the @x-ignore-shared migration capability must be set,
>>>>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>>>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>>>> The restarted qemu arguments must match those used to initially start qemu,
>>>>> plus the -incoming option.
>>>>
>>>> Please, add this message to doc/<somewhere> instead (or additionally) to
>>>> the commit log.
>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> ---
>>>>> qapi/migration.json | 16 +++++++++++++++-
>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index 184fb78..2d862fa 100644
>>>>> --- a/qapi/migration.json
>>>>> +++ b/qapi/migration.json
>>>>> @@ -620,9 +620,23 @@
>>>>> #
>>>>> # @normal: the original form of migration. (since 8.2)
>>>>> #
>>>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>>>> +# quit qemu, reboot to an updated kernel, and restart an updated
>>>>> +# version of qemu. The caller must specify a migration URI
>>>>> +# that writes to and reads from a file. Unlike normal mode,
>>>>> +# the use of certain local storage options does not block the
>>>>> +# migration, but the caller must not modify guest block devices
>>>>> +# between the quit and restart. The guest RAM memory-backend
>>>>> +# must be shared, and the @x-ignore-shared migration capability
>>>>> +# must be set, to avoid saving it to the file. Guest RAM must
>>>>> +# be non-volatile across reboot, such as by backing it with
>>>>> +# a dax device, but this is not enforced. The restarted qemu
>>>>> +# arguments must match those used to initially start qemu, plus
>>>>> +# the -incoming option. (since 8.2)
>>>>> +#
>>>>> ##
>>>>> { 'enum': 'MigMode',
>>>>> - 'data': [ 'normal' ] }
>>>>> + 'data': [ 'normal', 'cpr-reboot' ] }
>>>>>
>>>>> ##
>>>>> # @BitmapMigrationBitmapAliasTransform:
>>>>
>>>> It only works with file backend, and we don't have any check for that.
>>>> Wondering how to add that check.
>>>
>>> Actually, it works for other backends, but the ram contents are saved in the
>>> state file, which is slower. I should spell that out in the json comment and
>>> in the commit message.
>>
>> Thanks.
>>>
>>>> Additionally, you are not adding a migration test that does exactly what
>>>> you put there in the comment.
>>>
>>> I provide tests/avocado/cpr.py in the original long series. Would you
>>> like me to add it to this series, or post it later? Would you prefer I
>>> add a test to tests/qtest/migration-test.c?
>>
>> test/qtest/migration-test.c
>>
>> please.
>>
>> Something simple like what you say in the commit should be a good start.
>
> I wrote a new test which I will submit with V2 of this series. Turned out to be
> easy after Fabiano provided test_file_common.
>
> +static void *test_mode_reboot_start(QTestState *from, QTestState *to)
> +{
> + migrate_set_parameter_str(from, "mode", "cpr-reboot");
> + migrate_set_parameter_str(to, "mode", "cpr-reboot");
> +
> + migrate_set_capability(from, "x-ignore-shared", true);
> + migrate_set_capability(to, "x-ignore-shared", true);
> +
> + return NULL;
> +}
> +
> +static void test_mode_reboot(void)
> +{
> + g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
> + FILE_TEST_FILENAME);
> + MigrateCommon args = {
> + .start.use_shmem = true,
> + .connect_uri = uri,
> + .listen_uri = "defer",
> + .start_hook = test_mode_reboot_start
> + };
> +
> + test_file_common(&args, true);
> +}
>
> - Steve
As a started, that sounds good.
When you add more features, please add tests acordingly.
Thanks again, Juan.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-19 20:47 ` [PATCH V1 4/4] cpr: reboot mode Steve Sistare
2023-10-20 9:45 ` Juan Quintela
@ 2023-10-23 15:39 ` Peter Xu
2023-10-23 18:29 ` Steven Sistare
1 sibling, 1 reply; 29+ messages in thread
From: Peter Xu @ 2023-10-23 15:39 UTC (permalink / raw)
To: Steve Sistare; +Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras
On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
> Add the cpr-reboot migration mode. Usage:
>
> $ qemu-system-$arch -monitor stdio ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate -d file:vm.state
> (qemu) info status
> VM status: paused (postmigrate)
> (qemu) quit
>
> $ qemu-system-$arch -monitor stdio -incoming defer ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate_incoming file:vm.state
> (qemu) info status
> VM status: running
>
> In this mode, the migrate command saves state to a file, allowing one
> to quit qemu, reboot to an updated kernel, and restart an updated version
> of qemu. The caller must specify a migration URI that writes to and reads
> from a file. Unlike normal mode, the use of certain local storage options
> does not block the migration, but the caller must not modify guest block
> devices between the quit and restart. The guest RAM memory-backend must
> be shared, and the @x-ignore-shared migration capability must be set,
> to avoid saving RAM to the file. Guest RAM must be non-volatile across
> reboot, such as by backing it with a dax device, but this is not enforced.
> The restarted qemu arguments must match those used to initially start qemu,
> plus the -incoming option.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> qapi/migration.json | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 184fb78..2d862fa 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -620,9 +620,23 @@
> #
> # @normal: the original form of migration. (since 8.2)
> #
> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
> +# quit qemu, reboot to an updated kernel, and restart an updated
> +# version of qemu. The caller must specify a migration URI
> +# that writes to and reads from a file. Unlike normal mode,
> +# the use of certain local storage options does not block the
> +# migration, but the caller must not modify guest block devices
> +# between the quit and restart. The guest RAM memory-backend
> +# must be shared, and the @x-ignore-shared migration capability
> +# must be set, to avoid saving it to the file. Guest RAM must
> +# be non-volatile across reboot, such as by backing it with
> +# a dax device, but this is not enforced. The restarted qemu
> +# arguments must match those used to initially start qemu, plus
> +# the -incoming option. (since 8.2)
What happens if someone migrates with non-shared memory, or without
ignore-shared? Is it only because it'll be slow saving and loading?
If that's required, we should fail the mode set if (1) non-shared memory is
used, or (2) x-ignore-shared is not enabled. But I had a feeling it's the
other way round.
Reading the whole series, if it's so far all about "local storage", why
"cpr-reboot"? Why not "local" or "local storage" as the name?
I had a feeling that this patchset mixed a lot of higher level use case
into the mode definition. IMHO we should provide clear definition of each
mode on what it does. It's so far not so clear to me, even if I kind of
know what you plan to do.
I tried again google what CPR is for and found this:
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html
I also prefer spell it out, at least make it clear on what that means.. I
didn't even see "Checkpoint/restart" words mentioned anywhere in this
patchset.
Besides: do you have a tree somewhere for the whole set of latest CPR work?
Thanks,
> +#
> ##
> { 'enum': 'MigMode',
> - 'data': [ 'normal' ] }
> + 'data': [ 'normal', 'cpr-reboot' ] }
>
> ##
> # @BitmapMigrationBitmapAliasTransform:
> --
> 1.8.3.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-23 15:39 ` Peter Xu
@ 2023-10-23 18:29 ` Steven Sistare
2023-10-23 18:51 ` Steven Sistare
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 18:29 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras
On 10/23/2023 11:39 AM, Peter Xu wrote:
> On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
>> Add the cpr-reboot migration mode. Usage:
>>
>> $ qemu-system-$arch -monitor stdio ...
>> QEMU 8.1.50 monitor - type 'help' for more information
>> (qemu) migrate_set_capability x-ignore-shared on
>> (qemu) migrate_set_parameter mode cpr-reboot
>> (qemu) migrate -d file:vm.state
>> (qemu) info status
>> VM status: paused (postmigrate)
>> (qemu) quit
>>
>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>> QEMU 8.1.50 monitor - type 'help' for more information
>> (qemu) migrate_set_capability x-ignore-shared on
>> (qemu) migrate_set_parameter mode cpr-reboot
>> (qemu) migrate_incoming file:vm.state
>> (qemu) info status
>> VM status: running
>>
>> In this mode, the migrate command saves state to a file, allowing one
>> to quit qemu, reboot to an updated kernel, and restart an updated version
>> of qemu. The caller must specify a migration URI that writes to and reads
>> from a file. Unlike normal mode, the use of certain local storage options
>> does not block the migration, but the caller must not modify guest block
>> devices between the quit and restart. The guest RAM memory-backend must
>> be shared, and the @x-ignore-shared migration capability must be set,
>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>> reboot, such as by backing it with a dax device, but this is not enforced.
>> The restarted qemu arguments must match those used to initially start qemu,
>> plus the -incoming option.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> qapi/migration.json | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 184fb78..2d862fa 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -620,9 +620,23 @@
>> #
>> # @normal: the original form of migration. (since 8.2)
>> #
>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>> +# quit qemu, reboot to an updated kernel, and restart an updated
>> +# version of qemu. The caller must specify a migration URI
>> +# that writes to and reads from a file. Unlike normal mode,
>> +# the use of certain local storage options does not block the
>> +# migration, but the caller must not modify guest block devices
>> +# between the quit and restart. The guest RAM memory-backend
>> +# must be shared, and the @x-ignore-shared migration capability
>> +# must be set, to avoid saving it to the file. Guest RAM must
>> +# be non-volatile across reboot, such as by backing it with
>> +# a dax device, but this is not enforced. The restarted qemu
>> +# arguments must match those used to initially start qemu, plus
>> +# the -incoming option. (since 8.2)
>
> What happens if someone migrates with non-shared memory, or without
> ignore-shared? Is it only because it'll be slow saving and loading?
>
> If that's required, we should fail the mode set if (1) non-shared memory is
> used, or (2) x-ignore-shared is not enabled. But I had a feeling it's the
> other way round.
Juan also asked me to clarify this. I plan to resubmit this:
# ... Private guest RAM is saved in
# the file. To avoid this cost, the guest RAM memory-backend
# must be shared, and the @x-ignore-shared migration capability
# must be set. ...
>
> Reading the whole series, if it's so far all about "local storage", why
> "cpr-reboot"? Why not "local" or "local storage" as the name?
The use case is about rebooting and updating the host, so reboot is in
the name. Local storage just happens to be allowed for it.
> I had a feeling that this patchset mixed a lot of higher level use case
> into the mode definition. IMHO we should provide clear definition of each
> mode on what it does. It's so far not so clear to me, even if I kind of
> know what you plan to do.
I believe I already have, in the cover letter, commit message, and qapi
definition, at the start of each:
# @cpr-reboot: The migrate command saves state to a file, allowing one to
# quit qemu, reboot to an updated kernel, and restart an updated
# version of qemu.
The cover letter hints at the cpr-exec use case, and the long V9 patch series
describes it, and I will make sure the use case comes first when I submit cpr-exec,
which is:
* much shorter guest downtime than cpr reboot
* support vfio without requiring guest suspension
* keep certain character devices alive
> I tried again google what CPR is for and found this:
>
> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html
>
> I also prefer spell it out, at least make it clear on what that means.. I
> didn't even see "Checkpoint/restart" words mentioned anywhere in this
> patchset.
Will do.
> Besides: do you have a tree somewhere for the whole set of latest CPR work?
I have the V9 patch series:
https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com
and I can re-send my proposal for breaking it down into patch sets that I presented in the
qemu community meeting, if you did not save it.
- Steve
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-23 18:29 ` Steven Sistare
@ 2023-10-23 18:51 ` Steven Sistare
2023-10-23 19:05 ` Peter Xu
0 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 18:51 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras
On 10/23/2023 2:29 PM, Steven Sistare wrote:
> On 10/23/2023 11:39 AM, Peter Xu wrote:
>> On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
>>> Add the cpr-reboot migration mode. Usage:
>>>
>>> $ qemu-system-$arch -monitor stdio ...
>>> QEMU 8.1.50 monitor - type 'help' for more information
>>> (qemu) migrate_set_capability x-ignore-shared on
>>> (qemu) migrate_set_parameter mode cpr-reboot
>>> (qemu) migrate -d file:vm.state
>>> (qemu) info status
>>> VM status: paused (postmigrate)
>>> (qemu) quit
>>>
>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>> QEMU 8.1.50 monitor - type 'help' for more information
>>> (qemu) migrate_set_capability x-ignore-shared on
>>> (qemu) migrate_set_parameter mode cpr-reboot
>>> (qemu) migrate_incoming file:vm.state
>>> (qemu) info status
>>> VM status: running
>>>
>>> In this mode, the migrate command saves state to a file, allowing one
>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>> of qemu. The caller must specify a migration URI that writes to and reads
>>> from a file. Unlike normal mode, the use of certain local storage options
>>> does not block the migration, but the caller must not modify guest block
>>> devices between the quit and restart. The guest RAM memory-backend must
>>> be shared, and the @x-ignore-shared migration capability must be set,
>>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>> The restarted qemu arguments must match those used to initially start qemu,
>>> plus the -incoming option.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>> qapi/migration.json | 16 +++++++++++++++-
>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 184fb78..2d862fa 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -620,9 +620,23 @@
>>> #
>>> # @normal: the original form of migration. (since 8.2)
>>> #
>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>> +# quit qemu, reboot to an updated kernel, and restart an updated
>>> +# version of qemu. The caller must specify a migration URI
>>> +# that writes to and reads from a file. Unlike normal mode,
>>> +# the use of certain local storage options does not block the
>>> +# migration, but the caller must not modify guest block devices
>>> +# between the quit and restart. The guest RAM memory-backend
>>> +# must be shared, and the @x-ignore-shared migration capability
>>> +# must be set, to avoid saving it to the file. Guest RAM must
>>> +# be non-volatile across reboot, such as by backing it with
>>> +# a dax device, but this is not enforced. The restarted qemu
>>> +# arguments must match those used to initially start qemu, plus
>>> +# the -incoming option. (since 8.2)
>>
>> What happens if someone migrates with non-shared memory, or without
>> ignore-shared? Is it only because it'll be slow saving and loading?
>>
>> If that's required, we should fail the mode set if (1) non-shared memory is
>> used, or (2) x-ignore-shared is not enabled. But I had a feeling it's the
>> other way round.
>
> Juan also asked me to clarify this. I plan to resubmit this:
>
> # ... Private guest RAM is saved in
> # the file. To avoid this cost, the guest RAM memory-backend
> # must be shared, and the @x-ignore-shared migration capability
> # must be set. ...
>
>>
>> Reading the whole series, if it's so far all about "local storage", why
>> "cpr-reboot"? Why not "local" or "local storage" as the name?
>
> The use case is about rebooting and updating the host, so reboot is in
> the name. Local storage just happens to be allowed for it.
>
>> I had a feeling that this patchset mixed a lot of higher level use case
>> into the mode definition. IMHO we should provide clear definition of each
>> mode on what it does. It's so far not so clear to me, even if I kind of
>> know what you plan to do.
>
> I believe I already have, in the cover letter, commit message, and qapi
> definition, at the start of each:
>
> # @cpr-reboot: The migrate command saves state to a file, allowing one to
> # quit qemu, reboot to an updated kernel, and restart an updated
> # version of qemu.
>
> The cover letter hints at the cpr-exec use case, and the long V9 patch series
> describes it, and I will make sure the use case comes first when I submit cpr-exec,
> which is:
* restart an updated version of qemu (I buried the lead - steve)
> * much shorter guest downtime than cpr reboot
> * support vfio without requiring guest suspension
> * keep certain character devices alive
>
>> I tried again google what CPR is for and found this:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html
>>
>> I also prefer spell it out, at least make it clear on what that means.. I
>> didn't even see "Checkpoint/restart" words mentioned anywhere in this
>> patchset.
>
> Will do.
>
>> Besides: do you have a tree somewhere for the whole set of latest CPR work?
>
> I have the V9 patch series:
> https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com
> and I can re-send my proposal for breaking it down into patch sets that I presented in the
> qemu community meeting, if you did not save it.
>
> - Steve
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-23 18:51 ` Steven Sistare
@ 2023-10-23 19:05 ` Peter Xu
2023-10-23 20:06 ` Steven Sistare
0 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2023-10-23 19:05 UTC (permalink / raw)
To: Steven Sistare; +Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras
On Mon, Oct 23, 2023 at 02:51:50PM -0400, Steven Sistare wrote:
> On 10/23/2023 2:29 PM, Steven Sistare wrote:
> > On 10/23/2023 11:39 AM, Peter Xu wrote:
> >> On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
> >>> Add the cpr-reboot migration mode. Usage:
> >>>
> >>> $ qemu-system-$arch -monitor stdio ...
> >>> QEMU 8.1.50 monitor - type 'help' for more information
> >>> (qemu) migrate_set_capability x-ignore-shared on
> >>> (qemu) migrate_set_parameter mode cpr-reboot
> >>> (qemu) migrate -d file:vm.state
> >>> (qemu) info status
> >>> VM status: paused (postmigrate)
> >>> (qemu) quit
> >>>
> >>> $ qemu-system-$arch -monitor stdio -incoming defer ...
> >>> QEMU 8.1.50 monitor - type 'help' for more information
> >>> (qemu) migrate_set_capability x-ignore-shared on
> >>> (qemu) migrate_set_parameter mode cpr-reboot
> >>> (qemu) migrate_incoming file:vm.state
> >>> (qemu) info status
> >>> VM status: running
> >>>
> >>> In this mode, the migrate command saves state to a file, allowing one
> >>> to quit qemu, reboot to an updated kernel, and restart an updated version
> >>> of qemu. The caller must specify a migration URI that writes to and reads
> >>> from a file. Unlike normal mode, the use of certain local storage options
> >>> does not block the migration, but the caller must not modify guest block
> >>> devices between the quit and restart. The guest RAM memory-backend must
> >>> be shared, and the @x-ignore-shared migration capability must be set,
> >>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
> >>> reboot, such as by backing it with a dax device, but this is not enforced.
> >>> The restarted qemu arguments must match those used to initially start qemu,
> >>> plus the -incoming option.
> >>>
> >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >>> ---
> >>> qapi/migration.json | 16 +++++++++++++++-
> >>> 1 file changed, 15 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/qapi/migration.json b/qapi/migration.json
> >>> index 184fb78..2d862fa 100644
> >>> --- a/qapi/migration.json
> >>> +++ b/qapi/migration.json
> >>> @@ -620,9 +620,23 @@
> >>> #
> >>> # @normal: the original form of migration. (since 8.2)
> >>> #
> >>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
> >>> +# quit qemu, reboot to an updated kernel, and restart an updated
> >>> +# version of qemu. The caller must specify a migration URI
> >>> +# that writes to and reads from a file. Unlike normal mode,
> >>> +# the use of certain local storage options does not block the
> >>> +# migration, but the caller must not modify guest block devices
> >>> +# between the quit and restart. The guest RAM memory-backend
> >>> +# must be shared, and the @x-ignore-shared migration capability
> >>> +# must be set, to avoid saving it to the file. Guest RAM must
> >>> +# be non-volatile across reboot, such as by backing it with
> >>> +# a dax device, but this is not enforced. The restarted qemu
> >>> +# arguments must match those used to initially start qemu, plus
> >>> +# the -incoming option. (since 8.2)
> >>
> >> What happens if someone migrates with non-shared memory, or without
> >> ignore-shared? Is it only because it'll be slow saving and loading?
> >>
> >> If that's required, we should fail the mode set if (1) non-shared memory is
> >> used, or (2) x-ignore-shared is not enabled. But I had a feeling it's the
> >> other way round.
> >
> > Juan also asked me to clarify this. I plan to resubmit this:
> >
> > # ... Private guest RAM is saved in
> > # the file. To avoid this cost, the guest RAM memory-backend
> > # must be shared, and the @x-ignore-shared migration capability
> > # must be set. ...
Okay. We can also avoid mentioning "private guest RAM is saved to ..."
because that's what migration already does. IMO we can simplify all that
to:
It is suggested to use share memory with x-ignore-shared when using this
mode.
> >
> >>
> >> Reading the whole series, if it's so far all about "local storage", why
> >> "cpr-reboot"? Why not "local" or "local storage" as the name?
> >
> > The use case is about rebooting and updating the host, so reboot is in
> > the name. Local storage just happens to be allowed for it.
> >
> >> I had a feeling that this patchset mixed a lot of higher level use case
> >> into the mode definition. IMHO we should provide clear definition of each
> >> mode on what it does. It's so far not so clear to me, even if I kind of
> >> know what you plan to do.
> >
> > I believe I already have, in the cover letter, commit message, and qapi
> > definition, at the start of each:
> >
> > # @cpr-reboot: The migrate command saves state to a file, allowing one to
> > # quit qemu, reboot to an updated kernel, and restart an updated
> > # version of qemu.
I think this is why I'm confused: above sentence is describing a very
generic migration to file scenario to me. IOW, I think I can get the same
result described even with normal migration to file, or am I wrong?
IMHO the description here needs to explain the difference and when an user
should use this mode. I think the real answer resides in your whole set,
I'll try to read that.
In all cases, can we name it something like "live-upgrade" v.s. "normal"?
> >
> > The cover letter hints at the cpr-exec use case, and the long V9 patch series
> > describes it, and I will make sure the use case comes first when I submit cpr-exec,
> > which is:
> * restart an updated version of qemu (I buried the lead - steve)
> > * much shorter guest downtime than cpr reboot
> > * support vfio without requiring guest suspension
> > * keep certain character devices alive
> >
> >> I tried again google what CPR is for and found this:
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html
> >>
> >> I also prefer spell it out, at least make it clear on what that means.. I
> >> didn't even see "Checkpoint/restart" words mentioned anywhere in this
> >> patchset.
> >
> > Will do.
> >
> >> Besides: do you have a tree somewhere for the whole set of latest CPR work?
> >
> > I have the V9 patch series:
> > https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com
> > and I can re-send my proposal for breaking it down into patch sets that I presented in the
> > qemu community meeting, if you did not save it.
No need to resend. A link is exactly what I need; git tree even better.
I'll comment when I get something when reading that.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 4/4] cpr: reboot mode
2023-10-23 19:05 ` Peter Xu
@ 2023-10-23 20:06 ` Steven Sistare
0 siblings, 0 replies; 29+ messages in thread
From: Steven Sistare @ 2023-10-23 20:06 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras
On 10/23/2023 3:05 PM, Peter Xu wrote:
> On Mon, Oct 23, 2023 at 02:51:50PM -0400, Steven Sistare wrote:
>> On 10/23/2023 2:29 PM, Steven Sistare wrote:
>>> On 10/23/2023 11:39 AM, Peter Xu wrote:
>>>> On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
>>>>> Add the cpr-reboot migration mode. Usage:
>>>>>
>>>>> $ qemu-system-$arch -monitor stdio ...
>>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>>> (qemu) migrate -d file:vm.state
>>>>> (qemu) info status
>>>>> VM status: paused (postmigrate)
>>>>> (qemu) quit
>>>>>
>>>>> $ qemu-system-$arch -monitor stdio -incoming defer ...
>>>>> QEMU 8.1.50 monitor - type 'help' for more information
>>>>> (qemu) migrate_set_capability x-ignore-shared on
>>>>> (qemu) migrate_set_parameter mode cpr-reboot
>>>>> (qemu) migrate_incoming file:vm.state
>>>>> (qemu) info status
>>>>> VM status: running
>>>>>
>>>>> In this mode, the migrate command saves state to a file, allowing one
>>>>> to quit qemu, reboot to an updated kernel, and restart an updated version
>>>>> of qemu. The caller must specify a migration URI that writes to and reads
>>>>> from a file. Unlike normal mode, the use of certain local storage options
>>>>> does not block the migration, but the caller must not modify guest block
>>>>> devices between the quit and restart. The guest RAM memory-backend must
>>>>> be shared, and the @x-ignore-shared migration capability must be set,
>>>>> to avoid saving RAM to the file. Guest RAM must be non-volatile across
>>>>> reboot, such as by backing it with a dax device, but this is not enforced.
>>>>> The restarted qemu arguments must match those used to initially start qemu,
>>>>> plus the -incoming option.
>>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> ---
>>>>> qapi/migration.json | 16 +++++++++++++++-
>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index 184fb78..2d862fa 100644
>>>>> --- a/qapi/migration.json
>>>>> +++ b/qapi/migration.json
>>>>> @@ -620,9 +620,23 @@
>>>>> #
>>>>> # @normal: the original form of migration. (since 8.2)
>>>>> #
>>>>> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
>>>>> +# quit qemu, reboot to an updated kernel, and restart an updated
>>>>> +# version of qemu. The caller must specify a migration URI
>>>>> +# that writes to and reads from a file. Unlike normal mode,
>>>>> +# the use of certain local storage options does not block the
>>>>> +# migration, but the caller must not modify guest block devices
>>>>> +# between the quit and restart. The guest RAM memory-backend
>>>>> +# must be shared, and the @x-ignore-shared migration capability
>>>>> +# must be set, to avoid saving it to the file. Guest RAM must
>>>>> +# be non-volatile across reboot, such as by backing it with
>>>>> +# a dax device, but this is not enforced. The restarted qemu
>>>>> +# arguments must match those used to initially start qemu, plus
>>>>> +# the -incoming option. (since 8.2)
>>>>
>>>> What happens if someone migrates with non-shared memory, or without
>>>> ignore-shared? Is it only because it'll be slow saving and loading?
>>>>
>>>> If that's required, we should fail the mode set if (1) non-shared memory is
>>>> used, or (2) x-ignore-shared is not enabled. But I had a feeling it's the
>>>> other way round.
>>>
>>> Juan also asked me to clarify this. I plan to resubmit this:
>>>
>>> # ... Private guest RAM is saved in
>>> # the file. To avoid this cost, the guest RAM memory-backend
>>> # must be shared, and the @x-ignore-shared migration capability
>>> # must be set. ...
>
> Okay. We can also avoid mentioning "private guest RAM is saved to ..."
> because that's what migration already does. IMO we can simplify all that
> to:
>
> It is suggested to use share memory with x-ignore-shared when using this
> mode
OK, I'll massage it some more. I think we should explicitly warn about the
cost of saving all memory.
>>>> Reading the whole series, if it's so far all about "local storage", why
>>>> "cpr-reboot"? Why not "local" or "local storage" as the name?
>>>
>>> The use case is about rebooting and updating the host, so reboot is in
>>> the name. Local storage just happens to be allowed for it.
>>>
>>>> I had a feeling that this patchset mixed a lot of higher level use case
>>>> into the mode definition. IMHO we should provide clear definition of each
>>>> mode on what it does. It's so far not so clear to me, even if I kind of
>>>> know what you plan to do.
>>>
>>> I believe I already have, in the cover letter, commit message, and qapi
>>> definition, at the start of each:
>>>
>>> # @cpr-reboot: The migrate command saves state to a file, allowing one to
>>> # quit qemu, reboot to an updated kernel, and restart an updated
>>> # version of qemu.
>
> I think this is why I'm confused: above sentence is describing a very
> generic migration to file scenario to me. IOW, I think I can get the same
> result described even with normal migration to file, or am I wrong?
cpr-reboot has fewer blockers than normal migration to a file URI. Most
importantly, the presence of vfio devices will not block it as long as
the guest is suspended. That functionality is implemented in the patch
"vfio: allow cpr-reboot migration if suspended" in the V9 series.
I suppose we could use the presence of a "file URI" as the criteria for relaxing
blockers, and eliminate cpr-reboot mode. However, by making the mode explicit,
we can add mode-based options such as '-only-migratable <mode>'.
If we decide to delete the explicit reboot mode, I still need to add MigMode and
per-mode blockers when I submit cpr-exec mode.
> IMHO the description here needs to explain the difference and when an user
> should use this mode. I think the real answer resides in your whole set,
> I'll try to read that.
>
> In all cases, can we name it something like "live-upgrade" v.s. "normal"?
I like cpr because it is a short and unique identifier for functions, types,
variables, and user-visible tokens. It reduces line wrapping and makes the code
more readable, IMO.
- Steve
>>> The cover letter hints at the cpr-exec use case, and the long V9 patch series
>>> describes it, and I will make sure the use case comes first when I submit cpr-exec,
>>> which is:
>> * restart an updated version of qemu (I buried the lead - steve)
>>> * much shorter guest downtime than cpr reboot
>>> * support vfio without requiring guest suspension
>>> * keep certain character devices alive
>>>
>>>> I tried again google what CPR is for and found this:
>>>>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html
>>>>
>>>> I also prefer spell it out, at least make it clear on what that means.. I
>>>> didn't even see "Checkpoint/restart" words mentioned anywhere in this
>>>> patchset.
>>>
>>> Will do.
>>>
>>>> Besides: do you have a tree somewhere for the whole set of latest CPR work?
>>>
>>> I have the V9 patch series:
>>> https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com
>>> and I can re-send my proposal for breaking it down into patch sets that I presented in the
>>> qemu community meeting, if you did not save it.
>
> No need to resend. A link is exactly what I need; git tree even better.
> I'll comment when I get something when reading that.
>
> Thanks,
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH V1 0/4] Live Update reboot mode
2023-10-19 20:47 [PATCH V1 0/4] Live Update reboot mode Steve Sistare
` (3 preceding siblings ...)
2023-10-19 20:47 ` [PATCH V1 4/4] cpr: reboot mode Steve Sistare
@ 2023-10-19 21:18 ` Steven Sistare
2023-10-20 9:23 ` Juan Quintela
4 siblings, 1 reply; 29+ messages in thread
From: Steven Sistare @ 2023-10-19 21:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras
BTW, this series depends on the patch "migration: simplify blockers".
- Steve
On 10/19/2023 4:47 PM, Steve Sistare wrote:
> Add a mode migration parameter that can be used to select alternate
> migration algorithms. The default mode is normal, representing the
> current migration algorithm, and does not need to be explicitly set.
>
> Provide the cpr-reboot migration mode for live update, which saves state to
> a file. This allows one to quit qemu, reboot to an updated kernel, install
> an updated version of qemu, and resume via the migrate-incoming command.
> The caller must specify a migration URI that writes to and reads from a file,
> and must set the mode parameter before invoking the migrate or migrate-incoming
> commands.
>
> Unlike normal mode, the use of certain local storage options does not block
> cpr-reboot mode, but the caller must not modify guest block devices between
> the quit and restart. The guest RAM memory-backend must be shared, and the
> @x-ignore-shared migration capability must be set, to avoid saving RAM to the
> file. Guest RAM must be non-volatile across reboot, which can be achieved by
> backing it with a dax device, or /dev/shm PKRAM as proposed in
> https://lore.kernel.org/lkml/1617140178-8773-1-git-send-email-anthony.yznaga@oracle.com
> but this is not enforced. The restarted qemu arguments must match those used
> to initially start qemu, plus the -incoming option.
>
> This patch series contains minimal functionality. Future patches will enhance
> reboot mode by preserving vfio devices for suspended guests. They will also
> add a new mode for updating qemu using the exec system call, which will keep
> vfio devices and certain character devices alive.
>
> Here is an example of updating the host kernel using reboot mode.
>
> window 1 | window 2
> |
> # qemu-system-$arch -monitor stdio |
> mem-path=/dev/dax0.0 ... |
> QEMU 8.1.50 monitor - type 'help' for more info |
> (qemu) info status |
> VM status: running |
> | # yum update kernel-uek
> (qemu) migrate_set_capability x-ignore-shared on|
> (qemu) migrate_set_parameter mode cpr-reboot |
> (qemu) migrate -d file:vm.state |
> (qemu) info status |
> VM status: paused (postmigrate) |
> (qemu) quit |
> |
> # systemctl kexec |
> kexec_core: Starting new kernel |
> ... |
> |
> # qemu-system-$arch -monitor stdio |
> mem-path=/dev/dax0.0 -incoming defer ... |
> QEMU 8.1.50 monitor - type 'help' for more info |
> (qemu) info status |
> VM status: paused (inmigrate) |
> (qemu) migrate_set_capability x-ignore-shared on|
> (qemu) migrate_set_parameter mode cpr-reboot |
> (qemu) migrate_incoming file:vm.state |
> (qemu) info status |
> VM status: running |
>
> Steve Sistare (4):
> migration: mode parameter
> migration: per-mode blockers
> cpr: relax some blockers
> cpr: reboot mode
>
> backends/tpm/tpm_emulator.c | 2 +-
> block/parallels.c | 2 +-
> block/qcow.c | 2 +-
> block/vdi.c | 2 +-
> block/vhdx.c | 2 +-
> block/vmdk.c | 2 +-
> block/vpc.c | 2 +-
> block/vvfat.c | 2 +-
> hw/9pfs/9p.c | 2 +-
> hw/core/qdev-properties-system.c | 12 +++++
> hw/scsi/vhost-scsi.c | 2 +-
> hw/virtio/vhost.c | 2 +-
> include/hw/qdev-properties-system.h | 4 ++
> include/migration/blocker.h | 44 +++++++++++++++--
> include/migration/misc.h | 1 +
> migration/migration-hmp-cmds.c | 8 ++++
> migration/migration.c | 95 ++++++++++++++++++++++++++++++++-----
> migration/options.c | 21 ++++++++
> migration/options.h | 1 +
> qapi/migration.json | 41 ++++++++++++++--
> stubs/migr-blocker.c | 10 ++++
> target/i386/nvmm/nvmm-all.c | 3 +-
> 22 files changed, 230 insertions(+), 32 deletions(-)
>
^ permalink raw reply [flat|nested] 29+ messages in thread