| Message ID | 20230515195709.63843-3-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
Juan Quintela <quintela@redhat.com> writes: > We forget several places to add to trasferred amount of data. With "transferred". > this fixes I get: > > qemu_file_transferred() + multifd_bytes == transferred > > The only place whrer this is not true is during devices sending. But "where" > going all through the full tree searching for devices that use > QEMUFile directly is a bit too much. > > Multifd, precopy and xbzrle work as expected. Postocpy still misses 35 > bytes, but searching for them is getting complicated, so I stop here. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/ram.c | 14 ++++++++++++++ > migration/savevm.c | 19 +++++++++++++++++-- > migration/vmstate.c | 3 +++ > migration/meson.build | 2 +- > 4 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index f69d8d42b0..fd5a8db0f8 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file, > > g_free(le_bitmap); > > + stat64_add(&mig_stats.transferred, 8 + size + 8); Push this up after the qemu_fflush() call? > if (qemu_file_get_error(file)) { > return qemu_file_get_error(file); > } > @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > return ret; > } > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + stat64_add(&mig_stats.transferred, 8); > qemu_fflush(f); > } > /* > @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > RAMState **rsp = opaque; > RAMBlock *block; > int ret; > + size_t size = 0; > > if (compress_threads_save_setup()) { > return -1; > @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > qemu_put_be64(f, ram_bytes_total_with_ignored() > | RAM_SAVE_FLAG_MEM_SIZE); > > + size += 8; Move the blank line down. > RAMBLOCK_FOREACH_MIGRATABLE(block) { > qemu_put_byte(f, strlen(block->idstr)); > qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); > qemu_put_be64(f, block->used_length); > + size += 1 + strlen(block->idstr) + 8; > if (migrate_postcopy_ram() && block->page_size != > qemu_host_page_size) { > qemu_put_be64(f, block->page_size); > + size += 8; > } > if (migrate_ignore_shared()) { > qemu_put_be64(f, block->mr->addr); > + size += 8; > } > } > } > @@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > > if (!migrate_multifd_flush_after_each_section()) { > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + size += 8; > } > > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > + size += 8; > qemu_fflush(f); > > + stat64_add(&mig_stats.transferred, size); > return 0; > } > > @@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > RAMState **temp = opaque; > RAMState *rs = *temp; > int ret = 0; > + size_t size = 0; > > rs->last_stage = !migration_in_colo_state(); > > @@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > > if (!migrate_multifd_flush_after_each_section()) { > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + size += 8; > } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > + size += 8; > + stat64_add(&mig_stats.transferred, size); This is after qemu_fflush() in the other cases. > qemu_fflush(f); > > return 0; > diff --git a/migration/savevm.c b/migration/savevm.c > index e33788343a..c7af9050c2 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > qemu_put_byte(f, section_type); > qemu_put_be32(f, se->section_id); > > + size_t size = 1 + 4; Move the blank line down. > if (section_type == QEMU_VM_SECTION_FULL || > section_type == QEMU_VM_SECTION_START) { > /* ID string */ > @@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > + size += 1 + len + 4 + 4; > } > + stat64_add(&mig_stats.transferred, size); > } > > /* > @@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) > if (migrate_get_current()->send_section_footer) { > qemu_put_byte(f, QEMU_VM_SECTION_FOOTER); > qemu_put_be32(f, se->section_id); > + stat64_add(&mig_stats.transferred, 1 + 4); > } > } > > @@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f, > qemu_put_be16(f, (uint16_t)command); > qemu_put_be16(f, len); > qemu_put_buffer(f, data, len); > + stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len); > qemu_fflush(f); > } > > @@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f) > trace_savevm_state_header(); > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > qemu_put_be32(f, QEMU_VM_FILE_VERSION); > - > + size_t size = 4 + 4; Move the blank line down. > if (migrate_get_current()->send_configuration) { > qemu_put_byte(f, QEMU_VM_CONFIGURATION); > + size += 1; > vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0); > } > + stat64_add(&mig_stats.transferred, size); > } > > bool qemu_savevm_state_guest_unplug_pending(void) > @@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > { > SaveStateEntry *se; > int ret; > + size_t size = 0; > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (!se->ops || !se->ops->save_live_complete_postcopy) { > @@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > /* Section type */ > qemu_put_byte(f, QEMU_VM_SECTION_END); > qemu_put_be32(f, se->section_id); > - > + size += 1 + 4; Move the blank line down. > ret = se->ops->save_live_complete_postcopy(f, se->opaque); > trace_savevm_section_end(se->idstr, se->section_id, ret); > save_section_footer(f, se); > @@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > } > > qemu_put_byte(f, QEMU_VM_EOF); > + size += 1; > + stat64_add(&mig_stats.transferred, size); > qemu_fflush(f); > } > > @@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, > if (!in_postcopy) { > /* Postcopy stream will still be going */ > qemu_put_byte(f, QEMU_VM_EOF); > + stat64_add(&mig_stats.transferred, 1); > } > > json_writer_end_array(vmdesc); > @@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f) > /* save QEMU_VM_SECTION_END section */ > qemu_savevm_state_complete_precopy(f, true, false); > qemu_put_byte(f, QEMU_VM_EOF); > + stat64_add(&mig_stats.transferred, 1); > } > > int qemu_save_device_state(QEMUFile *f) > { > SaveStateEntry *se; > + size_t size = 0; > > if (!migration_in_colo_state()) { > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > qemu_put_be32(f, QEMU_VM_FILE_VERSION); > + size = 4 + 4; > } > cpu_synchronize_all_states(); > > @@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f) > > qemu_put_byte(f, QEMU_VM_EOF); > > + stat64_add(&mig_stats.transferred, size + 1); Move the blank line down. In other cases the pattern has been to update 'size' and then call stat64_add() - make this one the same? > return qemu_file_get_error(f); > } > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index af01d54b6f..ee3b70a6a8 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -12,6 +12,7 @@ > > #include "qemu/osdep.h" > #include "migration.h" > +#include "migration-stats.h" > #include "migration/vmstate.h" > #include "savevm.h" > #include "qapi/qmp/json-writer.h" > @@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, > written_bytes = qemu_file_transferred_fast(f) - old_offset; > vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i); > > + stat64_add(&mig_stats.transferred, written_bytes); Move the blank line down. > /* Compressed arrays only care about the first element */ > if (vmdesc_loop && vmsd_can_compress(field)) { > vmdesc_loop = NULL; > @@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > qemu_put_byte(f, len); > qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len); > qemu_put_be32(f, vmsdsub->version_id); > + stat64_add(&mig_stats.transferred, 1 + 1 + len + 4); > ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc); > if (ret) { > return ret; > diff --git a/migration/meson.build b/migration/meson.build > index dc8b1daef5..b3d0c537c8 100644 > --- a/migration/meson.build > +++ b/migration/meson.build > @@ -1,5 +1,6 @@ > # Files needed by unit tests > migration_files = files( > + 'migration-stats.c', > 'page_cache.c', > 'xbzrle.c', > 'vmstate-types.c', > @@ -18,7 +19,6 @@ softmmu_ss.add(files( > 'fd.c', > 'global_state.c', > 'migration-hmp-cmds.c', > - 'migration-stats.c', > 'migration.c', > 'multifd.c', > 'multifd-zlib.c', > -- > 2.40.1
On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote: > We forget several places to add to trasferred amount of data. With > this fixes I get: > > qemu_file_transferred() + multifd_bytes == transferred > > The only place whrer this is not true is during devices sending. But > going all through the full tree searching for devices that use > QEMUFile directly is a bit too much. > > Multifd, precopy and xbzrle work as expected. Postocpy still misses 35 > bytes, but searching for them is getting complicated, so I stop here. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/ram.c | 14 ++++++++++++++ > migration/savevm.c | 19 +++++++++++++++++-- > migration/vmstate.c | 3 +++ > migration/meson.build | 2 +- > 4 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index f69d8d42b0..fd5a8db0f8 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file, > > g_free(le_bitmap); > > + stat64_add(&mig_stats.transferred, 8 + size + 8); > if (qemu_file_get_error(file)) { > return qemu_file_get_error(file); > } > @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > return ret; > } > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + stat64_add(&mig_stats.transferred, 8); > qemu_fflush(f); > } > /* > @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > RAMState **rsp = opaque; > RAMBlock *block; > int ret; > + size_t size = 0; > > if (compress_threads_save_setup()) { > return -1; > @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > qemu_put_be64(f, ram_bytes_total_with_ignored() > | RAM_SAVE_FLAG_MEM_SIZE); > > + size += 8; > RAMBLOCK_FOREACH_MIGRATABLE(block) { > qemu_put_byte(f, strlen(block->idstr)); > qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); > qemu_put_be64(f, block->used_length); > + size += 1 + strlen(block->idstr) + 8; I was thinking some of them would look better with sizeof()s instead of given literal number, such as: size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length); Maybe too much? > if (migrate_postcopy_ram() && block->page_size != > qemu_host_page_size) { > qemu_put_be64(f, block->page_size); > + size += 8; > } > if (migrate_ignore_shared()) { > qemu_put_be64(f, block->mr->addr); > + size += 8; > } > } > } > @@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > > if (!migrate_multifd_flush_after_each_section()) { > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + size += 8; sizeof(uint64_t) here is probably too much. Maybe, it would be nice to have qemu_put_* to return the value, and in this case: size += qemu_put_be64(...) What do you think? Anyway, Reviewed-by: Leonardo Bras <leobras@redhat.com> > } > > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > + size += 8; > qemu_fflush(f); > > + stat64_add(&mig_stats.transferred, size); > return 0; > } > > @@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > RAMState **temp = opaque; > RAMState *rs = *temp; > int ret = 0; > + size_t size = 0; > > rs->last_stage = !migration_in_colo_state(); > > @@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > > if (!migrate_multifd_flush_after_each_section()) { > qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > + size += 8; > } > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > + size += 8; > + stat64_add(&mig_stats.transferred, size); > qemu_fflush(f); > > return 0; > diff --git a/migration/savevm.c b/migration/savevm.c > index e33788343a..c7af9050c2 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > qemu_put_byte(f, section_type); > qemu_put_be32(f, se->section_id); > > + size_t size = 1 + 4; > if (section_type == QEMU_VM_SECTION_FULL || > section_type == QEMU_VM_SECTION_START) { > /* ID string */ > @@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, > > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > + size += 1 + len + 4 + 4; > } > + stat64_add(&mig_stats.transferred, size); > } > > /* > @@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) > if (migrate_get_current()->send_section_footer) { > qemu_put_byte(f, QEMU_VM_SECTION_FOOTER); > qemu_put_be32(f, se->section_id); > + stat64_add(&mig_stats.transferred, 1 + 4); > } > } > > @@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f, > qemu_put_be16(f, (uint16_t)command); > qemu_put_be16(f, len); > qemu_put_buffer(f, data, len); > + stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len); > qemu_fflush(f); > } > > @@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f) > trace_savevm_state_header(); > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > qemu_put_be32(f, QEMU_VM_FILE_VERSION); > - > + size_t size = 4 + 4; > if (migrate_get_current()->send_configuration) { > qemu_put_byte(f, QEMU_VM_CONFIGURATION); > + size += 1; > vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0); > } > + stat64_add(&mig_stats.transferred, size); > } > > bool qemu_savevm_state_guest_unplug_pending(void) > @@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > { > SaveStateEntry *se; > int ret; > + size_t size = 0; > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (!se->ops || !se->ops->save_live_complete_postcopy) { > @@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > /* Section type */ > qemu_put_byte(f, QEMU_VM_SECTION_END); > qemu_put_be32(f, se->section_id); > - > + size += 1 + 4; > ret = se->ops->save_live_complete_postcopy(f, se->opaque); > trace_savevm_section_end(se->idstr, se->section_id, ret); > save_section_footer(f, se); > @@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) > } > > qemu_put_byte(f, QEMU_VM_EOF); > + size += 1; > + stat64_add(&mig_stats.transferred, size); > qemu_fflush(f); > } > > @@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, > if (!in_postcopy) { > /* Postcopy stream will still be going */ > qemu_put_byte(f, QEMU_VM_EOF); > + stat64_add(&mig_stats.transferred, 1); > } > > json_writer_end_array(vmdesc); > @@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f) > /* save QEMU_VM_SECTION_END section */ > qemu_savevm_state_complete_precopy(f, true, false); > qemu_put_byte(f, QEMU_VM_EOF); > + stat64_add(&mig_stats.transferred, 1); > } > > int qemu_save_device_state(QEMUFile *f) > { > SaveStateEntry *se; > + size_t size = 0; > > if (!migration_in_colo_state()) { > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > qemu_put_be32(f, QEMU_VM_FILE_VERSION); > + size = 4 + 4; > } > cpu_synchronize_all_states(); > > @@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f) > > qemu_put_byte(f, QEMU_VM_EOF); > > + stat64_add(&mig_stats.transferred, size + 1); > return qemu_file_get_error(f); > } > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index af01d54b6f..ee3b70a6a8 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -12,6 +12,7 @@ > > #include "qemu/osdep.h" > #include "migration.h" > +#include "migration-stats.h" > #include "migration/vmstate.h" > #include "savevm.h" > #include "qapi/qmp/json-writer.h" > @@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, > written_bytes = qemu_file_transferred_fast(f) - old_offset; > vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i); > > + stat64_add(&mig_stats.transferred, written_bytes); > /* Compressed arrays only care about the first element */ > if (vmdesc_loop && vmsd_can_compress(field)) { > vmdesc_loop = NULL; > @@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > qemu_put_byte(f, len); > qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len); > qemu_put_be32(f, vmsdsub->version_id); > + stat64_add(&mig_stats.transferred, 1 + 1 + len + 4); > ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc); > if (ret) { > return ret; > diff --git a/migration/meson.build b/migration/meson.build > index dc8b1daef5..b3d0c537c8 100644 > --- a/migration/meson.build > +++ b/migration/meson.build > @@ -1,5 +1,6 @@ > # Files needed by unit tests > migration_files = files( > + 'migration-stats.c', > 'page_cache.c', > 'xbzrle.c', > 'vmstate-types.c', > @@ -18,7 +19,6 @@ softmmu_ss.add(files( > 'fd.c', > 'global_state.c', > 'migration-hmp-cmds.c', > - 'migration-stats.c', > 'migration.c', > 'multifd.c', > 'multifd-zlib.c',
Leonardo Brás <leobras@redhat.com> wrote: > On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote: >> We forget several places to add to trasferred amount of data. With >> this fixes I get: >> >> qemu_file_transferred() + multifd_bytes == transferred >> >> The only place whrer this is not true is during devices sending. But >> going all through the full tree searching for devices that use >> QEMUFile directly is a bit too much. >> >> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35 >> bytes, but searching for them is getting complicated, so I stop here. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/ram.c | 14 ++++++++++++++ >> migration/savevm.c | 19 +++++++++++++++++-- >> migration/vmstate.c | 3 +++ >> migration/meson.build | 2 +- >> 4 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index f69d8d42b0..fd5a8db0f8 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file, >> >> g_free(le_bitmap); >> >> + stat64_add(&mig_stats.transferred, 8 + size + 8); >> if (qemu_file_get_error(file)) { >> return qemu_file_get_error(file); >> } >> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) >> return ret; >> } >> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); >> + stat64_add(&mig_stats.transferred, 8); >> qemu_fflush(f); >> } >> /* >> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> RAMState **rsp = opaque; >> RAMBlock *block; >> int ret; >> + size_t size = 0; >> >> if (compress_threads_save_setup()) { >> return -1; >> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> qemu_put_be64(f, ram_bytes_total_with_ignored() >> | RAM_SAVE_FLAG_MEM_SIZE); >> >> + size += 8; >> RAMBLOCK_FOREACH_MIGRATABLE(block) { >> qemu_put_byte(f, strlen(block->idstr)); >> qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); >> qemu_put_be64(f, block->used_length); >> + size += 1 + strlen(block->idstr) + 8; > > I was thinking some of them would look better with sizeof()s instead of given > literal number, such as: > > size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length); > > Maybe too much? I dropped this patch for two reasons: - reviewers gave me a bad time with it O:-) - it was there only so if anyone was meassuring that new counters are the same that old counters. But as I have already checked that, we don't need it. I drop it on the next round that I send. > Maybe, it would be nice to have qemu_put_* to return the value, and in this > case: > > size += qemu_put_be64(...) > > What do you think? Even more important than that is to return an error value, but that is a very long project. See on my next series that qemu_fflush() return errors, so code gets simplifed: qemu_fflush(file); if (qemu_file_get_error(file)) { handle error; } to: qemu_fflush(file); if (qemu_file_get_error(file)) { handle error; } We need to do basically all qemu_put_*() and qemu_get_*() functions, but it is a step on the right direction. Later, Juan.
On Fri, May 26, 2023 at 5:04 AM Juan Quintela <quintela@redhat.com> wrote: > > Leonardo Brás <leobras@redhat.com> wrote: > > On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote: > >> We forget several places to add to trasferred amount of data. With > >> this fixes I get: > >> > >> qemu_file_transferred() + multifd_bytes == transferred > >> > >> The only place whrer this is not true is during devices sending. But > >> going all through the full tree searching for devices that use > >> QEMUFile directly is a bit too much. > >> > >> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35 > >> bytes, but searching for them is getting complicated, so I stop here. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/ram.c | 14 ++++++++++++++ > >> migration/savevm.c | 19 +++++++++++++++++-- > >> migration/vmstate.c | 3 +++ > >> migration/meson.build | 2 +- > >> 4 files changed, 35 insertions(+), 3 deletions(-) > >> > >> diff --git a/migration/ram.c b/migration/ram.c > >> index f69d8d42b0..fd5a8db0f8 100644 > >> --- a/migration/ram.c > >> +++ b/migration/ram.c > >> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file, > >> > >> g_free(le_bitmap); > >> > >> + stat64_add(&mig_stats.transferred, 8 + size + 8); > >> if (qemu_file_get_error(file)) { > >> return qemu_file_get_error(file); > >> } > >> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > >> return ret; > >> } > >> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); > >> + stat64_add(&mig_stats.transferred, 8); > >> qemu_fflush(f); > >> } > >> /* > >> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > >> RAMState **rsp = opaque; > >> RAMBlock *block; > >> int ret; > >> + size_t size = 0; > >> > >> if (compress_threads_save_setup()) { > >> return -1; > >> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > >> qemu_put_be64(f, ram_bytes_total_with_ignored() > >> | RAM_SAVE_FLAG_MEM_SIZE); > >> > >> + size += 8; > >> RAMBLOCK_FOREACH_MIGRATABLE(block) { > >> qemu_put_byte(f, strlen(block->idstr)); > >> qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); > >> qemu_put_be64(f, block->used_length); > >> + size += 1 + strlen(block->idstr) + 8; > > > > I was thinking some of them would look better with sizeof()s instead of given > > literal number, such as: > > > > size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length); > > > > Maybe too much? > > I dropped this patch for two reasons: > > - reviewers gave me a bad time with it O:-) > - it was there only so if anyone was meassuring that new counters are > the same that old counters. > > But as I have already checked that, we don't need it. > > I drop it on the next round that I send. > > Maybe, it would be nice to have qemu_put_* to return the value, and in this > > case: > > > > size += qemu_put_be64(...) > > > > What do you think? > > Even more important than that is to return an error value, but that > is a very long project. > > See on my next series that qemu_fflush() return errors, so code gets > simplifed: > > qemu_fflush(file); > if (qemu_file_get_error(file)) { > handle error; > } > > to: > > qemu_fflush(file); > if (qemu_file_get_error(file)) { > handle error; > } > They look the same to me, what changed? > We need to do basically all qemu_put_*() and qemu_get_*() functions, but > it is a step on the right direction. > > Later, Juan. >
Leonardo Bras Soares Passos <leobras@redhat.com> wrote: > On Fri, May 26, 2023 at 5:04 AM Juan Quintela <quintela@redhat.com> wrote: >> > Maybe too much? >> >> I dropped this patch for two reasons: >> >> - reviewers gave me a bad time with it O:-) >> - it was there only so if anyone was meassuring that new counters are >> the same that old counters. >> >> But as I have already checked that, we don't need it. >> >> I drop it on the next round that I send. >> > Maybe, it would be nice to have qemu_put_* to return the value, and in this >> > case: >> > >> > size += qemu_put_be64(...) >> > >> > What do you think? >> >> Even more important than that is to return an error value, but that >> is a very long project. >> >> See on my next series that qemu_fflush() return errors, so code gets >> simplifed: >> >> qemu_fflush(file); >> if (qemu_file_get_error(file)) { >> handle error; >> } >> >> to: >> >> qemu_fflush(file); >> if (qemu_file_get_error(file)) { >> handle error; >> } >> > > They look the same to me, what changed? I did copy paste without changing: if (qemu_fflush(file)) { handle error; } >> We need to do basically all qemu_put_*() and qemu_get_*() functions, but >> it is a step on the right direction. >> >> Later, Juan. >>
diff --git a/migration/ram.c b/migration/ram.c index f69d8d42b0..fd5a8db0f8 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file, g_free(le_bitmap); + stat64_add(&mig_stats.transferred, 8 + size + 8); if (qemu_file_get_error(file)) { return qemu_file_get_error(file); } @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) return ret; } qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); + stat64_add(&mig_stats.transferred, 8); qemu_fflush(f); } /* @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) RAMState **rsp = opaque; RAMBlock *block; int ret; + size_t size = 0; if (compress_threads_save_setup()) { return -1; @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque) qemu_put_be64(f, ram_bytes_total_with_ignored() | RAM_SAVE_FLAG_MEM_SIZE); + size += 8; RAMBLOCK_FOREACH_MIGRATABLE(block) { qemu_put_byte(f, strlen(block->idstr)); qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr)); qemu_put_be64(f, block->used_length); + size += 1 + strlen(block->idstr) + 8; if (migrate_postcopy_ram() && block->page_size != qemu_host_page_size) { qemu_put_be64(f, block->page_size); + size += 8; } if (migrate_ignore_shared()) { qemu_put_be64(f, block->mr->addr); + size += 8; } } } @@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque) if (!migrate_multifd_flush_after_each_section()) { qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); + size += 8; } qemu_put_be64(f, RAM_SAVE_FLAG_EOS); + size += 8; qemu_fflush(f); + stat64_add(&mig_stats.transferred, size); return 0; } @@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) RAMState **temp = opaque; RAMState *rs = *temp; int ret = 0; + size_t size = 0; rs->last_stage = !migration_in_colo_state(); @@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque) if (!migrate_multifd_flush_after_each_section()) { qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH); + size += 8; } qemu_put_be64(f, RAM_SAVE_FLAG_EOS); + size += 8; + stat64_add(&mig_stats.transferred, size); qemu_fflush(f); return 0; diff --git a/migration/savevm.c b/migration/savevm.c index e33788343a..c7af9050c2 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, qemu_put_byte(f, section_type); qemu_put_be32(f, se->section_id); + size_t size = 1 + 4; if (section_type == QEMU_VM_SECTION_FULL || section_type == QEMU_VM_SECTION_START) { /* ID string */ @@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se, qemu_put_be32(f, se->instance_id); qemu_put_be32(f, se->version_id); + size += 1 + len + 4 + 4; } + stat64_add(&mig_stats.transferred, size); } /* @@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se) if (migrate_get_current()->send_section_footer) { qemu_put_byte(f, QEMU_VM_SECTION_FOOTER); qemu_put_be32(f, se->section_id); + stat64_add(&mig_stats.transferred, 1 + 4); } } @@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f, qemu_put_be16(f, (uint16_t)command); qemu_put_be16(f, len); qemu_put_buffer(f, data, len); + stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len); qemu_fflush(f); } @@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f) trace_savevm_state_header(); qemu_put_be32(f, QEMU_VM_FILE_MAGIC); qemu_put_be32(f, QEMU_VM_FILE_VERSION); - + size_t size = 4 + 4; if (migrate_get_current()->send_configuration) { qemu_put_byte(f, QEMU_VM_CONFIGURATION); + size += 1; vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0); } + stat64_add(&mig_stats.transferred, size); } bool qemu_savevm_state_guest_unplug_pending(void) @@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) { SaveStateEntry *se; int ret; + size_t size = 0; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (!se->ops || !se->ops->save_live_complete_postcopy) { @@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) /* Section type */ qemu_put_byte(f, QEMU_VM_SECTION_END); qemu_put_be32(f, se->section_id); - + size += 1 + 4; ret = se->ops->save_live_complete_postcopy(f, se->opaque); trace_savevm_section_end(se->idstr, se->section_id, ret); save_section_footer(f, se); @@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) } qemu_put_byte(f, QEMU_VM_EOF); + size += 1; + stat64_add(&mig_stats.transferred, size); qemu_fflush(f); } @@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, if (!in_postcopy) { /* Postcopy stream will still be going */ qemu_put_byte(f, QEMU_VM_EOF); + stat64_add(&mig_stats.transferred, 1); } json_writer_end_array(vmdesc); @@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f) /* save QEMU_VM_SECTION_END section */ qemu_savevm_state_complete_precopy(f, true, false); qemu_put_byte(f, QEMU_VM_EOF); + stat64_add(&mig_stats.transferred, 1); } int qemu_save_device_state(QEMUFile *f) { SaveStateEntry *se; + size_t size = 0; if (!migration_in_colo_state()) { qemu_put_be32(f, QEMU_VM_FILE_MAGIC); qemu_put_be32(f, QEMU_VM_FILE_VERSION); + size = 4 + 4; } cpu_synchronize_all_states(); @@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f) qemu_put_byte(f, QEMU_VM_EOF); + stat64_add(&mig_stats.transferred, size + 1); return qemu_file_get_error(f); } diff --git a/migration/vmstate.c b/migration/vmstate.c index af01d54b6f..ee3b70a6a8 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -12,6 +12,7 @@ #include "qemu/osdep.h" #include "migration.h" +#include "migration-stats.h" #include "migration/vmstate.h" #include "savevm.h" #include "qapi/qmp/json-writer.h" @@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, written_bytes = qemu_file_transferred_fast(f) - old_offset; vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i); + stat64_add(&mig_stats.transferred, written_bytes); /* Compressed arrays only care about the first element */ if (vmdesc_loop && vmsd_can_compress(field)) { vmdesc_loop = NULL; @@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, qemu_put_byte(f, len); qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len); qemu_put_be32(f, vmsdsub->version_id); + stat64_add(&mig_stats.transferred, 1 + 1 + len + 4); ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc); if (ret) { return ret; diff --git a/migration/meson.build b/migration/meson.build index dc8b1daef5..b3d0c537c8 100644 --- a/migration/meson.build +++ b/migration/meson.build @@ -1,5 +1,6 @@ # Files needed by unit tests migration_files = files( + 'migration-stats.c', 'page_cache.c', 'xbzrle.c', 'vmstate-types.c', @@ -18,7 +19,6 @@ softmmu_ss.add(files( 'fd.c', 'global_state.c', 'migration-hmp-cmds.c', - 'migration-stats.c', 'migration.c', 'multifd.c', 'multifd-zlib.c',
We forget several places to add to trasferred amount of data. With this fixes I get: qemu_file_transferred() + multifd_bytes == transferred The only place whrer this is not true is during devices sending. But going all through the full tree searching for devices that use QEMUFile directly is a bit too much. Multifd, precopy and xbzrle work as expected. Postocpy still misses 35 bytes, but searching for them is getting complicated, so I stop here. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/ram.c | 14 ++++++++++++++ migration/savevm.c | 19 +++++++++++++++++-- migration/vmstate.c | 3 +++ migration/meson.build | 2 +- 4 files changed, 35 insertions(+), 3 deletions(-)