On Fri, May 26, 2023 at 5:04 AM Juan Quintela <quint...@redhat.com> wrote:
>
> Leonardo Brás <leob...@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 <quint...@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.
>
