[PATCH 0/9] monitor/hmp-cmds: small improvements for migration

[PATCH 0/9] monitor/hmp-cmds: small improvements for migration

[Mao Zhongyi]

patch1 ~ patch4 have been r-b, here rebase to HEAD.
patch5 ~ patch9 mainly decorate some statistics
and minor logic changes of migration, because they
all involve migration, so merged into a patchset.

Cc: dgilbert@redhat.com
Cc: quintela@redhat.com

Mao Zhongyi (9):
  tests/migration: mem leak fix
  tests/migration: fix unreachable path in stress test
  monitor/hmp-cmds: add units for migrate_parameters
  monitor/hmp-cmds: don't silently output when running
    'migrate_set_downtime' fails
  monitor/hmp-cmds: delete redundant Error check before invoke
    hmp_handle_error()
  monitor/hmp-cmds: add 'goto end' to reduce duplicate code.
  monitor/hmp-cmds: improvements for the 'info migrate'
  docs/xbzrle: update 'cache miss rate' and 'encoding rate' to docs
  migration/ram: calculate un/encoded_size only when needed.

 docs/xbzrle.txt          |  8 +++++---
 migration/ram.c          |  9 +++++----
 monitor/hmp-cmds.c       | 30 ++++++++++++++++--------------
 tests/migration/stress.c | 34 +++++++---------------------------
 4 files changed, 33 insertions(+), 48 deletions(-)

-- 
2.17.1

[PATCH 1/9] tests/migration: mem leak fix

[Mao Zhongyi]

‘data’ has the possibility of memory leaks， so use the
glib macros g_autofree recommended by CODING_STYLE.rst
to automatically release the memory that returned from
g_malloc().

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 tests/migration/stress.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index 0c23964693..f9626d50ee 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -170,26 +170,14 @@ static unsigned long long now(void)
 static int stressone(unsigned long long ramsizeMB)
 {
     size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
-    char *ram = malloc(ramsizeMB * 1024 * 1024);
+    g_autofree char *ram = g_malloc(ramsizeMB * 1024 * 1024);
     char *ramptr;
     size_t i, j, k;
-    char *data = malloc(PAGE_SIZE);
+    g_autofree char *data = g_malloc(PAGE_SIZE);
     char *dataptr;
     size_t nMB = 0;
     unsigned long long before, after;
 
-    if (!ram) {
-        fprintf(stderr, "%s (%05d): ERROR: cannot allocate %llu MB of RAM: %s\n",
-                argv0, gettid(), ramsizeMB, strerror(errno));
-        return -1;
-    }
-    if (!data) {
-        fprintf(stderr, "%s (%d): ERROR: cannot allocate %d bytes of RAM: %s\n",
-                argv0, gettid(), PAGE_SIZE, strerror(errno));
-        free(ram);
-        return -1;
-    }
-
     /* We don't care about initial state, but we do want
      * to fault it all into RAM, otherwise the first iter
      * of the loop below will be quite slow. We can't use
@@ -198,8 +186,6 @@ static int stressone(unsigned long long ramsizeMB)
     memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
 
     if (random_bytes(data, PAGE_SIZE) < 0) {
-        free(ram);
-        free(data);
         return -1;
     }
 
@@ -227,9 +213,6 @@ static int stressone(unsigned long long ramsizeMB)
             }
         }
     }
-
-    free(data);
-    free(ram);
 }
 
 
-- 
2.17.1

[PATCH 2/9] tests/migration: fix unreachable path in stress test

[Mao Zhongyi]

If stressone() or stress() exits it's because of a failure
because the test runs forever otherwise, so change stressone
and stress type to void to make the exit_failure() as the exit
function of main().

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 tests/migration/stress.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index f9626d50ee..a062ef6b55 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -167,7 +167,7 @@ static unsigned long long now(void)
     return (tv.tv_sec * 1000ull) + (tv.tv_usec / 1000ull);
 }
 
-static int stressone(unsigned long long ramsizeMB)
+static void stressone(unsigned long long ramsizeMB)
 {
     size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
     g_autofree char *ram = g_malloc(ramsizeMB * 1024 * 1024);
@@ -186,7 +186,7 @@ static int stressone(unsigned long long ramsizeMB)
     memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
 
     if (random_bytes(data, PAGE_SIZE) < 0) {
-        return -1;
+        return;
     }
 
     before = now();
@@ -225,7 +225,7 @@ static void *stressthread(void *arg)
     return NULL;
 }
 
-static int stress(unsigned long long ramsizeGB, int ncpus)
+static void stress(unsigned long long ramsizeGB, int ncpus)
 {
     size_t i;
     unsigned long long ramsizeMB = ramsizeGB * 1024 / ncpus;
@@ -238,8 +238,6 @@ static int stress(unsigned long long ramsizeGB, int ncpus)
     }
 
     stressone(ramsizeMB);
-
-    return 0;
 }
 
 
@@ -335,8 +333,7 @@ int main(int argc, char **argv)
     fprintf(stdout, "%s (%05d): INFO: RAM %llu GiB across %d CPUs\n",
             argv0, gettid(), ramsizeGB, ncpus);
 
-    if (stress(ramsizeGB, ncpus) < 0)
-        exit_failure();
+    stress(ramsizeGB, ncpus);
 
-    exit_success();
+    exit_failure();
 }
-- 
2.17.1

[PATCH 3/9] monitor/hmp-cmds: add units for migrate_parameters

[Mao Zhongyi]

When running:
(qemu) info migrate_parameters
announce-initial: 50 ms
announce-max: 550 ms
announce-step: 100 ms
compress-wait-thread: on
...
max-bandwidth: 33554432 bytes/second
downtime-limit: 300 milliseconds
x-checkpoint-delay: 20000
...
xbzrle-cache-size: 67108864

add units for the parameters 'x-checkpoint-delay' and
'xbzrle-cache-size', it's easier to read, also move
milliseconds to ms to keep the same style.

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 monitor/hmp-cmds.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9c61e769ca..8c3e436b39 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -443,11 +443,11 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
             params->max_bandwidth);
         assert(params->has_downtime_limit);
-        monitor_printf(mon, "%s: %" PRIu64 " milliseconds\n",
+        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
             params->downtime_limit);
         assert(params->has_x_checkpoint_delay);
-        monitor_printf(mon, "%s: %u\n",
+        monitor_printf(mon, "%s: %u ms\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
             params->x_checkpoint_delay);
         assert(params->has_block_incremental);
@@ -460,7 +460,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION),
             MultiFDCompression_str(params->multifd_compression));
-        monitor_printf(mon, "%s: %" PRIu64 "\n",
+        monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
         monitor_printf(mon, "%s: %" PRIu64 "\n",
-- 
2.17.1

[PATCH 4/9] monitor/hmp-cmds: don't silently output when running 'migrate_set_downtime' fails

[Mao Zhongyi]

Although 'migrate_set_downtime' has been deprecated and replaced
with 'migrate_set_parameter downtime_limit', it has not been
completely eliminated, possibly due to compatibility with older
versions. I think as long as this old parameter is running, we
should report appropriate message when something goes wrong, not
be silent.

before:
(qemu) migrate_set_downtime -1
(qemu)

after:
(qemu) migrate_set_downtime -1
Error: Parameter 'downtime_limit' expects an integer in the range of 0 to 2000 seconds

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 monitor/hmp-cmds.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8c3e436b39..6938f1060e 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1188,8 +1188,11 @@ void hmp_migrate_pause(Monitor *mon, const QDict *qdict)
 /* Kept for backwards compatibility */
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
+    Error *err = NULL;
+
     double value = qdict_get_double(qdict, "value");
-    qmp_migrate_set_downtime(value, NULL);
+    qmp_migrate_set_downtime(value, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
-- 
2.17.1

[PATCH 5/9] monitor/hmp-cmds: delete redundant Error check before invoke hmp_handle_error()

[Mao Zhongyi]

hmp_handle_error() does Error check internally.

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 monitor/hmp-cmds.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 6938f1060e..acdd6baff3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1636,9 +1636,8 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     obj = user_creatable_add_opts(opts, &err);
     qemu_opts_del(opts);
 
-    if (err) {
-        hmp_handle_error(mon, err);
-    }
+    hmp_handle_error(mon, err);
+
     if (obj) {
         object_unref(obj);
     }
-- 
2.17.1

Re: [PATCH 5/9] monitor/hmp-cmds: delete redundant Error check before invoke hmp_handle_error()

[Dr. David Alan Gilbert]

* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> hmp_handle_error() does Error check internally.
> 
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>

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

> ---
>  monitor/hmp-cmds.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6938f1060e..acdd6baff3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1636,9 +1636,8 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>      obj = user_creatable_add_opts(opts, &err);
>      qemu_opts_del(opts);
>  
> -    if (err) {
> -        hmp_handle_error(mon, err);
> -    }
> +    hmp_handle_error(mon, err);
> +
>      if (obj) {
>          object_unref(obj);
>      }
> -- 
> 2.17.1
> 
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 6/9] monitor/hmp-cmds: add 'goto end' to reduce duplicate code.

[Mao Zhongyi]

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 monitor/hmp-cmds.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index acdd6baff3..e8cf72eb3a 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1501,8 +1501,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
                                 read_only,
                                 BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, &err);
             if (err) {
-                hmp_handle_error(mon, err);
-                return;
+                goto end;
             }
         }
 
@@ -1511,6 +1510,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
                                    &err);
     }
 
+end:
     hmp_handle_error(mon, err);
 }
 
@@ -1629,13 +1629,13 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
-        hmp_handle_error(mon, err);
-        return;
+        goto end;
     }
 
     obj = user_creatable_add_opts(opts, &err);
     qemu_opts_del(opts);
 
+end:
     hmp_handle_error(mon, err);
 
     if (obj) {
-- 
2.17.1

Re: [PATCH 6/9] monitor/hmp-cmds: add 'goto end' to reduce duplicate code.

[Dr. David Alan Gilbert]

* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>

Yes OK; I don't particularly like goto's; especially in these simpler
cases, but it's OK.


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

> ---
>  monitor/hmp-cmds.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index acdd6baff3..e8cf72eb3a 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1501,8 +1501,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>                                  read_only,
>                                  BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, &err);
>              if (err) {
> -                hmp_handle_error(mon, err);
> -                return;
> +                goto end;
>              }
>          }
>  
> @@ -1511,6 +1510,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>                                     &err);
>      }
>  
> +end:
>      hmp_handle_error(mon, err);
>  }
>  
> @@ -1629,13 +1629,13 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>  
>      opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>      if (err) {
> -        hmp_handle_error(mon, err);
> -        return;
> +        goto end;
>      }
>  
>      obj = user_creatable_add_opts(opts, &err);
>      qemu_opts_del(opts);
>  
> +end:
>      hmp_handle_error(mon, err);
>  
>      if (obj) {
> -- 
> 2.17.1
> 
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 7/9] monitor/hmp-cmds: improvements for the 'info migrate'

[Mao Zhongyi]

When running:

(qemu) info migrate
globals:
store-global-state: on
only-migratable: off
...
xbzrle transferred: 640892 kbytes
xbzrle pages: 16645936 pages
xbzrle cache miss: 1525426
xbzrle cache miss rate: 0.09
xbzrle encoding rate: 91.42
xbzrle overflow: 40896
...
compression pages: 377710 pages
compression busy: 0
compression busy rate: 0.00
compressed size: 463169457
compression rate: 3.33

Add units for 'xbzrle cache miss' and 'compressed size',
make it easier to read.

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 docs/xbzrle.txt    | 2 +-
 monitor/hmp-cmds.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
index b431bdaf0f..385b4993f8 100644
--- a/docs/xbzrle.txt
+++ b/docs/xbzrle.txt
@@ -112,7 +112,7 @@ is recommended.
     cache size: H bytes
     xbzrle transferred: I kbytes
     xbzrle pages: J pages
-    xbzrle cache miss: K
+    xbzrle cache miss: K pages
     xbzrle overflow: L
 
 xbzrle cache-miss: the number of cache misses to date - high cache-miss rate
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index e8cf72eb3a..24f3e8e44d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -299,7 +299,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->bytes >> 10);
         monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
                        info->xbzrle_cache->pages);
-        monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
+        monitor_printf(mon, "xbzrle cache miss: %" PRIu64 " pages\n",
                        info->xbzrle_cache->cache_miss);
         monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
                        info->xbzrle_cache->cache_miss_rate);
@@ -316,8 +316,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->compression->busy);
         monitor_printf(mon, "compression busy rate: %0.2f\n",
                        info->compression->busy_rate);
-        monitor_printf(mon, "compressed size: %" PRIu64 "\n",
-                       info->compression->compressed_size);
+        monitor_printf(mon, "compressed size: %" PRIu64 " kbytes\n",
+                       info->compression->compressed_size >> 10);
         monitor_printf(mon, "compression rate: %0.2f\n",
                        info->compression->compression_rate);
     }
-- 
2.17.1

Re: [PATCH 7/9] monitor/hmp-cmds: improvements for the 'info migrate'

[Dr. David Alan Gilbert]

* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> When running:
> 
> (qemu) info migrate
> globals:
> store-global-state: on
> only-migratable: off
> ...
> xbzrle transferred: 640892 kbytes
> xbzrle pages: 16645936 pages
> xbzrle cache miss: 1525426
> xbzrle cache miss rate: 0.09
> xbzrle encoding rate: 91.42
> xbzrle overflow: 40896
> ...
> compression pages: 377710 pages
> compression busy: 0
> compression busy rate: 0.00
> compressed size: 463169457
> compression rate: 3.33
> 
> Add units for 'xbzrle cache miss' and 'compressed size',
> make it easier to read.
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>

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

> ---
>  docs/xbzrle.txt    | 2 +-
>  monitor/hmp-cmds.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
> index b431bdaf0f..385b4993f8 100644
> --- a/docs/xbzrle.txt
> +++ b/docs/xbzrle.txt
> @@ -112,7 +112,7 @@ is recommended.
>      cache size: H bytes
>      xbzrle transferred: I kbytes
>      xbzrle pages: J pages
> -    xbzrle cache miss: K
> +    xbzrle cache miss: K pages
>      xbzrle overflow: L
>  
>  xbzrle cache-miss: the number of cache misses to date - high cache-miss rate
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index e8cf72eb3a..24f3e8e44d 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -299,7 +299,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->xbzrle_cache->bytes >> 10);
>          monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
>                         info->xbzrle_cache->pages);
> -        monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
> +        monitor_printf(mon, "xbzrle cache miss: %" PRIu64 " pages\n",
>                         info->xbzrle_cache->cache_miss);
>          monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
>                         info->xbzrle_cache->cache_miss_rate);
> @@ -316,8 +316,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->compression->busy);
>          monitor_printf(mon, "compression busy rate: %0.2f\n",
>                         info->compression->busy_rate);
> -        monitor_printf(mon, "compressed size: %" PRIu64 "\n",
> -                       info->compression->compressed_size);
> +        monitor_printf(mon, "compressed size: %" PRIu64 " kbytes\n",
> +                       info->compression->compressed_size >> 10);
>          monitor_printf(mon, "compression rate: %0.2f\n",
>                         info->compression->compression_rate);
>      }
> -- 
> 2.17.1
> 
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 8/9] docs/xbzrle: update 'cache miss rate' and 'encoding rate' to docs

[Mao Zhongyi]

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 docs/xbzrle.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
index 385b4993f8..6bd1828f34 100644
--- a/docs/xbzrle.txt
+++ b/docs/xbzrle.txt
@@ -113,9 +113,11 @@ is recommended.
     xbzrle transferred: I kbytes
     xbzrle pages: J pages
     xbzrle cache miss: K pages
-    xbzrle overflow: L
+    xbzrle cache miss rate: L
+    xbzrle encoding rate: M
+    xbzrle overflow: N
 
-xbzrle cache-miss: the number of cache misses to date - high cache-miss rate
+xbzrle cache miss: the number of cache misses to date - high cache-miss rate
 indicates that the cache size is set too low.
 xbzrle overflow: the number of overflows in the decoding which where the delta
 could not be compressed. This can happen if the changes in the pages are too
-- 
2.17.1

Re: [PATCH 8/9] docs/xbzrle: update 'cache miss rate' and 'encoding rate' to docs

[Dr. David Alan Gilbert]

* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>

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

> ---
>  docs/xbzrle.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
> index 385b4993f8..6bd1828f34 100644
> --- a/docs/xbzrle.txt
> +++ b/docs/xbzrle.txt
> @@ -113,9 +113,11 @@ is recommended.
>      xbzrle transferred: I kbytes
>      xbzrle pages: J pages
>      xbzrle cache miss: K pages
> -    xbzrle overflow: L
> +    xbzrle cache miss rate: L
> +    xbzrle encoding rate: M
> +    xbzrle overflow: N
>  
> -xbzrle cache-miss: the number of cache misses to date - high cache-miss rate
> +xbzrle cache miss: the number of cache misses to date - high cache-miss rate
>  indicates that the cache size is set too low.
>  xbzrle overflow: the number of overflows in the decoding which where the delta
>  could not be compressed. This can happen if the changes in the pages are too
> -- 
> 2.17.1
> 
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 9/9] migration/ram: calculate un/encoded_size only when needed.

[Mao Zhongyi]

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 migration/ram.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 41cc530d9d..ca20030b64 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -910,14 +910,15 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
             rs->xbzrle_cache_miss_prev) / page_count;
         rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
-        unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
-                         TARGET_PAGE_SIZE;
-        encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
         if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
             xbzrle_counters.encoding_rate = 0;
-        } else if (!encoded_size) {
+        } else if (xbzrle_counters.bytes == rs->xbzrle_bytes_prev) {
             xbzrle_counters.encoding_rate = UINT64_MAX;
         } else {
+            unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
+                             TARGET_PAGE_SIZE;
+            encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
+
             xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
         }
         rs->xbzrle_pages_prev = xbzrle_counters.pages;
-- 
2.17.1

Re: [PATCH 9/9] migration/ram: calculate un/encoded_size only when needed.

[Dr. David Alan Gilbert]

* Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> ---
>  migration/ram.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 41cc530d9d..ca20030b64 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -910,14 +910,15 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>          xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>              rs->xbzrle_cache_miss_prev) / page_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> -        unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
> -                         TARGET_PAGE_SIZE;
> -        encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
>          if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
>              xbzrle_counters.encoding_rate = 0;
> -        } else if (!encoded_size) {
> +        } else if (xbzrle_counters.bytes == rs->xbzrle_bytes_prev) {

No, I don't think this change is worth it - this is really just the same
as 'encoded_size', and then we may as well keep the two together.

Dave

>              xbzrle_counters.encoding_rate = UINT64_MAX;
>          } else {
> +            unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
> +                             TARGET_PAGE_SIZE;
> +            encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
> +
>              xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
>          }
>          rs->xbzrle_pages_prev = xbzrle_counters.pages;
> -- 
> 2.17.1
> 
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 9/9] migration/ram: calculate un/encoded_size only whenneeded.

[maozy]

On 6/12/20 3:05 AM, Dr. David Alan Gilbert wrote:
> * Mao Zhongyi (maozhongyi@cmss.chinamobile.com) wrote:
>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
>> ---
>>   migration/ram.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 41cc530d9d..ca20030b64 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -910,14 +910,15 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>>           xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>>               rs->xbzrle_cache_miss_prev) / page_count;
>>           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>> -        encoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
>> -                         TARGET_PAGE_SIZE;
>> -        encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
>>           if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
>>               xbzrle_counters.encoding_rate = 0;
>> -        } else if (!encoded_size) {
>> +        } else if (xbzrle_counters.bytes == rs->xbzrle_bytes_prev) {
> 
> No, I don't think this change is worth it - this is really just the same
> as 'encoded_size', and then we may as well keep the two together.

ok, thanks, let's keep 'encode_size' here.

BTW, this change borrows from the behavior of comppressed:

...
         compressed_size = compression_counters.compressed_size -
                           rs->compressed_size_prev;
         if (compressed_size) {
             double uncompressed_size = (compression_counters.pages -
                                     rs->compress_pages_prev) * 
TARGET_PAGE_SIZE;

             /* Compression-Ratio = Uncompressed-size / Compressed-size */
             compression_counters.compression_rate =
                                         uncompressed_size / 
compressed_size;
...


It splits 'compressed_size' and 'uncompressed_size', and calculates
'uncompressed_size' only when needed. Although 'unencoded_size' is
calculated, it is not necessarily used. if you think this split is
unnecessary, just discard it, so do I need to drop this patch and
resend the v2?

Thanks,
Mao

> 
> Dave
> 
>>               xbzrle_counters.encoding_rate = UINT64_MAX;
>>           } else {
>> +            unencoded_size = (xbzrle_counters.pages - rs->xbzrle_pages_prev) *
>> +                             TARGET_PAGE_SIZE;
>> +            encoded_size = xbzrle_counters.bytes - rs->xbzrle_bytes_prev;
>> +
>>               xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
>>           }
>>           rs->xbzrle_pages_prev = xbzrle_counters.pages;
>> -- 
>> 2.17.1
>>
>>
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
>