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

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(-)

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()?

>               p->pending_job--;
>               qemu_mutex_unlock(&p->mutex);
>

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.