Re: [PATCH 21/21] migration/multifd: Compute transferred bytes correctly

[Peter Xu <peterx@redhat.com>]

On Mon, May 08, 2023 at 03:09:09PM +0200, Juan Quintela wrote:
> In the past, we had to put the in the main thread all the operations
> related with sizes due to qemu_file not beeing thread safe.  As now
> all counters are atomic, we can update the counters just after the
> do the write.  As an aditional bonus, we are able to use the right
> value for the compression methods.  Right now we were assuming that
> there were no compression at all.

Maybe worth mention that initial packet is also accounted after this.

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Two more trivial nits:

> ---
>  migration/multifd.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 9d2ade7abc..3a19d8e304 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>  {
>      MultiFDInit_t msg = {};
> +    size_t size = sizeof(msg);
>      int ret;
>  
>      msg.magic = cpu_to_be32(MULTIFD_MAGIC);
> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>      msg.id = p->id;
>      memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
>  
> -    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
> +    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
>      if (ret != 0) {
>          return -1;
>      }
> +    stat64_add(&mig_stats.multifd_bytes, size);
> +    stat64_add(&mig_stats.transferred, size);
>      return 0;
>  }
>  
> @@ -396,7 +399,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;
> @@ -431,10 +433,7 @@ 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_mutex_unlock(&p->mutex);
> -    stat64_add(&mig_stats.transferred, transferred);
> -    stat64_add(&mig_stats.multifd_bytes, transferred);
>      qemu_sem_post(&p->sem);
>  
>      return 1;
> @@ -716,6 +715,8 @@ static void *multifd_send_thread(void *opaque)
>                  if (ret != 0) {
>                      break;
>                  }
> +                stat64_add(&mig_stats.multifd_bytes, p->packet_len);
> +                stat64_add(&mig_stats.transferred, p->packet_len);
>              } else {
>                  /* Send header using the same writev call */
>                  p->iov[0].iov_len = p->packet_len;
> @@ -728,6 +729,8 @@ static void *multifd_send_thread(void *opaque)
>                  break;
>              }
>  
> +            stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
> +            stat64_add(&mig_stats.transferred, p->next_packet_size);

Two nits:

Maybe merge the two so half atomic operations?

Also maybe also worth having a inline helper for adding both multifd_bytes
and transferred?

With/without that, all look good:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

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

-- 
Peter Xu