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

[Juan Quintela]

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.