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;
}
We need to do basically all qemu_put_*() and qemu_get_*() functions, but
it is a step on the right direction.
Later, Juan.
