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

[Leonardo Bras Soares Passos <leobras@redhat.com>]

On Fri, May 26, 2023 at 5:24 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +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.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/multifd.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index aabf9b6d98..0bf5958a9c 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;
> >>  }
> >
> > Humm, those are atomic ops, right?
> >
> > You think we could have 'multifd_bytes' and 'transferred' in the same cacheline,
> > to avoid 2 cacheline bounces?
>
> Don't matter on next series.
>
> mig_stats.transferred is dropped.
>
> And transferred becomes:
>
> qemu_file_transferred + multifd_bytes + rdma_bytes.
>
> So everytime that we do a write, we only update one counter.

That's even better :)

Thanks!

>
> > Well, it's unrelated to this patchset, so:
> >
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
>