We don't allow to use x-colo capability when replication is not
configured. So, no reason to build COLO when replication is disabled,
it's unusable in this case.
Note also that the check in migrate_caps_check() is not the only
restriction: some functions in migration/colo.c will just abort if
called with not defined CONFIG_REPLICATION, for example:
migration_iteration_finish()
case MIGRATION_STATUS_COLO:
migrate_start_colo_process()
colo_process_checkpoint()
abort()
It could probably make sense to have possibility to enable COLO without
REPLICATION, but this requires deeper audit of colo & replication code,
which may be done later if needed.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
hmp-commands.hx | 2 ++
migration/colo.c | 28 -------------------------
migration/meson.build | 6 ++++--
migration/migration-hmp-cmds.c | 2 ++
migration/options.c | 17 ++++++++--------
qapi/migration.json | 12 +++++++----
stubs/colo.c | 37 ++++++++++++++++++++++++++++++++++
stubs/meson.build | 1 +
8 files changed, 62 insertions(+), 43 deletions(-)
create mode 100644 stubs/colo.c
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb85ee1d26..fbd0932232 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1035,6 +1035,7 @@ SRST
migration (or once already in postcopy).
ERST
+#ifdef CONFIG_REPLICATION
{
.name = "x_colo_lost_heartbeat",
.args_type = "",
@@ -1043,6 +1044,7 @@ ERST
"a failover or takeover is needed.",
.cmd = hmp_x_colo_lost_heartbeat,
},
+#endif
SRST
``x_colo_lost_heartbeat``
diff --git a/migration/colo.c b/migration/colo.c
index 07bfa21fea..e4af47eeeb 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -26,9 +26,7 @@
#include "qemu/rcu.h"
#include "migration/failover.h"
#include "migration/ram.h"
-#ifdef CONFIG_REPLICATION
#include "block/replication.h"
-#endif
#include "net/colo-compare.h"
#include "net/colo.h"
#include "block/block.h"
@@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
static void secondary_vm_do_failover(void)
{
/* COLO needs enable block-replication */
-#ifdef CONFIG_REPLICATION
int old_state;
MigrationIncomingState *mis = migration_incoming_get_current();
Error *local_err = NULL;
@@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
if (mis->migration_incoming_co) {
qemu_coroutine_enter(mis->migration_incoming_co);
}
-#else
- abort();
-#endif
}
static void primary_vm_do_failover(void)
{
-#ifdef CONFIG_REPLICATION
MigrationState *s = migrate_get_current();
int old_state;
Error *local_err = NULL;
@@ -181,9 +174,6 @@ static void primary_vm_do_failover(void)
/* Notify COLO thread that failover work is finished */
qemu_sem_post(&s->colo_exit_sem);
-#else
- abort();
-#endif
}
COLOMode get_colo_mode(void)
@@ -217,7 +207,6 @@ void colo_do_failover(void)
}
}
-#ifdef CONFIG_REPLICATION
void qmp_xen_set_replication(bool enable, bool primary,
bool has_failover, bool failover,
Error **errp)
@@ -271,7 +260,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
/* Notify all filters of all NIC to do checkpoint */
colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
}
-#endif
COLOStatus *qmp_query_colo_status(Error **errp)
{
@@ -435,15 +423,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
}
qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
replication_do_checkpoint_all(&local_err);
if (local_err) {
qemu_mutex_unlock_iothread();
goto out;
}
-#else
- abort();
-#endif
colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, &local_err);
if (local_err) {
@@ -561,15 +545,11 @@ static void colo_process_checkpoint(MigrationState *s)
object_unref(OBJECT(bioc));
qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
if (local_err) {
qemu_mutex_unlock_iothread();
goto out;
}
-#else
- abort();
-#endif
vm_start();
qemu_mutex_unlock_iothread();
@@ -748,7 +728,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
return;
}
-#ifdef CONFIG_REPLICATION
replication_get_error_all(&local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -765,9 +744,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
qemu_mutex_unlock_iothread();
return;
}
-#else
- abort();
-#endif
/* Notify all filters of all NIC to do checkpoint */
colo_notify_filters_event(COLO_EVENT_CHECKPOINT, &local_err);
@@ -874,15 +850,11 @@ void *colo_process_incoming_thread(void *opaque)
object_unref(OBJECT(bioc));
qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
if (local_err) {
qemu_mutex_unlock_iothread();
goto out;
}
-#else
- abort();
-#endif
vm_start();
qemu_mutex_unlock_iothread();
trace_colo_vm_state_change("stop", "run");
diff --git a/migration/meson.build b/migration/meson.build
index 480ff6854a..9c6a8e65d3 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -13,8 +13,6 @@ softmmu_ss.add(files(
'block-dirty-bitmap.c',
'channel.c',
'channel-block.c',
- 'colo-failover.c',
- 'colo.c',
'exec.c',
'fd.c',
'global_state.c',
@@ -30,6 +28,10 @@ softmmu_ss.add(files(
'threadinfo.c',
), gnutls)
+if get_option('replication').allowed()
+ softmmu_ss.add(files('colo-failover.c', 'colo.c'))
+endif
+
softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
if get_option('live_block_migration').allowed()
softmmu_ss.add(files('block.c'))
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 4e9f00e7dc..9885d7c9f7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -643,6 +643,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, err);
}
+#ifdef CONFIG_REPLICATION
void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
@@ -650,6 +651,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
qmp_x_colo_lost_heartbeat(&err);
hmp_handle_error(mon, err);
}
+#endif
typedef struct HMPMigrationStatus {
QEMUTimer *timer;
diff --git a/migration/options.c b/migration/options.c
index 912cbadddb..eef2bd0f16 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -171,7 +171,9 @@ Property migration_properties[] = {
DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
+#ifdef CONFIG_REPLICATION
DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+#endif
DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
@@ -209,9 +211,13 @@ bool migrate_block(void)
bool migrate_colo(void)
{
+#ifdef CONFIG_REPLICATION
MigrationState *s = migrate_get_current();
return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
+#else
+ return false;
+#endif
}
bool migrate_compress(void)
@@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
MIGRATION_CAPABILITY_RDMA_PIN_ALL,
MIGRATION_CAPABILITY_COMPRESS,
MIGRATION_CAPABILITY_XBZRLE,
+#ifdef CONFIG_REPLICATION
MIGRATION_CAPABILITY_X_COLO,
+#endif
MIGRATION_CAPABILITY_VALIDATE_UUID,
MIGRATION_CAPABILITY_ZERO_COPY_SEND);
@@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
}
#endif
-#ifndef CONFIG_REPLICATION
- if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
- error_setg(errp, "QEMU compiled without replication module"
- " can't enable COLO");
- error_append_hint(errp, "Please enable replication before COLO.\n");
- return false;
- }
-#endif
-
if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
/* This check is reasonably expensive, so only when it's being
* set the first time, also it's only the destination that needs
diff --git a/qapi/migration.json b/qapi/migration.json
index 2c35b7b9cf..5cb095f7b3 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -486,7 +486,8 @@
{ 'enum': 'MigrationCapability',
'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
'compress', 'events', 'postcopy-ram',
- { 'name': 'x-colo', 'features': [ 'unstable' ] },
+ { 'name': 'x-colo', 'features': [ 'unstable' ],
+ 'if': 'CONFIG_REPLICATION' },
'release-ram',
'block', 'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
@@ -1381,7 +1382,8 @@
#
##
{ 'command': 'x-colo-lost-heartbeat',
- 'features': [ 'unstable' ] }
+ 'features': [ 'unstable' ],
+ 'if': 'CONFIG_REPLICATION' }
##
# @migrate_cancel:
@@ -1657,7 +1659,8 @@
##
{ 'struct': 'COLOStatus',
'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
- 'reason': 'COLOExitReason' } }
+ 'reason': 'COLOExitReason' },
+ 'if': 'CONFIG_REPLICATION' }
##
# @query-colo-status:
@@ -1674,7 +1677,8 @@
# Since: 3.1
##
{ 'command': 'query-colo-status',
- 'returns': 'COLOStatus' }
+ 'returns': 'COLOStatus',
+ 'if': 'CONFIG_REPLICATION' }
##
# @migrate-recover:
diff --git a/stubs/colo.c b/stubs/colo.c
new file mode 100644
index 0000000000..f306ab45d6
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,37 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "migration/migration.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+
+void colo_shutdown(void)
+{
+ abort();
+}
+
+void *colo_process_incoming_thread(void *opaque)
+{
+ abort();
+}
+
+void colo_checkpoint_notify(void *opaque)
+{
+ abort();
+}
+
+void migrate_start_colo_process(MigrationState *s)
+{
+ abort();
+}
+
+bool migration_in_colo_state(void)
+{
+ return false;
+}
+
+bool migration_incoming_in_colo_state(void)
+{
+ return false;
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index b2b5956d97..8412cad15f 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
stub_ss.add(files('target-monitor-defs.c'))
stub_ss.add(files('trace-control.c'))
stub_ss.add(files('uuid.c'))
+stub_ss.add(files('colo.c'))
stub_ss.add(files('vmstate.c'))
stub_ss.add(files('vm-stop.c'))
stub_ss.add(files('win32-kbd-hook.c'))
--
2.34.1Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
> migration_iteration_finish()
> case MIGRATION_STATUS_COLO:
> migrate_start_colo_process()
> colo_process_checkpoint()
> abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Nice patch. Thanks.
> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
> static void secondary_vm_do_failover(void)
> {
> /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
> int old_state;
> MigrationIncomingState *mis = migration_incoming_get_current();
> Error *local_err = NULL;
> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
> if (mis->migration_incoming_co) {
> qemu_coroutine_enter(mis->migration_incoming_co);
> }
> -#else
> - abort();
> -#endif
> }
With only this chunks you have proved that your argument is right.
abort() is never a solution.
> diff --git a/migration/options.c b/migration/options.c
> index 912cbadddb..eef2bd0f16 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -171,7 +171,9 @@ Property migration_properties[] = {
> DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
> DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
> DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
> DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> @@ -209,9 +211,13 @@ bool migrate_block(void)
> bool migrate_colo(void)
> {
> +#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
>
> return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
> +#else
> + return false;
> +#endif
> }
#ifdef 1
>
> bool migrate_compress(void)
> @@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
> MIGRATION_CAPABILITY_RDMA_PIN_ALL,
> MIGRATION_CAPABILITY_COMPRESS,
> MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
> MIGRATION_CAPABILITY_X_COLO,
> +#endif
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
#ifdef 2
> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> }
> #endif
>
> -#ifndef CONFIG_REPLICATION
> - if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> - error_setg(errp, "QEMU compiled without replication module"
> - " can't enable COLO");
> - error_append_hint(errp, "Please enable replication before COLO.\n");
> - return false;
> - }
> -#endif
> -
> if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> /* This check is reasonably expensive, so only when it's being
> * set the first time, also it's only the destination that needs
I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
trying to get all capabilities to this format.
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..f306ab45d6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,37 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> + abort();
> +}
This is wrong, it should be empty.
void migration_shutdown(void)
{
/*
* When the QEMU main thread exit, the COLO thread
* may wait a semaphore. So, we should wakeup the
* COLO thread before migration shutdown.
*/
colo_shutdown();
......
}
> +void *colo_process_incoming_thread(void *opaque)
> +{
> + abort();
> +}
At least print an error message?
> +void colo_checkpoint_notify(void *opaque)
> +{
> + abort();
> +}
Another error message.
It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c
changing
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
if (migration_in_colo_state()) {
colo_checkpoint_notify(s);
}
}
To
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
colo_checkpoint_refresh(s);
}
That way we can convert it to an empty function.
> +void migrate_start_colo_process(MigrationState *s)
> +{
> + abort();
> +}
Another case of changing the function interface?
case MIGRATION_STATUS_COLO:
if (!migrate_colo()) {
error_report("%s: critical error: calling COLO code without "
"COLO enabled", __func__);
}
migrate_start_colo_process(s);
The changes of functions interfaces are independent of this patch.
Later, Juan.
On 28.04.23 10:30, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> We don't allow to use x-colo capability when replication is not
>> configured. So, no reason to build COLO when replication is disabled,
>> it's unusable in this case.
>>
>> Note also that the check in migrate_caps_check() is not the only
>> restriction: some functions in migration/colo.c will just abort if
>> called with not defined CONFIG_REPLICATION, for example:
>>
>> migration_iteration_finish()
>> case MIGRATION_STATUS_COLO:
>> migrate_start_colo_process()
>> colo_process_checkpoint()
>> abort()
>>
>> It could probably make sense to have possibility to enable COLO without
>> REPLICATION, but this requires deeper audit of colo & replication code,
>> which may be done later if needed.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Nice patch. Thanks.
>
>> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
>> static void secondary_vm_do_failover(void)
>> {
>> /* COLO needs enable block-replication */
>> -#ifdef CONFIG_REPLICATION
>> int old_state;
>> MigrationIncomingState *mis = migration_incoming_get_current();
>> Error *local_err = NULL;
>> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
>> if (mis->migration_incoming_co) {
>> qemu_coroutine_enter(mis->migration_incoming_co);
>> }
>> -#else
>> - abort();
>> -#endif
>> }
>
> With only this chunks you have proved that your argument is right.
> abort() is never a solution.
>
>> diff --git a/migration/options.c b/migration/options.c
>> index 912cbadddb..eef2bd0f16 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -171,7 +171,9 @@ Property migration_properties[] = {
>> DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>> DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>> MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
>> +#ifdef CONFIG_REPLICATION
>> DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
>> +#endif
>> DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
>> DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
>> DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
>> @@ -209,9 +211,13 @@ bool migrate_block(void)
>
>> bool migrate_colo(void)
>> {
>> +#ifdef CONFIG_REPLICATION
>> MigrationState *s = migrate_get_current();
>>
>> return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
>> +#else
>> + return false;
>> +#endif
>> }
>
> #ifdef 1
>
>>
>> bool migrate_compress(void)
>> @@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>> MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>> MIGRATION_CAPABILITY_COMPRESS,
>> MIGRATION_CAPABILITY_XBZRLE,
>> +#ifdef CONFIG_REPLICATION
>> MIGRATION_CAPABILITY_X_COLO,
>> +#endif
>> MIGRATION_CAPABILITY_VALIDATE_UUID,
>> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>>
>
> #ifdef 2
>
>> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>> }
>> #endif
>>
>> -#ifndef CONFIG_REPLICATION
>> - if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>> - error_setg(errp, "QEMU compiled without replication module"
>> - " can't enable COLO");
>> - error_append_hint(errp, "Please enable replication before COLO.\n");
>> - return false;
>> - }
>> -#endif
>> -
>> if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>> /* This check is reasonably expensive, so only when it's being
>> * set the first time, also it's only the destination that needs
>
> I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
> trying to get all capabilities to this format.
OK, let's start with your idea, interface may be removed later if we want.
>
>
>> diff --git a/stubs/colo.c b/stubs/colo.c
>> new file mode 100644
>> index 0000000000..f306ab45d6
>> --- /dev/null
>> +++ b/stubs/colo.c
>> @@ -0,0 +1,37 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/notify.h"
>> +#include "net/colo-compare.h"
>> +#include "migration/colo.h"
>> +#include "migration/migration.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qapi-commands-migration.h"
>> +
>> +void colo_shutdown(void)
>> +{
>> + abort();
>> +}
>
> This is wrong, it should be empty.
Oops right. Good catch
>
> void migration_shutdown(void)
> {
> /*
> * When the QEMU main thread exit, the COLO thread
> * may wait a semaphore. So, we should wakeup the
> * COLO thread before migration shutdown.
> */
> colo_shutdown();
>
> ......
>
> }
>
>
>
>> +void *colo_process_incoming_thread(void *opaque)
>> +{
>> + abort();
>> +}
>
> At least print an error message?
OK
>
>> +void colo_checkpoint_notify(void *opaque)
>> +{
>> + abort();
>> +}
>
> Another error message.
>
> It is independently of this patch, but I am thinking about changing the
> interface and doing something like this in options.c
>
> changing
>
> if (params->has_x_checkpoint_delay) {
> s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
> if (migration_in_colo_state()) {
> colo_checkpoint_notify(s);
> }
> }
>
> To
>
> if (params->has_x_checkpoint_delay) {
> s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
> colo_checkpoint_refresh(s);
> }
>
> That way we can convert it to an empty function.
Sounds good. I can make a patch and include it to v4
>
>> +void migrate_start_colo_process(MigrationState *s)
>> +{
>> + abort();
>> +}
>
> Another case of changing the function interface?
>
> case MIGRATION_STATUS_COLO:
> if (!migrate_colo()) {
> error_report("%s: critical error: calling COLO code without "
> "COLO enabled", __func__);
> }
I think, it could be abort()
> migrate_start_colo_process(s);
>
> The changes of functions interfaces are independent of this patch.
>
Still, making empty stubs is clearer than stubs with abort(). I'll try and if changing the interface is not too much, will include it in v4.
--
Best regards,
Vladimir
On Thu, 27 Apr 2023 23:29:45 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
> migration_iteration_finish()
> case MIGRATION_STATUS_COLO:
> migrate_start_colo_process()
> colo_process_checkpoint()
> abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> ---
> hmp-commands.hx | 2 ++
> migration/colo.c | 28 -------------------------
> migration/meson.build | 6 ++++--
> migration/migration-hmp-cmds.c | 2 ++
> migration/options.c | 17 ++++++++--------
> qapi/migration.json | 12 +++++++----
> stubs/colo.c | 37 ++++++++++++++++++++++++++++++++++
> stubs/meson.build | 1 +
> 8 files changed, 62 insertions(+), 43 deletions(-)
> create mode 100644 stubs/colo.c
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
> migration (or once already in postcopy).
> ERST
>
> +#ifdef CONFIG_REPLICATION
> {
> .name = "x_colo_lost_heartbeat",
> .args_type = "",
> @@ -1043,6 +1044,7 @@ ERST
> "a failover or takeover is needed.",
> .cmd = hmp_x_colo_lost_heartbeat,
> },
> +#endif
>
> SRST
> ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c
> index 07bfa21fea..e4af47eeeb 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -26,9 +26,7 @@
> #include "qemu/rcu.h"
> #include "migration/failover.h"
> #include "migration/ram.h"
> -#ifdef CONFIG_REPLICATION
> #include "block/replication.h"
> -#endif
> #include "net/colo-compare.h"
> #include "net/colo.h"
> #include "block/block.h"
> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
> static void secondary_vm_do_failover(void)
> {
> /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
> int old_state;
> MigrationIncomingState *mis = migration_incoming_get_current();
> Error *local_err = NULL;
> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
> if (mis->migration_incoming_co) {
> qemu_coroutine_enter(mis->migration_incoming_co);
> }
> -#else
> - abort();
> -#endif
> }
>
> static void primary_vm_do_failover(void)
> {
> -#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
> int old_state;
> Error *local_err = NULL;
> @@ -181,9 +174,6 @@ static void primary_vm_do_failover(void)
>
> /* Notify COLO thread that failover work is finished */
> qemu_sem_post(&s->colo_exit_sem);
> -#else
> - abort();
> -#endif
> }
>
> COLOMode get_colo_mode(void)
> @@ -217,7 +207,6 @@ void colo_do_failover(void)
> }
> }
>
> -#ifdef CONFIG_REPLICATION
> void qmp_xen_set_replication(bool enable, bool primary,
> bool has_failover, bool failover,
> Error **errp)
> @@ -271,7 +260,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
> /* Notify all filters of all NIC to do checkpoint */
> colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
> }
> -#endif
>
> COLOStatus *qmp_query_colo_status(Error **errp)
> {
> @@ -435,15 +423,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
> }
> qemu_mutex_lock_iothread();
>
> -#ifdef CONFIG_REPLICATION
> replication_do_checkpoint_all(&local_err);
> if (local_err) {
> qemu_mutex_unlock_iothread();
> goto out;
> }
> -#else
> - abort();
> -#endif
>
> colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, &local_err);
> if (local_err) {
> @@ -561,15 +545,11 @@ static void colo_process_checkpoint(MigrationState *s)
> object_unref(OBJECT(bioc));
>
> qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
> replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
> if (local_err) {
> qemu_mutex_unlock_iothread();
> goto out;
> }
> -#else
> - abort();
> -#endif
>
> vm_start();
> qemu_mutex_unlock_iothread();
> @@ -748,7 +728,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> return;
> }
>
> -#ifdef CONFIG_REPLICATION
> replication_get_error_all(&local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -765,9 +744,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> qemu_mutex_unlock_iothread();
> return;
> }
> -#else
> - abort();
> -#endif
> /* Notify all filters of all NIC to do checkpoint */
> colo_notify_filters_event(COLO_EVENT_CHECKPOINT, &local_err);
>
> @@ -874,15 +850,11 @@ void *colo_process_incoming_thread(void *opaque)
> object_unref(OBJECT(bioc));
>
> qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
> replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
> if (local_err) {
> qemu_mutex_unlock_iothread();
> goto out;
> }
> -#else
> - abort();
> -#endif
> vm_start();
> qemu_mutex_unlock_iothread();
> trace_colo_vm_state_change("stop", "run");
> diff --git a/migration/meson.build b/migration/meson.build
> index 480ff6854a..9c6a8e65d3 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
> 'block-dirty-bitmap.c',
> 'channel.c',
> 'channel-block.c',
> - 'colo-failover.c',
> - 'colo.c',
> 'exec.c',
> 'fd.c',
> 'global_state.c',
> @@ -30,6 +28,10 @@ softmmu_ss.add(files(
> 'threadinfo.c',
> ), gnutls)
>
> +if get_option('replication').allowed()
> + softmmu_ss.add(files('colo-failover.c', 'colo.c'))
> +endif
> +
> softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
> if get_option('live_block_migration').allowed()
> softmmu_ss.add(files('block.c'))
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 4e9f00e7dc..9885d7c9f7 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -643,6 +643,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> +#ifdef CONFIG_REPLICATION
> void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> @@ -650,6 +651,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
> qmp_x_colo_lost_heartbeat(&err);
> hmp_handle_error(mon, err);
> }
> +#endif
>
> typedef struct HMPMigrationStatus {
> QEMUTimer *timer;
> diff --git a/migration/options.c b/migration/options.c
> index 912cbadddb..eef2bd0f16 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -171,7 +171,9 @@ Property migration_properties[] = {
> DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
> DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
> DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
> DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> @@ -209,9 +211,13 @@ bool migrate_block(void)
>
> bool migrate_colo(void)
> {
> +#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
>
> return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
> +#else
> + return false;
> +#endif
> }
>
> bool migrate_compress(void)
> @@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
> MIGRATION_CAPABILITY_RDMA_PIN_ALL,
> MIGRATION_CAPABILITY_COMPRESS,
> MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
> MIGRATION_CAPABILITY_X_COLO,
> +#endif
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> }
> #endif
>
> -#ifndef CONFIG_REPLICATION
> - if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> - error_setg(errp, "QEMU compiled without replication module"
> - " can't enable COLO");
> - error_append_hint(errp, "Please enable replication before COLO.\n");
> - return false;
> - }
> -#endif
> -
> if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> /* This check is reasonably expensive, so only when it's being
> * set the first time, also it's only the destination that needs
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2c35b7b9cf..5cb095f7b3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -486,7 +486,8 @@
> { 'enum': 'MigrationCapability',
> 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> 'compress', 'events', 'postcopy-ram',
> - { 'name': 'x-colo', 'features': [ 'unstable' ] },
> + { 'name': 'x-colo', 'features': [ 'unstable' ],
> + 'if': 'CONFIG_REPLICATION' },
> 'release-ram',
> 'block', 'return-path', 'pause-before-switchover', 'multifd',
> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> @@ -1381,7 +1382,8 @@
> #
> ##
> { 'command': 'x-colo-lost-heartbeat',
> - 'features': [ 'unstable' ] }
> + 'features': [ 'unstable' ],
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @migrate_cancel:
> @@ -1657,7 +1659,8 @@
> ##
> { 'struct': 'COLOStatus',
> 'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> - 'reason': 'COLOExitReason' } }
> + 'reason': 'COLOExitReason' },
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @query-colo-status:
> @@ -1674,7 +1677,8 @@
> # Since: 3.1
> ##
> { 'command': 'query-colo-status',
> - 'returns': 'COLOStatus' }
> + 'returns': 'COLOStatus',
> + 'if': 'CONFIG_REPLICATION' }
>
> ##
> # @migrate-recover:
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..f306ab45d6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,37 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> + abort();
> +}
> +
> +void *colo_process_incoming_thread(void *opaque)
> +{
> + abort();
> +}
> +
> +void colo_checkpoint_notify(void *opaque)
> +{
> + abort();
> +}
> +
> +void migrate_start_colo_process(MigrationState *s)
> +{
> + abort();
> +}
> +
> +bool migration_in_colo_state(void)
> +{
> + return false;
> +}
> +
> +bool migration_incoming_in_colo_state(void)
> +{
> + return false;
> +}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index b2b5956d97..8412cad15f 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
> stub_ss.add(files('target-monitor-defs.c'))
> stub_ss.add(files('trace-control.c'))
> stub_ss.add(files('uuid.c'))
> +stub_ss.add(files('colo.c'))
> stub_ss.add(files('vmstate.c'))
> stub_ss.add(files('vm-stop.c'))
> stub_ss.add(files('win32-kbd-hook.c'))
--
© 2016 - 2026 Red Hat, Inc.