Re: [PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp
* Peter Xu (pet...@redhat.com) wrote:
> On Thu, Jun 09, 2022 at 05:02:29PM -0400, Peter Xu wrote:
> > On Wed, Jun 08, 2022 at 06:05:28PM +0100, Dr. David Alan Gilbert wrote:
> > > > @@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void
> > > > *opaque)
> > > > /* TODO we should move all of this lot into postcopy_ram.c or a
> > > > shared code
> > > > * in migration.c
> > > > */
> > > > -cpu_synchronize_all_post_init();
> > > > +cpu_synchronize_all_post_init(_err);
> > > > +if (local_err) {
> > > > +/*
> > > > + * TODO: a better way to do this is to tell the src that we
> > > > cannot
> > > > + * run the VM here so hopefully we can keep the VM running on
> > > > src
> > > > + * and immediately halt the switch-over. But that needs work.
> > >
> > > Yes, I think it is possible; unlike some of the later errors in the same
> > > function, in this case we know no disks/network/etc have been touched,
> > > so we should be able to recover.
> > > I wonder if we can move the postcopy_state_set(POSTCOPY_INCOMING_RUNNING)
> > > out of loadvm_postcopy_handle_run to after this point.
> > >
> > > We've already got the return path, so we should be able to signal the
> > > failure unless we're very unlucky.
> >
> > Right. It's just that for the new ACK we may need to modify the return
> > path protocol for sure, because none of the existing ones can notify such
> > an information.
> >
> > One idea is to reuse MIG_RP_MSG_RESUME_ACK, it was only used for postcopy
> > recovery before to do the final handshake with offload=1 only (which is
> > defined as MIGRATION_RESUME_ACK_VALUE). We could try to fill in the
> > payload with some !1 value, to tell the source that we NACK the migration
> > then src fails the migration as long as possible?
> >
> > That seems to be even compatibile with one old qemu migrating to a new qemu
> > scenario, because when the old qemu notices the MIG_RP_MSG_RESUME_ACK
> > message with !1 payload, it'll mark the rp bad:
>
> Oh it won't be compatible.. The clean way to do this is we need to modify
> the src qemu to halt in postcopy_start() to wait for that ack before
> continue. That may need another cap/param to enable.
OK; I was wondering aobut sending a RP_MSG_SHUT with a failure; but if
you'd need to change the source it's still a problem.
> The thing is I'm not very sure whether this will be worth it.
>
> Non-compatible migrations should be rare on put register failures. For the
> issue I was working on, it was actually a kernel bug that triggered it but
> it's just hard to figure out where's wrong. With properly working kernels
> and matching hosts they should just not really heppen. I'm worried adding
> too much complexity could over-engineer things without much benefits.
OK that makes sense.
> In that case, I'd think it proper if we start with what this patchset
> provides, which at least allows us to fail in a crystal clear way?
Yes, the clear error is important.
Dave
> >
> > if (migrate_handle_rp_resume_ack(ms, tmp32)) {
> > mark_source_rp_bad(ms);
> > goto out;
> > }
> >
> > static int migrate_handle_rp_resume_ack(MigrationState *s, uint32_t value)
> > {
> > trace_source_return_path_thread_resume_ack(value);
> >
> > if (value != MIGRATION_RESUME_ACK_VALUE) {
> > error_report("%s: illegal resume_ack value %"PRIu32,
> >__func__, value);
> > return -1;
> > }
> > ...
> > }
> >
> > If it looks generally good, I can try with such a change in v2.
> >
> > Thanks,
> >
> > --
> > Peter Xu
>
> --
> Peter Xu
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp
On Thu, Jun 09, 2022 at 05:02:29PM -0400, Peter Xu wrote:
> On Wed, Jun 08, 2022 at 06:05:28PM +0100, Dr. David Alan Gilbert wrote:
> > > @@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void
> > > *opaque)
> > > /* TODO we should move all of this lot into postcopy_ram.c or a
> > > shared code
> > > * in migration.c
> > > */
> > > -cpu_synchronize_all_post_init();
> > > +cpu_synchronize_all_post_init(_err);
> > > +if (local_err) {
> > > +/*
> > > + * TODO: a better way to do this is to tell the src that we
> > > cannot
> > > + * run the VM here so hopefully we can keep the VM running on src
> > > + * and immediately halt the switch-over. But that needs work.
> >
> > Yes, I think it is possible; unlike some of the later errors in the same
> > function, in this case we know no disks/network/etc have been touched,
> > so we should be able to recover.
> > I wonder if we can move the postcopy_state_set(POSTCOPY_INCOMING_RUNNING)
> > out of loadvm_postcopy_handle_run to after this point.
> >
> > We've already got the return path, so we should be able to signal the
> > failure unless we're very unlucky.
>
> Right. It's just that for the new ACK we may need to modify the return
> path protocol for sure, because none of the existing ones can notify such
> an information.
>
> One idea is to reuse MIG_RP_MSG_RESUME_ACK, it was only used for postcopy
> recovery before to do the final handshake with offload=1 only (which is
> defined as MIGRATION_RESUME_ACK_VALUE). We could try to fill in the
> payload with some !1 value, to tell the source that we NACK the migration
> then src fails the migration as long as possible?
>
> That seems to be even compatibile with one old qemu migrating to a new qemu
> scenario, because when the old qemu notices the MIG_RP_MSG_RESUME_ACK
> message with !1 payload, it'll mark the rp bad:
Oh it won't be compatible.. The clean way to do this is we need to modify
the src qemu to halt in postcopy_start() to wait for that ack before
continue. That may need another cap/param to enable.
The thing is I'm not very sure whether this will be worth it.
Non-compatible migrations should be rare on put register failures. For the
issue I was working on, it was actually a kernel bug that triggered it but
it's just hard to figure out where's wrong. With properly working kernels
and matching hosts they should just not really heppen. I'm worried adding
too much complexity could over-engineer things without much benefits.
In that case, I'd think it proper if we start with what this patchset
provides, which at least allows us to fail in a crystal clear way?
>
> if (migrate_handle_rp_resume_ack(ms, tmp32)) {
> mark_source_rp_bad(ms);
> goto out;
> }
>
> static int migrate_handle_rp_resume_ack(MigrationState *s, uint32_t value)
> {
> trace_source_return_path_thread_resume_ack(value);
>
> if (value != MIGRATION_RESUME_ACK_VALUE) {
> error_report("%s: illegal resume_ack value %"PRIu32,
>__func__, value);
> return -1;
> }
> ...
> }
>
> If it looks generally good, I can try with such a change in v2.
>
> Thanks,
>
> --
> Peter Xu
--
Peter Xu
Re: [PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp
On Wed, Jun 08, 2022 at 06:05:28PM +0100, Dr. David Alan Gilbert wrote:
> > @@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void
> > *opaque)
> > /* TODO we should move all of this lot into postcopy_ram.c or a shared
> > code
> > * in migration.c
> > */
> > -cpu_synchronize_all_post_init();
> > +cpu_synchronize_all_post_init(_err);
> > +if (local_err) {
> > +/*
> > + * TODO: a better way to do this is to tell the src that we cannot
> > + * run the VM here so hopefully we can keep the VM running on src
> > + * and immediately halt the switch-over. But that needs work.
>
> Yes, I think it is possible; unlike some of the later errors in the same
> function, in this case we know no disks/network/etc have been touched,
> so we should be able to recover.
> I wonder if we can move the postcopy_state_set(POSTCOPY_INCOMING_RUNNING)
> out of loadvm_postcopy_handle_run to after this point.
>
> We've already got the return path, so we should be able to signal the
> failure unless we're very unlucky.
Right. It's just that for the new ACK we may need to modify the return
path protocol for sure, because none of the existing ones can notify such
an information.
One idea is to reuse MIG_RP_MSG_RESUME_ACK, it was only used for postcopy
recovery before to do the final handshake with offload=1 only (which is
defined as MIGRATION_RESUME_ACK_VALUE). We could try to fill in the
payload with some !1 value, to tell the source that we NACK the migration
then src fails the migration as long as possible?
That seems to be even compatibile with one old qemu migrating to a new qemu
scenario, because when the old qemu notices the MIG_RP_MSG_RESUME_ACK
message with !1 payload, it'll mark the rp bad:
if (migrate_handle_rp_resume_ack(ms, tmp32)) {
mark_source_rp_bad(ms);
goto out;
}
static int migrate_handle_rp_resume_ack(MigrationState *s, uint32_t value)
{
trace_source_return_path_thread_resume_ack(value);
if (value != MIGRATION_RESUME_ACK_VALUE) {
error_report("%s: illegal resume_ack value %"PRIu32,
__func__, value);
return -1;
}
...
}
If it looks generally good, I can try with such a change in v2.
Thanks,
--
Peter Xu
Re: [PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp
* Peter Xu (pet...@redhat.com) wrote:
> Allow cpu_synchronize_all_post_init() to fail with an errp when it's set.
> Modify both precopy and postcopy to try to detect such error.
>
> Signed-off-by: Peter Xu
> ---
> hw/core/machine.c | 2 +-
> include/sysemu/cpus.h | 2 +-
> migration/savevm.c| 20 +---
> softmmu/cpus.c| 2 +-
> 4 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c53548d0b1..b5daad82f8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1447,7 +1447,7 @@ void qemu_remove_machine_init_done_notifier(Notifier
> *notify)
>
> void qdev_machine_creation_done(void)
> {
> -cpu_synchronize_all_post_init();
> +cpu_synchronize_all_post_init(NULL);
>
> if (current_machine->boot_config.has_once) {
> qemu_boot_set(current_machine->boot_config.once, _fatal);
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index b5c87d48b3..a51ee46441 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -45,7 +45,7 @@ bool cpus_are_resettable(void);
>
> void cpu_synchronize_all_states(void);
> void cpu_synchronize_all_post_reset(void);
> -void cpu_synchronize_all_post_init(void);
> +void cpu_synchronize_all_post_init(Error **errp);
> void cpu_synchronize_all_pre_loadvm(void);
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d9076897b8..1175ddefd4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
> /* TODO we should move all of this lot into postcopy_ram.c or a shared
> code
> * in migration.c
> */
> -cpu_synchronize_all_post_init();
> +cpu_synchronize_all_post_init(_err);
> +if (local_err) {
> +/*
> + * TODO: a better way to do this is to tell the src that we cannot
> + * run the VM here so hopefully we can keep the VM running on src
> + * and immediately halt the switch-over. But that needs work.
Yes, I think it is possible; unlike some of the later errors in the same
function, in this case we know no disks/network/etc have been touched,
so we should be able to recover.
I wonder if we can move the postcopy_state_set(POSTCOPY_INCOMING_RUNNING)
out of loadvm_postcopy_handle_run to after this point.
We've already got the return path, so we should be able to signal the
failure unless we're very unlucky.
Dave
> + */
> +error_report_err(local_err);
> +local_err = NULL;
> +autostart = false;
> +}
>
> trace_loadvm_postcopy_handle_run_bh("after cpu sync");
>
> @@ -2772,7 +2782,11 @@ int qemu_loadvm_state(QEMUFile *f)
> }
>
> qemu_loadvm_state_cleanup();
> -cpu_synchronize_all_post_init();
> +cpu_synchronize_all_post_init(_err);
> +if (local_err) {
> +error_report_err(local_err);
> +return -EINVAL;
> +}
>
> return ret;
> }
> @@ -2789,7 +2803,7 @@ int qemu_load_device_state(QEMUFile *f)
> return ret;
> }
>
> -cpu_synchronize_all_post_init();
> +cpu_synchronize_all_post_init(NULL);
> return 0;
> }
>
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 464c06201c..59c70fd496 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -146,7 +146,7 @@ void cpu_synchronize_all_post_reset(void)
> }
> }
>
> -void cpu_synchronize_all_post_init(void)
> +void cpu_synchronize_all_post_init(Error **errp)
> {
> CPUState *cpu;
>
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp
Allow cpu_synchronize_all_post_init() to fail with an errp when it's set.
Modify both precopy and postcopy to try to detect such error.
Signed-off-by: Peter Xu
---
hw/core/machine.c | 2 +-
include/sysemu/cpus.h | 2 +-
migration/savevm.c| 20 +---
softmmu/cpus.c| 2 +-
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c53548d0b1..b5daad82f8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1447,7 +1447,7 @@ void qemu_remove_machine_init_done_notifier(Notifier
*notify)
void qdev_machine_creation_done(void)
{
-cpu_synchronize_all_post_init();
+cpu_synchronize_all_post_init(NULL);
if (current_machine->boot_config.has_once) {
qemu_boot_set(current_machine->boot_config.once, _fatal);
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index b5c87d48b3..a51ee46441 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -45,7 +45,7 @@ bool cpus_are_resettable(void);
void cpu_synchronize_all_states(void);
void cpu_synchronize_all_post_reset(void);
-void cpu_synchronize_all_post_init(void);
+void cpu_synchronize_all_post_init(Error **errp);
void cpu_synchronize_all_pre_loadvm(void);
#ifndef CONFIG_USER_ONLY
diff --git a/migration/savevm.c b/migration/savevm.c
index d9076897b8..1175ddefd4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
/* TODO we should move all of this lot into postcopy_ram.c or a shared code
* in migration.c
*/
-cpu_synchronize_all_post_init();
+cpu_synchronize_all_post_init(_err);
+if (local_err) {
+/*
+ * TODO: a better way to do this is to tell the src that we cannot
+ * run the VM here so hopefully we can keep the VM running on src
+ * and immediately halt the switch-over. But that needs work.
+ */
+error_report_err(local_err);
+local_err = NULL;
+autostart = false;
+}
trace_loadvm_postcopy_handle_run_bh("after cpu sync");
@@ -2772,7 +2782,11 @@ int qemu_loadvm_state(QEMUFile *f)
}
qemu_loadvm_state_cleanup();
-cpu_synchronize_all_post_init();
+cpu_synchronize_all_post_init(_err);
+if (local_err) {
+error_report_err(local_err);
+return -EINVAL;
+}
return ret;
}
@@ -2789,7 +2803,7 @@ int qemu_load_device_state(QEMUFile *f)
return ret;
}
-cpu_synchronize_all_post_init();
+cpu_synchronize_all_post_init(NULL);
return 0;
}
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 464c06201c..59c70fd496 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -146,7 +146,7 @@ void cpu_synchronize_all_post_reset(void)
}
}
-void cpu_synchronize_all_post_init(void)
+void cpu_synchronize_all_post_init(Error **errp)
{
CPUState *cpu;
--
2.32.0
