| Message ID | 20191016022933.7276-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | apic: Fix migration breakage of >255 vcpus | expand |
* Peter Xu (peterx@redhat.com) wrote: > Before finally register one SaveStateEntry, we detect for duplicated > entries. This could be helpful to notify us asap instead of get > silent migration failures which could be hard to diagnose. > > For example, this patch will generate a message like this (if without > previous fixes on x2apic) as long as we wants to boot a VM instance > with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will > bail out even before VM starts: > > savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0 > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> Right, lets see what this triggers :-) Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/savevm.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 1e44f06d7a..83e91ddafa 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -264,6 +264,8 @@ static SaveState savevm_state = { > .global_section_id = 0, > }; > > +static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id); > + > static bool should_validate_capability(int capability) > { > assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX); > @@ -714,6 +716,18 @@ static void savevm_state_handler_insert(SaveStateEntry *nse) > > assert(priority <= MIG_PRI_MAX); > > + /* > + * This should never happen otherwise migration will probably fail > + * silently somewhere because we can be wrongly applying one > + * object properties upon another one. Bail out ASAP. > + */ > + if (find_se(nse->idstr, nse->instance_id)) { > + error_report("%s: Detected duplicate SaveStateEntry: " > + "id=%s, instance_id=0x%"PRIx32, __func__, > + nse->idstr, nse->instance_id); > + exit(EXIT_FAILURE); > + } > + > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (save_state_priority(se) < priority) { > break; > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Xu <peterx@redhat.com> wrote: > Before finally register one SaveStateEntry, we detect for duplicated > entries. This could be helpful to notify us asap instead of get > silent migration failures which could be hard to diagnose. > > For example, this patch will generate a message like this (if without > previous fixes on x2apic) as long as we wants to boot a VM instance > with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will > bail out even before VM starts: > > savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0 > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
Peter Xu <peterx@redhat.com> wrote: > Before finally register one SaveStateEntry, we detect for duplicated > entries. This could be helpful to notify us asap instead of get > silent migration failures which could be hard to diagnose. > > For example, this patch will generate a message like this (if without > previous fixes on x2apic) as long as we wants to boot a VM instance > with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will > bail out even before VM starts: > > savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0 > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> Hi peter I have to drop this one. There is something on current tree (I think that it is the VMSTATE_IF) that make isa-ide choke for migration on "make check". Will take a look to it later. Later, Juan.
diff --git a/migration/savevm.c b/migration/savevm.c index 1e44f06d7a..83e91ddafa 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -264,6 +264,8 @@ static SaveState savevm_state = { .global_section_id = 0, }; +static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id); + static bool should_validate_capability(int capability) { assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX); @@ -714,6 +716,18 @@ static void savevm_state_handler_insert(SaveStateEntry *nse) assert(priority <= MIG_PRI_MAX); + /* + * This should never happen otherwise migration will probably fail + * silently somewhere because we can be wrongly applying one + * object properties upon another one. Bail out ASAP. + */ + if (find_se(nse->idstr, nse->instance_id)) { + error_report("%s: Detected duplicate SaveStateEntry: " + "id=%s, instance_id=0x%"PRIx32, __func__, + nse->idstr, nse->instance_id); + exit(EXIT_FAILURE); + } + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (save_state_priority(se) < priority) { break;
Before finally register one SaveStateEntry, we detect for duplicated entries. This could be helpful to notify us asap instead of get silent migration failures which could be hard to diagnose. For example, this patch will generate a message like this (if without previous fixes on x2apic) as long as we wants to boot a VM instance with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will bail out even before VM starts: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0 Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/savevm.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)