Re: [PATCH v2 04/11] multifd: Count the number of bytes sent correctly

[Juan Quintela]

Chuang Xu <xuchuangxclwt@bytedance.com> wrote:
> Hi,Juan,
>
> On 2023/1/30 下午4:09, 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 | 6 ++++--
>>   2 files changed, 6 insertions(+), 2 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 61cafe4c76..cd26b2fda9 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,7 +428,8 @@ 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;
>> +    uint64_t transferred = p->sent_bytes;
>> +    p->sent_bytes = 0;
>>       qemu_file_acct_rate_limit(f, transferred);
>>       qemu_mutex_unlock(&p->mutex);
>>       stat64_add(&ram_atomic_counters.multifd_bytes, transferred);
>> @@ -719,6 +719,8 @@ static void *multifd_send_thread(void *opaque)
>>               }
>>
>>               qemu_mutex_lock(&p->mutex);
>> +            p->sent_bytes += p->packet_len;
>> +            p->sent_bytes += p->next_packet_size;
>
> Consider a scenario where some normal pages are transmitted in the first 
> round,
> followed by several consecutive rounds of zero pages. When zero pages
> are transmitted,
> next_packet_size of first round is still incorrectly added to
> sent_bytes. If we set a rate
> limiting for dirty page transmission, the transmission performance of
> multi zero check
> will degrade.
>
> Maybe we should set next_packet_size to 0 in multifd_send_pages()?

See my series of migration atomic counters O:-)

You are right with your comments, that is the reason why it took me so
many patches to fix it properly.

After the last serie on the list that set_bytes variable don't exist
anymore and I just do (with atomic operations):

multifd_bytes += size_of_write_just_done;

And no more sheanigans.

Thanks, Juan.