Leonardo Brás <leob...@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 <quint...@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 <leob...@redhat.com>
Thanks, Juan.
