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

[Chuang Xu <xuchuangxclwt@bytedance.com>]

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