Re: [PATCH v7 05/13] multifd: Count the number of bytes sent correctly

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) 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 71f49b4063..8a45dda58c 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 166246b9b7..eef47c274f 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_update_transfer(f, transferred);
> -    ram_counters.multifd_bytes += transferred;
> -    ram_counters.transferred += transferred;
> +    ram_transferred_add(p->sent_bytes);
> +    ram_counters.multifd_bytes += p->sent_bytes;
> +    qemu_file_update_transfer(f, p->sent_bytes);

Note that got renamed in bc698c367d6fac15454ee3ff6bb168e43c151465
but other than that


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +    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_update_transfer(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);
>  
> @@ -712,6 +708,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;
>              p->pending_job--;
>              qemu_mutex_unlock(&p->mutex);
>  
> -- 
> 2.35.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK