| Message ID | 20230112164403.105085-2-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-mem: Handle preallocation with migration | expand |
* David Hildenbrand (david@redhat.com) wrote: > Let's move more code into vmstate_save(), reducing code duplication and > preparing for reuse of vmstate_save() in qemu_savevm_state_setup(). We > have to move vmstate_save() to make the compiler happy. > > We'll now also trace from qemu_save_device_state(). Mostly OK, but.. > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > migration/savevm.c | 79 ++++++++++++++++++++++------------------------ Doesn't this also need to upate trace-events? Dave > 1 file changed, 37 insertions(+), 42 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index a0cdb714f7..d8830297e4 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -897,17 +897,6 @@ static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, > } > } > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se, > - JSONWriter *vmdesc) > -{ > - trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)"); > - if (!se->vmsd) { > - vmstate_save_old_style(f, se, vmdesc); > - return 0; > - } > - return vmstate_save_state(f, se->vmsd, se->opaque, vmdesc); > -} > - > /* > * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL) > */ > @@ -941,6 +930,43 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) > } > } > > +static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc) > +{ > + int ret; > + > + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { > + return 0; > + } > + if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { > + trace_savevm_section_skip(se->idstr, se->section_id); > + return 0; > + } > + > + trace_savevm_section_start(se->idstr, se->section_id); > + save_section_header(f, se, QEMU_VM_SECTION_FULL); > + if (vmdesc) { > + json_writer_start_object(vmdesc, NULL); > + json_writer_str(vmdesc, "name", se->idstr); > + json_writer_int64(vmdesc, "instance_id", se->instance_id); > + } > + > + trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)"); > + if (!se->vmsd) { > + vmstate_save_old_style(f, se, vmdesc); > + } else { > + ret = vmstate_save_state(f, se->vmsd, se->opaque, vmdesc); > + if (ret) { > + return ret; > + } > + } > + > + trace_savevm_section_end(se->idstr, se->section_id, 0); > + save_section_footer(f, se); > + if (vmdesc) { > + json_writer_end_object(vmdesc); > + } > + return 0; > +} > /** > * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the > * command and associated data. > @@ -1374,31 +1400,11 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, > json_writer_int64(vmdesc, "page_size", qemu_target_page_size()); > json_writer_start_array(vmdesc, "devices"); > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > - > - if ((!se->ops || !se->ops->save_state) && !se->vmsd) { > - continue; > - } > - if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { > - trace_savevm_section_skip(se->idstr, se->section_id); > - continue; > - } > - > - trace_savevm_section_start(se->idstr, se->section_id); > - > - json_writer_start_object(vmdesc, NULL); > - json_writer_str(vmdesc, "name", se->idstr); > - json_writer_int64(vmdesc, "instance_id", se->instance_id); > - > - save_section_header(f, se, QEMU_VM_SECTION_FULL); > ret = vmstate_save(f, se, vmdesc); > if (ret) { > qemu_file_set_error(f, ret); > return ret; > } > - trace_savevm_section_end(se->idstr, se->section_id, 0); > - save_section_footer(f, se); > - > - json_writer_end_object(vmdesc); > } > > if (inactivate_disks) { > @@ -1594,21 +1600,10 @@ int qemu_save_device_state(QEMUFile *f) > if (se->is_ram) { > continue; > } > - if ((!se->ops || !se->ops->save_state) && !se->vmsd) { > - continue; > - } > - if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { > - continue; > - } > - > - save_section_header(f, se, QEMU_VM_SECTION_FULL); > - > ret = vmstate_save(f, se, NULL); > if (ret) { > return ret; > } > - > - save_section_footer(f, se); > } > > qemu_put_byte(f, QEMU_VM_EOF); > -- > 2.39.0 >
On 12.01.23 17:58, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> Let's move more code into vmstate_save(), reducing code duplication and >> preparing for reuse of vmstate_save() in qemu_savevm_state_setup(). We >> have to move vmstate_save() to make the compiler happy. >> >> We'll now also trace from qemu_save_device_state(). > > Mostly OK, but.. > >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> migration/savevm.c | 79 ++++++++++++++++++++++------------------------ > > Doesn't this also need to upate trace-events? The existing trace events from qemu_savevm_state_complete_precopy_non_iterable() are simply moved to vmstate_save(), so qemu_save_device_state() will implicitly use them. So no update should be needed (no new events), or am I missing something? Thanks!
* David Hildenbrand (david@redhat.com) wrote: > On 12.01.23 17:58, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > > > Let's move more code into vmstate_save(), reducing code duplication and > > > preparing for reuse of vmstate_save() in qemu_savevm_state_setup(). We > > > have to move vmstate_save() to make the compiler happy. > > > > > > We'll now also trace from qemu_save_device_state(). > > > > Mostly OK, but.. > > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > --- > > > migration/savevm.c | 79 ++++++++++++++++++++++------------------------ > > > > Doesn't this also need to upate trace-events? > > The existing trace events from > qemu_savevm_state_complete_precopy_non_iterable() are simply moved to > vmstate_save(), so qemu_save_device_state() will implicitly use them. > > So no update should be needed (no new events), or am I missing something? Aren't you losing the trace_savevm_state_setup() trace? Dave > Thanks! > > > -- > Thanks, > > David / dhildenb >
On 12.01.23 19:36, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 12.01.23 17:58, Dr. David Alan Gilbert wrote: >>> * David Hildenbrand (david@redhat.com) wrote: >>>> Let's move more code into vmstate_save(), reducing code duplication and >>>> preparing for reuse of vmstate_save() in qemu_savevm_state_setup(). We >>>> have to move vmstate_save() to make the compiler happy. >>>> >>>> We'll now also trace from qemu_save_device_state(). >>> >>> Mostly OK, but.. >>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> migration/savevm.c | 79 ++++++++++++++++++++++------------------------ >>> >>> Doesn't this also need to upate trace-events? >> >> The existing trace events from >> qemu_savevm_state_complete_precopy_non_iterable() are simply moved to >> vmstate_save(), so qemu_save_device_state() will implicitly use them. >> >> So no update should be needed (no new events), or am I missing something? > > Aren't you losing the trace_savevm_state_setup() trace? trace_savevm_state_setup() is called from qemu_savevm_state_setup() before/after this change. Calling it from qemu_save_device_state() would be wrong: they skip the setup phase and don't call any save_setup() -- skipping all "se->is_ram".
diff --git a/migration/savevm.c b/migration/savevm.c index a0cdb714f7..d8830297e4 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -897,17 +897,6 @@ static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, } } -static int vmstate_save(QEMUFile *f, SaveStateEntry *se, - JSONWriter *vmdesc) -{ - trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)"); - if (!se->vmsd) { - vmstate_save_old_style(f, se, vmdesc); - return 0; - } - return vmstate_save_state(f, se->vmsd, se->opaque, vmdesc); -} - /* * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL) */ @@ -941,6 +930,43 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) } } +static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc) +{ + int ret; + + if ((!se->ops || !se->ops->save_state) && !se->vmsd) { + return 0; + } + if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { + trace_savevm_section_skip(se->idstr, se->section_id); + return 0; + } + + trace_savevm_section_start(se->idstr, se->section_id); + save_section_header(f, se, QEMU_VM_SECTION_FULL); + if (vmdesc) { + json_writer_start_object(vmdesc, NULL); + json_writer_str(vmdesc, "name", se->idstr); + json_writer_int64(vmdesc, "instance_id", se->instance_id); + } + + trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)"); + if (!se->vmsd) { + vmstate_save_old_style(f, se, vmdesc); + } else { + ret = vmstate_save_state(f, se->vmsd, se->opaque, vmdesc); + if (ret) { + return ret; + } + } + + trace_savevm_section_end(se->idstr, se->section_id, 0); + save_section_footer(f, se); + if (vmdesc) { + json_writer_end_object(vmdesc); + } + return 0; +} /** * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the * command and associated data. @@ -1374,31 +1400,11 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, json_writer_int64(vmdesc, "page_size", qemu_target_page_size()); json_writer_start_array(vmdesc, "devices"); QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { - - if ((!se->ops || !se->ops->save_state) && !se->vmsd) { - continue; - } - if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { - trace_savevm_section_skip(se->idstr, se->section_id); - continue; - } - - trace_savevm_section_start(se->idstr, se->section_id); - - json_writer_start_object(vmdesc, NULL); - json_writer_str(vmdesc, "name", se->idstr); - json_writer_int64(vmdesc, "instance_id", se->instance_id); - - save_section_header(f, se, QEMU_VM_SECTION_FULL); ret = vmstate_save(f, se, vmdesc); if (ret) { qemu_file_set_error(f, ret); return ret; } - trace_savevm_section_end(se->idstr, se->section_id, 0); - save_section_footer(f, se); - - json_writer_end_object(vmdesc); } if (inactivate_disks) { @@ -1594,21 +1600,10 @@ int qemu_save_device_state(QEMUFile *f) if (se->is_ram) { continue; } - if ((!se->ops || !se->ops->save_state) && !se->vmsd) { - continue; - } - if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) { - continue; - } - - save_section_header(f, se, QEMU_VM_SECTION_FULL); - ret = vmstate_save(f, se, NULL); if (ret) { return ret; } - - save_section_footer(f, se); } qemu_put_byte(f, QEMU_VM_EOF);
Let's move more code into vmstate_save(), reducing code duplication and preparing for reuse of vmstate_save() in qemu_savevm_state_setup(). We have to move vmstate_save() to make the compiler happy. We'll now also trace from qemu_save_device_state(). Signed-off-by: David Hildenbrand <david@redhat.com> --- migration/savevm.c | 79 ++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 42 deletions(-)