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

[Leonardo Brás]

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.


> +    qemu_file_acct_rate_limit(f, p->sent_bytes);
> +    p->sent_bytes = 0;
>      qemu_mutex_unlock(&p->mutex);
>      qemu_sem_post(&p->sem);
>  
> @@ -605,9 +604,6 @@ int multifd_send_sync_main(QEMUFile *f)
>          p->packet_num = multifd_send_state->packet_num++;
>          p->flags |= MULTIFD_FLAG_SYNC;
>          p->pending_job++;
> -        qemu_file_acct_rate_limit(f, p->packet_len);
> -        ram_counters.multifd_bytes += p->packet_len;
> -        ram_counters.transferred += p->packet_len;
>          qemu_mutex_unlock(&p->mutex);
>          qemu_sem_post(&p->sem);
>  
> @@ -714,6 +710,8 @@ static void *multifd_send_thread(void *opaque)
>              }
>  
>              qemu_mutex_lock(&p->mutex);
> +            p->sent_bytes += p->packet_len;;

Double semicolon.

> +            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.

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

Best regards, 
Leo