This way we can read it from any thread.
I checked that it gives the same value than the current one.  We never
use to qemu_files at the same time.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 ++++
 migration/qemu-file.c       | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 2358caad63..b7795e7914 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -81,6 +81,10 @@ typedef struct {
      * Number of bytes sent during precopy stage.
      */
     Stat64 precopy_bytes;
+    /*
+     * Number of bytes transferred with QEMUFile.
+     */
+    Stat64 qemu_file_transferred;
     /*
      * Amount of transferred data at the start of current cycle.
      */
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index be3dab85cb..eb0497e532 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f)
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
             f->total_transferred += size;
+            stat64_add(&mig_stats.qemu_file_transferred, size);
         }
 
         qemu_iovec_release_ram(f);
@@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
 
 uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
-    uint64_t ret = f->total_transferred;
+    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;
 
     g_assert(qemu_file_is_writable(f));
@@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
 {
     g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
-    return f->total_transferred;
+    return stat64_get(&mig_stats.qemu_file_transferred);
 }
 
 void qemu_put_be16(QEMUFile *f, unsigned int v)
-- 
2.40.1

On Tue, May 30, 2023 at 08:39:26PM +0200, Juan Quintela wrote:
> This way we can read it from any thread.
> I checked that it gives the same value than the current one.  We never
> use to qemu_files at the same time.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

The follow up patch may be better to be squashed or it's very confusing..

Why do we need to convert mostly everything into atomics?  Is it modified
outside migration thread?

AFAIR atomic ops are still expensive, and will get more expensive on larger
hosts, IOW it'll be good for us to keep non-atomic when possible (and
that's why when I changed some counters in postcopy work I only changed the
limited set because then the rest are still accessed in 1 single thread and
keep running fast).

> ---
>  migration/migration-stats.h | 4 ++++
>  migration/qemu-file.c       | 5 +++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 2358caad63..b7795e7914 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -81,6 +81,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * Number of bytes transferred with QEMUFile.
> +     */
> +    Stat64 qemu_file_transferred;
>      /*
>       * Amount of transferred data at the start of current cycle.
>       */
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index be3dab85cb..eb0497e532 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f)
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
>              f->total_transferred += size;
> +            stat64_add(&mig_stats.qemu_file_transferred, size);
>          }
>  
>          qemu_iovec_release_ram(f);
> @@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
>  
>  uint64_t qemu_file_transferred_noflush(QEMUFile *f)
>  {
> -    uint64_t ret = f->total_transferred;
> +    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
>      int i;
>  
>      g_assert(qemu_file_is_writable(f));
> @@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
>  {
>      g_assert(qemu_file_is_writable(f));
>      qemu_fflush(f);
> -    return f->total_transferred;
> +    return stat64_get(&mig_stats.qemu_file_transferred);
>  }
>  
>  void qemu_put_be16(QEMUFile *f, unsigned int v)
> -- 
> 2.40.1
> 

-- 
Peter Xu

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2023 at 08:39:26PM +0200, Juan Quintela wrote:
>> This way we can read it from any thread.
>> I checked that it gives the same value than the current one.  We never
>> use to qemu_files at the same time.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> The follow up patch may be better to be squashed or it's very confusing..

As said before, there used to be a nice:

if (old_counter != new_counter)
   printf(old_counter and new counter)

> Why do we need to convert mostly everything into atomics?  Is it modified
> outside migration thread?

Because we have four users of this stuff:

- io thread: needs it for info migrate basically
- migration_thread: needs it to know when to stop.  qemu_file is
  supposed to only happen here.
- ... until someone called peter decided to create another qemu file for
  preempt.  Current code don't account for this.
- multifd bytes is a completelly diffrent story, that is why it use its
  own atomics.

After this series go in, basically we only update one atomic for each
write (ok two yet, downtime/precopy/postocpy_bytes are still not
integrated).

But there used to be three:
- transferred
- multifd_bytes (for multifd pages)
- downtime/-precopy/postocpy

And we are back to only one (now we use transferred or multifd, never both).

> AFAIR atomic ops are still expensive, and will get more expensive on larger
> hosts,

If you care for performance, you are using multifd.
one atomic every 64 pages is kind ok.

For rate limiting, we do it 10 times a second, and:
a - it shouldn't matter with so few times
b - it is not a regression, we were already updating some
c - correct vs fast any day

And the other big improvement is that after this series, you can use any
counter on any thread.  No need to contorsions to update the rate_limit
that we used to have.

> IOW it'll be good for us to keep non-atomic when possible (and
> that's why when I changed some counters in postcopy work I only changed the
> limited set because then the rest are still accessed in 1 single thread and
> keep running fast).

void ram_transferred_add(uint64_t bytes)
{
    if (runstate_is_running()) {
        stat64_add(&mig_stats.precopy_bytes, bytes);
    } else if (migration_in_postcopy()) {
        stat64_add(&mig_stats.postcopy_bytes, bytes);
    } else {
        stat64_add(&mig_stats.downtime_bytes, bytes);
    }
    stat64_add(&mig_stats.transferred, bytes);
}

That is used everywhere, and I am dropping this to a single atomic.

Later, Juan.