Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[Leonardo Bras Soares Passos]

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.
>