Re: [PATCH v2 16/16] migration/multifd: Compute transferred bytes correctly

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> In the past, we had to put the in the main thread all the operations
>> related with sizes due to qemu_file not beeing thread safe.  As now
>> all counters are atomic, we can update the counters just after the
>> do the write.  As an aditional bonus, we are able to use the right
>> value for the compression methods.  Right now we were assuming that
>> there were no compression at all.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/multifd.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index aabf9b6d98..0bf5958a9c 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods 
>> *ops)
>>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>>  {
>>      MultiFDInit_t msg = {};
>> +    size_t size = sizeof(msg);
>>      int ret;
>>  
>>      msg.magic = cpu_to_be32(MULTIFD_MAGIC);
>> @@ -182,10 +183,12 @@ static int 
>> multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>>      msg.id = p->id;
>>      memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
>>  
>> -    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
>> +    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
>>      if (ret != 0) {
>>          return -1;
>>      }
>> +    stat64_add(&mig_stats.multifd_bytes, size);
>> +    stat64_add(&mig_stats.transferred, size);
>>      return 0;
>>  }
>
> Humm, those are atomic ops, right?
>
> You think we could have 'multifd_bytes' and 'transferred' in the same 
> cacheline,
> to avoid 2 cacheline bounces?

Don't matter on next series.

mig_stats.transferred is dropped.

And transferred becomes:

qemu_file_transferred + multifd_bytes + rdma_bytes.

So everytime that we do a write, we only update one counter.

> Well, it's unrelated to this patchset, so:
>
> Reviewed-by: Leonardo Bras <leobras@redhat.com>