Re: [Qemu-devel] [PATCH 4/5] ram: Use MigrationStats for statistics

[Peter Xu]

On Tue, Jun 06, 2017 at 07:33:45PM +0200, Juan Quintela wrote:
> Peter Xu  wrote:
> > On Mon, Jun 05, 2017 at 01:34:45PM +0100, Dr. David Alan Gilbert wrote:
> >> * Juan Quintela (quint...@redhat.com) wrote:
> >> > RAM Statistics need to survive migration to make info migrate work, so we
> >> > need to store them outside of RAMState.  As we already have an struct
> >> > with those fields, just used them. (MigrationStats and XBZRLECacheStats).
> >> > 
> >> > Signed-off-by: Juan Quintela 
> >> 
> >> Hmm OK; this feels very much like it's the opposite of 180f61f from
> >> March; these variables keep moving around over the last couple of months
> >> - are they going to stay still now?
> >
> > O:-)
> >
> > Meanwhile, I don't know whether it'll be necessary to remove all the
> > functions like ram_bytes_transferred(), e.g., it would be just:
> >
> > uint64_t ram_bytes_transferred(void)
> > {
> > -return ram_state.bytes_transferred;
> > +return ram_counters.transferred;
> > }
> >
> > But I'm okay with either.
> 
> That value was only used for filling the statistics.  And we are filling
> a struct from another struct of the exact same type.  Going through an
> exported function looks stranger.
> 
> And as said in $commit, the idea was that creating a new counter was
> easy, right now you have to:
> 
> - add it to MigrationParam (still have to do this)
> - add it to MigrationParams (still have to do this)
> - create the field in MigrationStats or RAMState
> - create a function that exports it
> - add that function in ram.h to export it
> - add it on qmp_query (still have to do this)
> 
> So, we are moving from 6 steps to 3 steps.  I think we are much better
> now, no? O:-)

Hmm, okay!

(as long as we won't move these functions back one day :-)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/5] ram: Use MigrationStats for statistics

[Juan Quintela]

Peter Xu  wrote:
> On Mon, Jun 05, 2017 at 01:34:45PM +0100, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quint...@redhat.com) wrote:
>> > RAM Statistics need to survive migration to make info migrate work, so we
>> > need to store them outside of RAMState.  As we already have an struct
>> > with those fields, just used them. (MigrationStats and XBZRLECacheStats).
>> > 
>> > Signed-off-by: Juan Quintela 
>> 
>> Hmm OK; this feels very much like it's the opposite of 180f61f from
>> March; these variables keep moving around over the last couple of months
>> - are they going to stay still now?
>
> O:-)
>
> Meanwhile, I don't know whether it'll be necessary to remove all the
> functions like ram_bytes_transferred(), e.g., it would be just:
>
> uint64_t ram_bytes_transferred(void)
> {
> -return ram_state.bytes_transferred;
> +return ram_counters.transferred;
> }
>
> But I'm okay with either.

That value was only used for filling the statistics.  And we are filling
a struct from another struct of the exact same type.  Going through an
exported function looks stranger.

And as said in $commit, the idea was that creating a new counter was
easy, right now you have to:

- add it to MigrationParam (still have to do this)
- add it to MigrationParams (still have to do this)
- create the field in MigrationStats or RAMState
- create a function that exports it
- add that function in ram.h to export it
- add it on qmp_query (still have to do this)

So, we are moving from 6 steps to 3 steps.  I think we are much better
now, no? O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 4/5] ram: Use MigrationStats for statistics

[Peter Xu]

On Mon, Jun 05, 2017 at 01:34:45PM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
> > RAM Statistics need to survive migration to make info migrate work, so we
> > need to store them outside of RAMState.  As we already have an struct
> > with those fields, just used them. (MigrationStats and XBZRLECacheStats).
> > 
> > Signed-off-by: Juan Quintela 
> 
> Hmm OK; this feels very much like it's the opposite of 180f61f from
> March; these variables keep moving around over the last couple of months
> - are they going to stay still now?

O:-)

Meanwhile, I don't know whether it'll be necessary to remove all the
functions like ram_bytes_transferred(), e.g., it would be just:

uint64_t ram_bytes_transferred(void)
{
-return ram_state.bytes_transferred;
+return ram_counters.transferred;
}

But I'm okay with either.

> 
> 
> Reviewed-by: Dr. David Alan Gilbert 

Reviewed-by: Peter Xu 

> 
> > ---
> >  migration/migration.c |  33 +-
> >  migration/ram.c   | 179 
> > ++
> >  migration/ram.h   |  15 +
> >  3 files changed, 68 insertions(+), 159 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 2c13217..331cab7 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -480,28 +480,28 @@ static void populate_ram_info(MigrationInfo *info, 
> > MigrationState *s)
> >  {
> >  info->has_ram = true;
> >  info->ram = g_malloc0(sizeof(*info->ram));
> > -info->ram->transferred = ram_bytes_transferred();
> > +info->ram->transferred = ram_counters.transferred;
> >  info->ram->total = ram_bytes_total();
> > -info->ram->duplicate = dup_mig_pages_transferred();
> > +info->ram->duplicate = ram_counters.duplicate;
> >  /* legacy value.  It is not used anymore */
> >  info->ram->skipped = 0;
> > -info->ram->normal = norm_mig_pages_transferred();
> > -info->ram->normal_bytes = norm_mig_pages_transferred() *
> > +info->ram->normal = ram_counters.normal;
> > +info->ram->normal_bytes = ram_counters.normal *
> >  qemu_target_page_size();
> >  info->ram->mbps = s->mbps;
> > -info->ram->dirty_sync_count = ram_dirty_sync_count();
> > -info->ram->postcopy_requests = ram_postcopy_requests();
> > +info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> > +info->ram->postcopy_requests = ram_counters.postcopy_requests;
> >  info->ram->page_size = qemu_target_page_size();
> >  
> >  if (migrate_use_xbzrle()) {
> >  info->has_xbzrle_cache = true;
> >  info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
> >  info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
> > -info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
> > -info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
> > -info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
> > -info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
> > -info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
> > +info->xbzrle_cache->bytes = xbzrle_counters.bytes;
> > +info->xbzrle_cache->pages = xbzrle_counters.pages;
> > +info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
> > +info->xbzrle_cache->cache_miss_rate = 
> > xbzrle_counters.cache_miss_rate;
> > +info->xbzrle_cache->overflow = xbzrle_counters.overflow;
> >  }
> >  
> >  if (cpu_throttle_active()) {
> > @@ -518,10 +518,11 @@ static void populate_ram_info(MigrationInfo *info, 
> > MigrationState *s)
> >  }
> >  
> >  if (s->state != MIGRATION_STATUS_COMPLETED) {
> > -info->ram->remaining_pages = ram_pages_remaining();
> > -info->ram->remaining = ram_pages_remaining() *
> > +
> > +info->ram->remaining_pages = ram_counters.remaining_pages;
> > +info->ram->remaining = ram_counters.remaining_pages *
> >  qemu_target_page_size();
> > -info->ram->dirty_pages_rate = ram_dirty_pages_rate();
> > +info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
> >  }
> >  }
> >  
> > @@ -1886,8 +1887,8 @@ static void *migration_thread(void *opaque)
> >bandwidth, threshold_size);
> >  /* if we haven't sent anything, we don't want to recalculate
> > 1 is a small enough number for our purposes */
> > -if (ram_dirty_pages_rate() && transferred_bytes > 1) {
> > -s->expected_downtime = ram_dirty_pages_rate() *
> > +if (ram_counters.dirty_pages_rate && transferred_bytes > 
> > 1) {
> > +s->expected_downtime = ram_counters.dirty_pages_rate *
> >  qemu_target_page_size() / bandwidth;
> >  }
> >  
> > diff --git a/migration/ram.c 

Re: [Qemu-devel] [PATCH 4/5] ram: Use MigrationStats for statistics

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> RAM Statistics need to survive migration to make info migrate work, so we
> need to store them outside of RAMState.  As we already have an struct
> with those fields, just used them. (MigrationStats and XBZRLECacheStats).
> 
> Signed-off-by: Juan Quintela 

Hmm OK; this feels very much like it's the opposite of 180f61f from
March; these variables keep moving around over the last couple of months
- are they going to stay still now?


Reviewed-by: Dr. David Alan Gilbert 

> ---
>  migration/migration.c |  33 +-
>  migration/ram.c   | 179 
> ++
>  migration/ram.h   |  15 +
>  3 files changed, 68 insertions(+), 159 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 2c13217..331cab7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -480,28 +480,28 @@ static void populate_ram_info(MigrationInfo *info, 
> MigrationState *s)
>  {
>  info->has_ram = true;
>  info->ram = g_malloc0(sizeof(*info->ram));
> -info->ram->transferred = ram_bytes_transferred();
> +info->ram->transferred = ram_counters.transferred;
>  info->ram->total = ram_bytes_total();
> -info->ram->duplicate = dup_mig_pages_transferred();
> +info->ram->duplicate = ram_counters.duplicate;
>  /* legacy value.  It is not used anymore */
>  info->ram->skipped = 0;
> -info->ram->normal = norm_mig_pages_transferred();
> -info->ram->normal_bytes = norm_mig_pages_transferred() *
> +info->ram->normal = ram_counters.normal;
> +info->ram->normal_bytes = ram_counters.normal *
>  qemu_target_page_size();
>  info->ram->mbps = s->mbps;
> -info->ram->dirty_sync_count = ram_dirty_sync_count();
> -info->ram->postcopy_requests = ram_postcopy_requests();
> +info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> +info->ram->postcopy_requests = ram_counters.postcopy_requests;
>  info->ram->page_size = qemu_target_page_size();
>  
>  if (migrate_use_xbzrle()) {
>  info->has_xbzrle_cache = true;
>  info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
>  info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
> -info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
> -info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
> -info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
> -info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
> -info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
> +info->xbzrle_cache->bytes = xbzrle_counters.bytes;
> +info->xbzrle_cache->pages = xbzrle_counters.pages;
> +info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
> +info->xbzrle_cache->cache_miss_rate = 
> xbzrle_counters.cache_miss_rate;
> +info->xbzrle_cache->overflow = xbzrle_counters.overflow;
>  }
>  
>  if (cpu_throttle_active()) {
> @@ -518,10 +518,11 @@ static void populate_ram_info(MigrationInfo *info, 
> MigrationState *s)
>  }
>  
>  if (s->state != MIGRATION_STATUS_COMPLETED) {
> -info->ram->remaining_pages = ram_pages_remaining();
> -info->ram->remaining = ram_pages_remaining() *
> +
> +info->ram->remaining_pages = ram_counters.remaining_pages;
> +info->ram->remaining = ram_counters.remaining_pages *
>  qemu_target_page_size();
> -info->ram->dirty_pages_rate = ram_dirty_pages_rate();
> +info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
>  }
>  }
>  
> @@ -1886,8 +1887,8 @@ static void *migration_thread(void *opaque)
>bandwidth, threshold_size);
>  /* if we haven't sent anything, we don't want to recalculate
> 1 is a small enough number for our purposes */
> -if (ram_dirty_pages_rate() && transferred_bytes > 1) {
> -s->expected_downtime = ram_dirty_pages_rate() *
> +if (ram_counters.dirty_pages_rate && transferred_bytes > 1) {
> +s->expected_downtime = ram_counters.dirty_pages_rate *
>  qemu_target_page_size() / bandwidth;
>  }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 30519e1..6c48219 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -71,6 +71,8 @@ static inline bool is_zero_range(uint8_t *p, uint64_t size)
>  return buffer_is_zero(p, size);
>  }
>  
> +XBZRLECacheStats xbzrle_counters;
> +
>  /* struct contains XBZRLE cache and a static page
> used by the compression */
>  static struct {
> @@ -174,8 +176,6 @@ struct RAMState {
>  bool ram_bulk_stage;
>  /* How many times we have dirty too many pages */
>  int dirty_rate_high_cnt;
> -/* How many times we have synchronized the bitmap */
> -uint64_t 

[Qemu-devel] [PATCH 4/5] ram: Use MigrationStats for statistics

[Juan Quintela]

RAM Statistics need to survive migration to make info migrate work, so we
need to store them outside of RAMState.  As we already have an struct
with those fields, just used them. (MigrationStats and XBZRLECacheStats).

Signed-off-by: Juan Quintela 
---
 migration/migration.c |  33 +-
 migration/ram.c   | 179 ++
 migration/ram.h   |  15 +
 3 files changed, 68 insertions(+), 159 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2c13217..331cab7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -480,28 +480,28 @@ static void populate_ram_info(MigrationInfo *info, 
MigrationState *s)
 {
 info->has_ram = true;
 info->ram = g_malloc0(sizeof(*info->ram));
-info->ram->transferred = ram_bytes_transferred();
+info->ram->transferred = ram_counters.transferred;
 info->ram->total = ram_bytes_total();
-info->ram->duplicate = dup_mig_pages_transferred();
+info->ram->duplicate = ram_counters.duplicate;
 /* legacy value.  It is not used anymore */
 info->ram->skipped = 0;
-info->ram->normal = norm_mig_pages_transferred();
-info->ram->normal_bytes = norm_mig_pages_transferred() *
+info->ram->normal = ram_counters.normal;
+info->ram->normal_bytes = ram_counters.normal *
 qemu_target_page_size();
 info->ram->mbps = s->mbps;
-info->ram->dirty_sync_count = ram_dirty_sync_count();
-info->ram->postcopy_requests = ram_postcopy_requests();
+info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
+info->ram->postcopy_requests = ram_counters.postcopy_requests;
 info->ram->page_size = qemu_target_page_size();
 
 if (migrate_use_xbzrle()) {
 info->has_xbzrle_cache = true;
 info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
 info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
-info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
-info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
-info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
-info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
-info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
+info->xbzrle_cache->bytes = xbzrle_counters.bytes;
+info->xbzrle_cache->pages = xbzrle_counters.pages;
+info->xbzrle_cache->cache_miss = xbzrle_counters.cache_miss;
+info->xbzrle_cache->cache_miss_rate = xbzrle_counters.cache_miss_rate;
+info->xbzrle_cache->overflow = xbzrle_counters.overflow;
 }
 
 if (cpu_throttle_active()) {
@@ -518,10 +518,11 @@ static void populate_ram_info(MigrationInfo *info, 
MigrationState *s)
 }
 
 if (s->state != MIGRATION_STATUS_COMPLETED) {
-info->ram->remaining_pages = ram_pages_remaining();
-info->ram->remaining = ram_pages_remaining() *
+
+info->ram->remaining_pages = ram_counters.remaining_pages;
+info->ram->remaining = ram_counters.remaining_pages *
 qemu_target_page_size();
-info->ram->dirty_pages_rate = ram_dirty_pages_rate();
+info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
 }
 }
 
@@ -1886,8 +1887,8 @@ static void *migration_thread(void *opaque)
   bandwidth, threshold_size);
 /* if we haven't sent anything, we don't want to recalculate
1 is a small enough number for our purposes */
-if (ram_dirty_pages_rate() && transferred_bytes > 1) {
-s->expected_downtime = ram_dirty_pages_rate() *
+if (ram_counters.dirty_pages_rate && transferred_bytes > 1) {
+s->expected_downtime = ram_counters.dirty_pages_rate *
 qemu_target_page_size() / bandwidth;
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index 30519e1..6c48219 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -71,6 +71,8 @@ static inline bool is_zero_range(uint8_t *p, uint64_t size)
 return buffer_is_zero(p, size);
 }
 
+XBZRLECacheStats xbzrle_counters;
+
 /* struct contains XBZRLE cache and a static page
used by the compression */
 static struct {
@@ -174,8 +176,6 @@ struct RAMState {
 bool ram_bulk_stage;
 /* How many times we have dirty too many pages */
 int dirty_rate_high_cnt;
-/* How many times we have synchronized the bitmap */
-uint64_t bitmap_sync_count;
 /* these variables are used for bitmap sync */
 /* last time we did a full bitmap_sync */
 int64_t time_last_bitmap_sync;
@@ -187,32 +187,8 @@ struct RAMState {
 uint64_t xbzrle_cache_miss_prev;
 /* number of iterations at the beginning of period */
 uint64_t iterations_prev;
-/* Accounting fields */
-/* number of zero pages.  It used to be pages filled by the same char. */
-uint64_t zero_pages;
-/* number of normal transferred