Re: [PATCH v7 04/12] multifd: Count the number of bytes sent correctly

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
>> Current code asumes that all pages are whole.  That is not true for
>> example for compression already.  Fix it for creating a new field
>> ->sent_bytes that includes it.
>> 
>> All ram_counters are used only from the migration thread, so we have
>> two options:
>> - put a mutex and fill everything when we sent it (not only
>> ram_counters, also qemu_file->xfer_bytes).
>> - Create a local variable that implements how much has been sent
>> through each channel.  And when we push another packet, we "add" the
>> previous stats.
>> 
>> I choose two due to less changes overall.  On the previous code we
>> increase transferred and then we sent.  Current code goes the other
>> way around.  It sents the data, and after the fact, it updates the
>> counters.  Notice that each channel can have a maximum of half a
>> megabyte of data without counting, so it is not very important.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/multifd.h |  2 ++
>>  migration/multifd.c | 14 ++++++--------
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index e2802a9ce2..36f899c56f 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -102,6 +102,8 @@ typedef struct {
>>      uint32_t flags;
>>      /* global number of generated multifd packets */
>>      uint64_t packet_num;
>> +    /* How many bytes have we sent on the last packet */
>> +    uint64_t sent_bytes;
>>      /* thread has work to do */
>>      int pending_job;
>>      /* array of pages to sent.
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index aa3808a6f4..e25b529235 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -394,7 +394,6 @@ static int multifd_send_pages(QEMUFile *f)
>>      static int next_channel;
>>      MultiFDSendParams *p = NULL; /* make happy gcc */
>>      MultiFDPages_t *pages = multifd_send_state->pages;
>> -    uint64_t transferred;
>>  
>>      if (qatomic_read(&multifd_send_state->exiting)) {
>>          return -1;
>> @@ -429,10 +428,10 @@ static int multifd_send_pages(QEMUFile *f)
>>      p->packet_num = multifd_send_state->packet_num++;
>>      multifd_send_state->pages = p->pages;
>>      p->pages = pages;
>> -    transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
>> -    qemu_file_acct_rate_limit(f, transferred);
>> -    ram_counters.multifd_bytes += transferred;
>> -    ram_counters.transferred += transferred;
>> +    ram_transferred_add(p->sent_bytes);
>> +    ram_counters.multifd_bytes += p->sent_bytes;
>
> I'm worndering if we could avoid having this last line by having
> ram_transferred_add() to include:
>
> if (migrate_use_multifd()) {
>     ram_counters.multifd_bytes += bytes;
> }
>
> But I am not sure if other usages from ram_transferred_add() could interfere.

I preffer not to, because ram_addr_ram() is also used for non multifd code.

> Double semicolon.

Fixed, thanks.

>> +            p->sent_bytes += p->next_packet_size;
>>              p->pending_job--;
>>              qemu_mutex_unlock(&p->mutex);
>>  
>
> IIUC, it changes how rate-limiting and ram counters perceive how many bytes 
> have
> been sent, by counting actual bytes instead of page multiples. This should
> reflect what have been actually sent (in terms of rate limiting).
>
> I'm wondering if having the ram_counters.transferred to reflect acutal bytes,
> instead of the number of pages * pagesize will cause any user (or management
> code) to be confuse in any way.

It shouldn't, because we already have things that don't sent that data
as multiples:
- any compression code
- xbzrle

so I think we are right here.

> Other than that:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Thanks, Juan.