| Message ID | 1714406135-451286-6-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > Provide the VMStateDescription precreate field to mark objects that must > be loaded on the incoming side before devices have been created, because > they provide properties that will be needed at creation time. They will > be saved to and loaded from their own QEMUFile, via It's not obvious to me what the reason is to have a separate QEMUFile. Could you expand on this? > qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these > functions are not yet called in this patch. Allow them to be called > before or after normal migration is active, when current_migration and > current_incoming are not valid. >
On 5/7/2024 5:02 PM, Fabiano Rosas wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > >> Provide the VMStateDescription precreate field to mark objects that must >> be loaded on the incoming side before devices have been created, because >> they provide properties that will be needed at creation time. They will >> be saved to and loaded from their own QEMUFile, via > > It's not obvious to me what the reason is to have a separate > QEMUFile. Could you expand on this? The migration stream is read in the calling sequence at B below, but precreate state is needed at A before chardev and memory backends are created. main() qemu_init() A: qemu_create_early_backends() qemu_create_late_backends() migration_object_init() qmp_x_exit_preconfig() qmp_migrate_incoming() qemu_default_main() qemu_main_loop() fd_accept_incoming_migration() migration_channel_process_incoming() migration_ioc_process_incoming() migration_incoming_process() process_incoming_migration_co() B: qemu_loadvm_state() precreate objects could be emitted first in the existing migration stream and read at A, but this requires untangling numerous ordering dependencies amongst migration_object_init, qemu_create_machine, configure_accelerators, monitor init, and the main loop. - Steve
Steve Sistare <steven.sistare@oracle.com> writes: > Provide the VMStateDescription precreate field to mark objects that must > be loaded on the incoming side before devices have been created, because > they provide properties that will be needed at creation time. They will > be saved to and loaded from their own QEMUFile, via > qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these > functions are not yet called in this patch. Allow them to be called > before or after normal migration is active, when current_migration and > current_incoming are not valid. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Mon, Apr 29, 2024 at 08:55:14AM -0700, Steve Sistare wrote: > Provide the VMStateDescription precreate field to mark objects that must > be loaded on the incoming side before devices have been created, because > they provide properties that will be needed at creation time. They will > be saved to and loaded from their own QEMUFile, via > qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these > functions are not yet called in this patch. Allow them to be called > before or after normal migration is active, when current_migration and > current_incoming are not valid. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/migration/vmstate.h | 6 ++++ > migration/savevm.c | 69 +++++++++++++++++++++++++++++++++++++++++---- > migration/savevm.h | 3 ++ > 3 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 294d2d8..4691334 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -198,6 +198,12 @@ struct VMStateDescription { > * a QEMU_VM_SECTION_START section. > */ > bool early_setup; > + > + /* > + * Send/receive this object in the precreate migration stream. > + */ > + bool precreate; > + > int version_id; > int minimum_version_id; > MigrationPriority priority; > diff --git a/migration/savevm.c b/migration/savevm.c > index 9789823..a30bcd9 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -239,6 +239,7 @@ static SaveState savevm_state = { > > #define SAVEVM_FOREACH(se, entry) \ > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) \ > + if (!se->vmsd || !se->vmsd->precreate) > > #define SAVEVM_FOREACH_ALL(se, entry) \ > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) > @@ -1006,13 +1007,19 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > } > } > > +static bool send_section_footer(SaveStateEntry *se) > +{ > + return (se->vmsd && se->vmsd->precreate) || > + migrate_get_current()->send_section_footer; > +} Does the precreate vmsd "require" the footer? Or it should also work? IMHO it's less optimal to bind features without good reasons. Thanks,
On 5/27/2024 2:16 PM, Peter Xu wrote: > On Mon, Apr 29, 2024 at 08:55:14AM -0700, Steve Sistare wrote: >> Provide the VMStateDescription precreate field to mark objects that must >> be loaded on the incoming side before devices have been created, because >> they provide properties that will be needed at creation time. They will >> be saved to and loaded from their own QEMUFile, via >> qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these >> functions are not yet called in this patch. Allow them to be called >> before or after normal migration is active, when current_migration and >> current_incoming are not valid. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> include/migration/vmstate.h | 6 ++++ >> migration/savevm.c | 69 +++++++++++++++++++++++++++++++++++++++++---- >> migration/savevm.h | 3 ++ >> 3 files changed, 73 insertions(+), 5 deletions(-) >> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >> index 294d2d8..4691334 100644 >> --- a/include/migration/vmstate.h >> +++ b/include/migration/vmstate.h >> @@ -198,6 +198,12 @@ struct VMStateDescription { >> * a QEMU_VM_SECTION_START section. >> */ >> bool early_setup; >> + >> + /* >> + * Send/receive this object in the precreate migration stream. >> + */ >> + bool precreate; >> + >> int version_id; >> int minimum_version_id; >> MigrationPriority priority; >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 9789823..a30bcd9 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -239,6 +239,7 @@ static SaveState savevm_state = { >> >> #define SAVEVM_FOREACH(se, entry) \ >> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) \ >> + if (!se->vmsd || !se->vmsd->precreate) >> >> #define SAVEVM_FOREACH_ALL(se, entry) \ >> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) >> @@ -1006,13 +1007,19 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, >> } >> } >> >> +static bool send_section_footer(SaveStateEntry *se) >> +{ >> + return (se->vmsd && se->vmsd->precreate) || >> + migrate_get_current()->send_section_footer; >> +} > > Does the precreate vmsd "require" the footer? Or it should also work? > IMHO it's less optimal to bind features without good reasons. It is not required. However, IMO we should not treat send-section-footer as a fungible feature. It is strictly an improvement, as was added to catch misformated sections. It is only registered as a feature for backwards compatibility with qemu 2.3 and xen. For a brand new data stream such as precreate, where we are not constrained by backwards compatibility, we should unconditionally use the better protocol, and always send the footer. - Steve
On Tue, May 28, 2024 at 11:09:49AM -0400, Steven Sistare wrote: > On 5/27/2024 2:16 PM, Peter Xu wrote: > > On Mon, Apr 29, 2024 at 08:55:14AM -0700, Steve Sistare wrote: > > > Provide the VMStateDescription precreate field to mark objects that must > > > be loaded on the incoming side before devices have been created, because > > > they provide properties that will be needed at creation time. They will > > > be saved to and loaded from their own QEMUFile, via > > > qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these > > > functions are not yet called in this patch. Allow them to be called > > > before or after normal migration is active, when current_migration and > > > current_incoming are not valid. > > > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > > --- > > > include/migration/vmstate.h | 6 ++++ > > > migration/savevm.c | 69 +++++++++++++++++++++++++++++++++++++++++---- > > > migration/savevm.h | 3 ++ > > > 3 files changed, 73 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > > > index 294d2d8..4691334 100644 > > > --- a/include/migration/vmstate.h > > > +++ b/include/migration/vmstate.h > > > @@ -198,6 +198,12 @@ struct VMStateDescription { > > > * a QEMU_VM_SECTION_START section. > > > */ > > > bool early_setup; > > > + > > > + /* > > > + * Send/receive this object in the precreate migration stream. > > > + */ > > > + bool precreate; > > > + > > > int version_id; > > > int minimum_version_id; > > > MigrationPriority priority; > > > diff --git a/migration/savevm.c b/migration/savevm.c > > > index 9789823..a30bcd9 100644 > > > --- a/migration/savevm.c > > > +++ b/migration/savevm.c > > > @@ -239,6 +239,7 @@ static SaveState savevm_state = { > > > #define SAVEVM_FOREACH(se, entry) \ > > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) \ > > > + if (!se->vmsd || !se->vmsd->precreate) > > > #define SAVEVM_FOREACH_ALL(se, entry) \ > > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) > > > @@ -1006,13 +1007,19 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > > > } > > > } > > > +static bool send_section_footer(SaveStateEntry *se) > > > +{ > > > + return (se->vmsd && se->vmsd->precreate) || > > > + migrate_get_current()->send_section_footer; > > > +} > > > > Does the precreate vmsd "require" the footer? Or it should also work? > > IMHO it's less optimal to bind features without good reasons. > > It is not required. However, IMO we should not treat send-section-footer as > a fungible feature. It is strictly an improvement, as was added to catch > misformated sections. It is only registered as a feature for backwards > compatibility with qemu 2.3 and xen. > > For a brand new data stream such as precreate, where we are not constrained > by backwards compatibility, we should unconditionally use the better protocol, > and always send the footer. I see your point, but I still don't think we should mangle these things. It makes future justification harder on whether section footer should be sent. Take example of whatever new feature added for migration like mapped-ram, we might also want to enforce it by adding "return migrate_mapped_ram() || ..." but it means we keep growing this with no benefit. What you worry on "what if this is turned off" isn't a real one: nobody will turn it off! We started to deprecate machines, and I had a feeling it will be enforced at some point by default.
On 5/29/2024 2:39 PM, Peter Xu wrote: > On Tue, May 28, 2024 at 11:09:49AM -0400, Steven Sistare wrote: >> On 5/27/2024 2:16 PM, Peter Xu wrote: >>> On Mon, Apr 29, 2024 at 08:55:14AM -0700, Steve Sistare wrote: >>>> Provide the VMStateDescription precreate field to mark objects that must >>>> be loaded on the incoming side before devices have been created, because >>>> they provide properties that will be needed at creation time. They will >>>> be saved to and loaded from their own QEMUFile, via >>>> qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these >>>> functions are not yet called in this patch. Allow them to be called >>>> before or after normal migration is active, when current_migration and >>>> current_incoming are not valid. >>>> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> include/migration/vmstate.h | 6 ++++ >>>> migration/savevm.c | 69 +++++++++++++++++++++++++++++++++++++++++---- >>>> migration/savevm.h | 3 ++ >>>> 3 files changed, 73 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index 294d2d8..4691334 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -198,6 +198,12 @@ struct VMStateDescription { >>>> * a QEMU_VM_SECTION_START section. >>>> */ >>>> bool early_setup; >>>> + >>>> + /* >>>> + * Send/receive this object in the precreate migration stream. >>>> + */ >>>> + bool precreate; >>>> + >>>> int version_id; >>>> int minimum_version_id; >>>> MigrationPriority priority; >>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>> index 9789823..a30bcd9 100644 >>>> --- a/migration/savevm.c >>>> +++ b/migration/savevm.c >>>> @@ -239,6 +239,7 @@ static SaveState savevm_state = { >>>> #define SAVEVM_FOREACH(se, entry) \ >>>> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) \ >>>> + if (!se->vmsd || !se->vmsd->precreate) >>>> #define SAVEVM_FOREACH_ALL(se, entry) \ >>>> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) >>>> @@ -1006,13 +1007,19 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, >>>> } >>>> } >>>> +static bool send_section_footer(SaveStateEntry *se) >>>> +{ >>>> + return (se->vmsd && se->vmsd->precreate) || >>>> + migrate_get_current()->send_section_footer; >>>> +} >>> >>> Does the precreate vmsd "require" the footer? Or it should also work? >>> IMHO it's less optimal to bind features without good reasons. >> >> It is not required. However, IMO we should not treat send-section-footer as >> a fungible feature. It is strictly an improvement, as was added to catch >> misformated sections. It is only registered as a feature for backwards >> compatibility with qemu 2.3 and xen. >> >> For a brand new data stream such as precreate, where we are not constrained >> by backwards compatibility, we should unconditionally use the better protocol, >> and always send the footer. > > I see your point, but I still don't think we should mangle these things. > It makes future justification harder on whether section footer should be > sent. > > Take example of whatever new feature added for migration like mapped-ram, > we might also want to enforce it by adding "return migrate_mapped_ram() || > ..." but it means we keep growing this with no benefit. > > What you worry on "what if this is turned off" isn't a real one: nobody > will turn it off! We started to deprecate machines, and I had a feeling > it will be enforced at some point by default. That's fine, I'll delete the send_section_footer() function. - Steve
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 294d2d8..4691334 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -198,6 +198,12 @@ struct VMStateDescription { * a QEMU_VM_SECTION_START section. */ bool early_setup; + + /* + * Send/receive this object in the precreate migration stream. + */ + bool precreate; + int version_id; int minimum_version_id; MigrationPriority priority; diff --git a/migration/savevm.c b/migration/savevm.c index 9789823..a30bcd9 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -239,6 +239,7 @@ static SaveState savevm_state = { #define SAVEVM_FOREACH(se, entry) \ QTAILQ_FOREACH(se, &savevm_state.handlers, entry) \ + if (!se->vmsd || !se->vmsd->precreate) #define SAVEVM_FOREACH_ALL(se, entry) \ QTAILQ_FOREACH(se, &savevm_state.handlers, entry) @@ -1006,13 +1007,19 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, } } +static bool send_section_footer(SaveStateEntry *se) +{ + return (se->vmsd && se->vmsd->precreate) || + migrate_get_current()->send_section_footer; +} + /* * Write a footer onto device sections that catches cases misformatted device * sections. */ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) { - if (migrate_get_current()->send_section_footer) { + if (send_section_footer(se)) { qemu_put_byte(f, QEMU_VM_SECTION_FOOTER); qemu_put_be32(f, se->section_id); } @@ -1319,6 +1326,52 @@ int qemu_savevm_state_prepare(Error **errp) return 0; } +int qemu_savevm_precreate_save(QEMUFile *f, Error **errp) +{ + int ret; + SaveStateEntry *se; + + qemu_put_be32(f, QEMU_VM_FILE_MAGIC); + qemu_put_be32(f, QEMU_VM_FILE_VERSION); + + SAVEVM_FOREACH_ALL(se, entry) { + if (se->vmsd && se->vmsd->precreate) { + ret = vmstate_save(f, se, NULL, errp); + if (ret) { + qemu_file_set_error(f, ret); + return ret; + } + } + } + qemu_fflush(f); + return 0; +} + +int qemu_savevm_precreate_load(QEMUFile *f, Error **errp) +{ + unsigned int v; + int ret; + + v = qemu_get_be32(f); + if (v != QEMU_VM_FILE_MAGIC) { + error_setg(errp, "Not a migration stream"); + return -EINVAL; + } + + v = qemu_get_be32(f); + if (v != QEMU_VM_FILE_VERSION) { + error_setg(errp, "Unsupported migration stream version"); + return -ENOTSUP; + } + + ret = qemu_loadvm_state_main(f, NULL); + if (ret) { + error_setg_errno(errp, -ret, "qemu_savevm_precreate_load"); + } + + return ret; +} + int qemu_savevm_state_setup(QEMUFile *f, Error **errp) { ERRP_GUARD(); @@ -2559,7 +2612,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se) uint8_t read_mark; uint32_t read_section_id; - if (!migrate_get_current()->send_section_footer) { + if (!send_section_footer(se)) { /* No footer to check */ return true; } @@ -2895,9 +2948,12 @@ retry: while (true) { section_type = qemu_get_byte(f); - ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, NULL); - if (ret) { - break; + if (mis) { + ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, + NULL); + if (ret) { + break; + } } trace_qemu_loadvm_state_section(section_type); @@ -2936,6 +2992,9 @@ retry: out: if (ret < 0) { qemu_file_set_error(f, ret); + if (!mis) { + return ret; + } /* Cancel bitmaps incoming regardless of recovery */ dirty_bitmap_mig_cancel_incoming(); diff --git a/migration/savevm.h b/migration/savevm.h index 9ec96a9..6f207b5 100644 --- a/migration/savevm.h +++ b/migration/savevm.h @@ -70,4 +70,7 @@ int qemu_loadvm_approve_switchover(void); int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, bool in_postcopy, bool inactivate_disks); +int qemu_savevm_precreate_save(QEMUFile *f, Error **errp); +int qemu_savevm_precreate_load(QEMUFile *f, Error **errp); + #endif
Provide the VMStateDescription precreate field to mark objects that must be loaded on the incoming side before devices have been created, because they provide properties that will be needed at creation time. They will be saved to and loaded from their own QEMUFile, via qemu_savevm_precreate_save and qemu_savevm_precreate_load, but these functions are not yet called in this patch. Allow them to be called before or after normal migration is active, when current_migration and current_incoming are not valid. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/migration/vmstate.h | 6 ++++ migration/savevm.c | 69 +++++++++++++++++++++++++++++++++++++++++---- migration/savevm.h | 3 ++ 3 files changed, 73 insertions(+), 5 deletions(-)