[Qemu-devel] [PATCH v2 0/8] migration: compression optimization

[Qemu-devel] [PATCH v2 0/8] migration: compression optimization

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Thanks to Peter's suggestion, i split the long series (1) and this is the
first part.

I am not sure if Dave is happy to @reduced-size, will change immediately
if it's objected. :)

Changelog in v2:
1) introduce a parameter to make the main thread wait for free thread
   thread to compress the data as current behavior based on the comments
   from Dave and Peter
2) description improvement for compression statistics, thanks for Eric's
   improvement
3) move the handle of zero page to the threads
4) fix the issue that the new dirtied page would be overwritten at the
   destination when we drop flush_compressed_data at the end of iteration
   which is pointed out by Dave

(1) https://www.spinics.net/lists/kvm/msg169774.html
   
Xiao Guangrong (8):
  migration: do not wait for free thread
  migration: fix counting normal page for compression
  migration: show the statistics of compression
  migration: introduce save_zero_page_to_file
  migration: drop the return value of do_compress_ram_page
  migration: move handle of zero page to the thread
  migration: hold the lock only if it is really needed
  migration: do not flush_compressed_data at the end of each iteration

 hmp.c                 |  21 +++++
 include/qemu/queue.h  |   1 +
 migration/migration.c |  32 +++++++
 migration/migration.h |   1 +
 migration/ram.c       | 244 +++++++++++++++++++++++++++++++++++++-------------
 migration/ram.h       |   1 +
 qapi/migration.json   |  49 ++++++++--
 7 files changed, 281 insertions(+), 68 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH v2 1/8] migration: do not wait for free thread

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Instead of putting the main thread to sleep state to wait for
free compression thread, we can directly post it out as normal
page that reduces the latency and uses CPUs more efficiently

A parameter, compress-wait-thread, is introduced, it can be
enabled if the user really wants the old behavior

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hmp.c                 |  8 ++++++++
 migration/migration.c | 21 +++++++++++++++++++++
 migration/migration.h |  1 +
 migration/ram.c       | 45 ++++++++++++++++++++++++++-------------------
 qapi/migration.json   | 23 ++++++++++++++++++-----
 5 files changed, 74 insertions(+), 24 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2aafb50e8e..47d36e3ccf 100644
--- a/hmp.c
+++ b/hmp.c
@@ -327,6 +327,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_THREADS),
             params->compress_threads);
+        assert(params->has_compress_wait_thread);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD),
+            params->compress_wait_thread ? "on" : "off");
         assert(params->has_decompress_threads);
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS),
@@ -1623,6 +1627,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_compress_threads = true;
         visit_type_int(v, param, &p->compress_threads, &err);
         break;
+    case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
+        p->has_compress_wait_thread = true;
+        visit_type_bool(v, param, &p->compress_wait_thread, &err);
+        break;
     case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
         p->has_decompress_threads = true;
         visit_type_int(v, param, &p->decompress_threads, &err);
diff --git a/migration/migration.c b/migration/migration.c
index 8d56d56930..0af75465b3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -671,6 +671,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->compress_level = s->parameters.compress_level;
     params->has_compress_threads = true;
     params->compress_threads = s->parameters.compress_threads;
+    params->has_compress_wait_thread = true;
+    params->compress_wait_thread = s->parameters.compress_wait_thread;
     params->has_decompress_threads = true;
     params->decompress_threads = s->parameters.decompress_threads;
     params->has_cpu_throttle_initial = true;
@@ -1061,6 +1063,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->compress_threads = params->compress_threads;
     }
 
+    if (params->has_compress_wait_thread) {
+        dest->compress_wait_thread = params->compress_wait_thread;
+    }
+
     if (params->has_decompress_threads) {
         dest->decompress_threads = params->decompress_threads;
     }
@@ -1126,6 +1132,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.compress_threads = params->compress_threads;
     }
 
+    if (params->has_compress_wait_thread) {
+        s->parameters.compress_wait_thread = params->compress_wait_thread;
+    }
+
     if (params->has_decompress_threads) {
         s->parameters.decompress_threads = params->decompress_threads;
     }
@@ -1852,6 +1862,15 @@ int migrate_compress_threads(void)
     return s->parameters.compress_threads;
 }
 
+int migrate_compress_wait_thread(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.compress_wait_thread;
+}
+
 int migrate_decompress_threads(void)
 {
     MigrationState *s;
@@ -3113,6 +3132,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
                       parameters.compress_threads,
                       DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
+    DEFINE_PROP_BOOL("x-compress-wait-thread", MigrationState,
+                      parameters.compress_wait_thread, false),
     DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
                       parameters.decompress_threads,
                       DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
diff --git a/migration/migration.h b/migration/migration.h
index 64a7b33735..a46b9e6c8d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -271,6 +271,7 @@ bool migrate_use_return_path(void);
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
+int migrate_compress_wait_thread(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
diff --git a/migration/ram.c b/migration/ram.c
index 52dd678092..0ad234c692 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1889,30 +1889,34 @@ static int compress_page_with_multi_thread(RAMState *rs, RAMBlock *block,
                                            ram_addr_t offset)
 {
     int idx, thread_count, bytes_xmit = -1, pages = -1;
+    bool wait = migrate_compress_wait_thread();
 
     thread_count = migrate_compress_threads();
     qemu_mutex_lock(&comp_done_lock);
-    while (true) {
-        for (idx = 0; idx < thread_count; idx++) {
-            if (comp_param[idx].done) {
-                comp_param[idx].done = false;
-                bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
-                qemu_mutex_lock(&comp_param[idx].mutex);
-                set_compress_params(&comp_param[idx], block, offset);
-                qemu_cond_signal(&comp_param[idx].cond);
-                qemu_mutex_unlock(&comp_param[idx].mutex);
-                pages = 1;
-                ram_counters.normal++;
-                ram_counters.transferred += bytes_xmit;
-                break;
-            }
-        }
-        if (pages > 0) {
+retry:
+    for (idx = 0; idx < thread_count; idx++) {
+        if (comp_param[idx].done) {
+            comp_param[idx].done = false;
+            bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
+            qemu_mutex_lock(&comp_param[idx].mutex);
+            set_compress_params(&comp_param[idx], block, offset);
+            qemu_cond_signal(&comp_param[idx].cond);
+            qemu_mutex_unlock(&comp_param[idx].mutex);
+            pages = 1;
+            ram_counters.normal++;
+            ram_counters.transferred += bytes_xmit;
             break;
-        } else {
-            qemu_cond_wait(&comp_done_cond, &comp_done_lock);
         }
     }
+
+    /*
+     * if there is no thread is free to compress the data and the user
+     * really expects the slowdown, wait it.
+     */
+    if (pages < 0 && wait) {
+        qemu_cond_wait(&comp_done_cond, &comp_done_lock);
+        goto retry;
+    }
     qemu_mutex_unlock(&comp_done_lock);
 
     return pages;
@@ -2226,7 +2230,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
      * CPU resource.
      */
     if (block == rs->last_sent_block && save_page_use_compression(rs)) {
-        return compress_page_with_multi_thread(rs, block, offset);
+        res = compress_page_with_multi_thread(rs, block, offset);
+        if (res > 0) {
+            return res;
+        }
     } else if (migrate_use_multifd()) {
         return ram_save_multifd_page(rs, block, offset);
     }
diff --git a/qapi/migration.json b/qapi/migration.json
index 186e8a7303..b4f394844b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -462,6 +462,11 @@
 # @compress-threads: Set compression thread count to be used in live migration,
 #          the compression thread count is an integer between 1 and 255.
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#          if it's enabled, otherwise, the page will be posted out immediately
+#          in the main thread without compression. It's off on default.
+#          (Since: 3.0)
+#
 # @decompress-threads: Set decompression thread count to be used in live
 #          migration, the decompression thread count is an integer between 1
 #          and 255. Usually, decompression is at least 4 times as fast as
@@ -526,11 +531,11 @@
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
-  'data': ['compress-level', 'compress-threads', 'decompress-threads',
-           'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname', 'max-bandwidth',
-           'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
-           'x-multifd-channels', 'x-multifd-page-count',
+  'data': ['compress-level', 'compress-threads', 'compress-wait-thread',
+           'decompress-threads', 'cpu-throttle-initial',
+           'cpu-throttle-increment', 'tls-creds', 'tls-hostname',
+           'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay',
+           'block-incremental', 'x-multifd-channels', 'x-multifd-page-count',
            'xbzrle-cache-size', 'max-postcopy-bandwidth' ] }
 
 ##
@@ -540,6 +545,9 @@
 #
 # @compress-threads: compression thread count
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#                        (Since: 3.0)
+#
 # @decompress-threads: decompression thread count
 #
 # @cpu-throttle-initial: Initial percentage of time guest cpus are
@@ -610,6 +618,7 @@
 { 'struct': 'MigrateSetParameters',
   'data': { '*compress-level': 'int',
             '*compress-threads': 'int',
+            '*compress-wait-thread': 'bool',
             '*decompress-threads': 'int',
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
@@ -649,6 +658,9 @@
 #
 # @compress-threads: compression thread count
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#                        (Since: 3.0)
+#
 # @decompress-threads: decompression thread count
 #
 # @cpu-throttle-initial: Initial percentage of time guest cpus are
@@ -714,6 +726,7 @@
 { 'struct': 'MigrationParameters',
   'data': { '*compress-level': 'uint8',
             '*compress-threads': 'uint8',
+            '*compress-wait-thread': 'bool',
             '*decompress-threads': 'uint8',
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 1/8] migration: do not wait for free thread

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:13PM +0800, guangrong.xiao@gmail.com wrote:
> @@ -3113,6 +3132,8 @@ static Property migration_properties[] = {
>      DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
>                        parameters.compress_threads,
>                        DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
> +    DEFINE_PROP_BOOL("x-compress-wait-thread", MigrationState,
> +                      parameters.compress_wait_thread, false),

This performance feature bit makes sense to me, but I would still
think it should be true by default to keep the old behavior:

- it might change the behavior drastically: we might be in a state
  between "normal" migration and "compressed" migration since we'll
  contain both of the pages.  Old compression users might not expect
  that.

- it might still even perform worse - an extreme case is that when
  network bandwidth is very very limited but instead we have plenty of
  CPU resources. [1]

So it's really a good tunable for me when people really needs to
understand what's it before turning it on.

>      DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
>                        parameters.decompress_threads,
>                        DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
> diff --git a/migration/migration.h b/migration/migration.h
> index 64a7b33735..a46b9e6c8d 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -271,6 +271,7 @@ bool migrate_use_return_path(void);
>  bool migrate_use_compression(void);
>  int migrate_compress_level(void);
>  int migrate_compress_threads(void);
> +int migrate_compress_wait_thread(void);
>  int migrate_decompress_threads(void);
>  bool migrate_use_events(void);
>  bool migrate_postcopy_blocktime(void);
> diff --git a/migration/ram.c b/migration/ram.c
> index 52dd678092..0ad234c692 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1889,30 +1889,34 @@ static int compress_page_with_multi_thread(RAMState *rs, RAMBlock *block,
>                                             ram_addr_t offset)
>  {
>      int idx, thread_count, bytes_xmit = -1, pages = -1;
> +    bool wait = migrate_compress_wait_thread();
>  
>      thread_count = migrate_compress_threads();
>      qemu_mutex_lock(&comp_done_lock);
> -    while (true) {
> -        for (idx = 0; idx < thread_count; idx++) {
> -            if (comp_param[idx].done) {
> -                comp_param[idx].done = false;
> -                bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> -                qemu_mutex_lock(&comp_param[idx].mutex);
> -                set_compress_params(&comp_param[idx], block, offset);
> -                qemu_cond_signal(&comp_param[idx].cond);
> -                qemu_mutex_unlock(&comp_param[idx].mutex);
> -                pages = 1;
> -                ram_counters.normal++;
> -                ram_counters.transferred += bytes_xmit;
> -                break;
> -            }
> -        }
> -        if (pages > 0) {
> +retry:
> +    for (idx = 0; idx < thread_count; idx++) {
> +        if (comp_param[idx].done) {
> +            comp_param[idx].done = false;
> +            bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> +            qemu_mutex_lock(&comp_param[idx].mutex);
> +            set_compress_params(&comp_param[idx], block, offset);
> +            qemu_cond_signal(&comp_param[idx].cond);
> +            qemu_mutex_unlock(&comp_param[idx].mutex);
> +            pages = 1;
> +            ram_counters.normal++;
> +            ram_counters.transferred += bytes_xmit;
>              break;
> -        } else {
> -            qemu_cond_wait(&comp_done_cond, &comp_done_lock);
>          }
>      }
> +
> +    /*
> +     * if there is no thread is free to compress the data and the user
> +     * really expects the slowdown, wait it.

Considering [1] above, IMHO it might not really be a slow down but it
depends.  Maybe only mentioning about the fact that we're sending a
normal page instead of the compressed page if wait is not specified.

> +     */
> +    if (pages < 0 && wait) {
> +        qemu_cond_wait(&comp_done_cond, &comp_done_lock);
> +        goto retry;
> +    }
>      qemu_mutex_unlock(&comp_done_lock);
>  
>      return pages;
> @@ -2226,7 +2230,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>       * CPU resource.
>       */
>      if (block == rs->last_sent_block && save_page_use_compression(rs)) {
> -        return compress_page_with_multi_thread(rs, block, offset);
> +        res = compress_page_with_multi_thread(rs, block, offset);
> +        if (res > 0) {
> +            return res;
> +        }
>      } else if (migrate_use_multifd()) {
>          return ram_save_multifd_page(rs, block, offset);
>      }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 186e8a7303..b4f394844b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -462,6 +462,11 @@
>  # @compress-threads: Set compression thread count to be used in live migration,
>  #          the compression thread count is an integer between 1 and 255.
>  #
> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
> +#          if it's enabled, otherwise, the page will be posted out immediately
> +#          in the main thread without compression. It's off on default.
> +#          (Since: 3.0)
> +#

Should "Since 3.1" in all the places.

We'll need to touch up the "by default" part depending on whether we'd
need to change it according to above comment.

Otherwise it looks good to me.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 1/8] migration: do not wait for free thread

[Xiao Guangrong]

On 07/23/2018 11:25 AM, Peter Xu wrote:
> On Thu, Jul 19, 2018 at 08:15:13PM +0800, guangrong.xiao@gmail.com wrote:
>> @@ -3113,6 +3132,8 @@ static Property migration_properties[] = {
>>       DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
>>                         parameters.compress_threads,
>>                         DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
>> +    DEFINE_PROP_BOOL("x-compress-wait-thread", MigrationState,
>> +                      parameters.compress_wait_thread, false),
> 
> This performance feature bit makes sense to me, but I would still
> think it should be true by default to keep the old behavior:
> 
> - it might change the behavior drastically: we might be in a state
>    between "normal" migration and "compressed" migration since we'll
>    contain both of the pages.  Old compression users might not expect
>    that.
> 
> - it might still even perform worse - an extreme case is that when
>    network bandwidth is very very limited but instead we have plenty of
>    CPU resources. [1]
> 
> So it's really a good tunable for me when people really needs to
> understand what's it before turning it on.

That looks good to me.

> 
>>       DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
>>                         parameters.decompress_threads,
>>                         DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 64a7b33735..a46b9e6c8d 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -271,6 +271,7 @@ bool migrate_use_return_path(void);
>>   bool migrate_use_compression(void);
>>   int migrate_compress_level(void);
>>   int migrate_compress_threads(void);
>> +int migrate_compress_wait_thread(void);
>>   int migrate_decompress_threads(void);
>>   bool migrate_use_events(void);
>>   bool migrate_postcopy_blocktime(void);
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 52dd678092..0ad234c692 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1889,30 +1889,34 @@ static int compress_page_with_multi_thread(RAMState *rs, RAMBlock *block,
>>                                              ram_addr_t offset)
>>   {
>>       int idx, thread_count, bytes_xmit = -1, pages = -1;
>> +    bool wait = migrate_compress_wait_thread();
>>   
>>       thread_count = migrate_compress_threads();
>>       qemu_mutex_lock(&comp_done_lock);
>> -    while (true) {
>> -        for (idx = 0; idx < thread_count; idx++) {
>> -            if (comp_param[idx].done) {
>> -                comp_param[idx].done = false;
>> -                bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
>> -                qemu_mutex_lock(&comp_param[idx].mutex);
>> -                set_compress_params(&comp_param[idx], block, offset);
>> -                qemu_cond_signal(&comp_param[idx].cond);
>> -                qemu_mutex_unlock(&comp_param[idx].mutex);
>> -                pages = 1;
>> -                ram_counters.normal++;
>> -                ram_counters.transferred += bytes_xmit;
>> -                break;
>> -            }
>> -        }
>> -        if (pages > 0) {
>> +retry:
>> +    for (idx = 0; idx < thread_count; idx++) {
>> +        if (comp_param[idx].done) {
>> +            comp_param[idx].done = false;
>> +            bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
>> +            qemu_mutex_lock(&comp_param[idx].mutex);
>> +            set_compress_params(&comp_param[idx], block, offset);
>> +            qemu_cond_signal(&comp_param[idx].cond);
>> +            qemu_mutex_unlock(&comp_param[idx].mutex);
>> +            pages = 1;
>> +            ram_counters.normal++;
>> +            ram_counters.transferred += bytes_xmit;
>>               break;
>> -        } else {
>> -            qemu_cond_wait(&comp_done_cond, &comp_done_lock);
>>           }
>>       }
>> +
>> +    /*
>> +     * if there is no thread is free to compress the data and the user
>> +     * really expects the slowdown, wait it.
> 
> Considering [1] above, IMHO it might not really be a slow down but it
> depends.  Maybe only mentioning about the fact that we're sending a
> normal page instead of the compressed page if wait is not specified.
> 

Okay, will update the comments based on your suggestion.

>> +     */
>> +    if (pages < 0 && wait) {
>> +        qemu_cond_wait(&comp_done_cond, &comp_done_lock);
>> +        goto retry;
>> +    }
>>       qemu_mutex_unlock(&comp_done_lock);
>>   
>>       return pages;
>> @@ -2226,7 +2230,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>        * CPU resource.
>>        */
>>       if (block == rs->last_sent_block && save_page_use_compression(rs)) {
>> -        return compress_page_with_multi_thread(rs, block, offset);
>> +        res = compress_page_with_multi_thread(rs, block, offset);
>> +        if (res > 0) {
>> +            return res;
>> +        }
>>       } else if (migrate_use_multifd()) {
>>           return ram_save_multifd_page(rs, block, offset);
>>       }
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 186e8a7303..b4f394844b 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -462,6 +462,11 @@
>>   # @compress-threads: Set compression thread count to be used in live migration,
>>   #          the compression thread count is an integer between 1 and 255.
>>   #
>> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
>> +#          if it's enabled, otherwise, the page will be posted out immediately
>> +#          in the main thread without compression. It's off on default.
>> +#          (Since: 3.0)
>> +#
> 
> Should "Since 3.1" in all the places.
> 

Oh... the thing goes faster than i realized :)

> We'll need to touch up the "by default" part depending on whether we'd
> need to change it according to above comment.
> 
> Otherwise it looks good to me.
> 

Okay, thank you, Peter.

Re: [Qemu-devel] [PATCH v2 1/8] migration: do not wait for free thread

[Eric Blake]

On 07/19/2018 07:15 AM, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Instead of putting the main thread to sleep state to wait for
> free compression thread, we can directly post it out as normal
> page that reduces the latency and uses CPUs more efficiently
> 
> A parameter, compress-wait-thread, is introduced, it can be
> enabled if the user really wants the old behavior
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>   hmp.c                 |  8 ++++++++
>   migration/migration.c | 21 +++++++++++++++++++++
>   migration/migration.h |  1 +
>   migration/ram.c       | 45 ++++++++++++++++++++++++++-------------------
>   qapi/migration.json   | 23 ++++++++++++++++++-----
>   5 files changed, 74 insertions(+), 24 deletions(-)
> 

> +++ b/qapi/migration.json
> @@ -462,6 +462,11 @@
>   # @compress-threads: Set compression thread count to be used in live migration,
>   #          the compression thread count is an integer between 1 and 255.
>   #
> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
> +#          if it's enabled, otherwise, the page will be posted out immediately
> +#          in the main thread without compression. It's off on default.
> +#          (Since: 3.0)

Is this a bug fix? It's awfully late in the release cycle to be adding 
new features; is this something that we can live without until 3.1?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 1/8] migration: do not wait for free thread

[Xiao Guangrong]

On 07/24/2018 02:36 AM, Eric Blake wrote:
> On 07/19/2018 07:15 AM, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> Instead of putting the main thread to sleep state to wait for
>> free compression thread, we can directly post it out as normal
>> page that reduces the latency and uses CPUs more efficiently
>>
>> A parameter, compress-wait-thread, is introduced, it can be
>> enabled if the user really wants the old behavior
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   hmp.c                 |  8 ++++++++
>>   migration/migration.c | 21 +++++++++++++++++++++
>>   migration/migration.h |  1 +
>>   migration/ram.c       | 45 ++++++++++++++++++++++++++-------------------
>>   qapi/migration.json   | 23 ++++++++++++++++++-----
>>   5 files changed, 74 insertions(+), 24 deletions(-)
>>
> 
>> +++ b/qapi/migration.json
>> @@ -462,6 +462,11 @@
>>   # @compress-threads: Set compression thread count to be used in live migration,
>>   #          the compression thread count is an integer between 1 and 255.
>>   #
>> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
>> +#          if it's enabled, otherwise, the page will be posted out immediately
>> +#          in the main thread without compression. It's off on default.
>> +#          (Since: 3.0)
> 
> Is this a bug fix? It's awfully late in the release cycle to be adding new features; is this something that we can live without until 3.1?
> 

It's performance improvement, i think it is not urgent. :)

[Qemu-devel] [PATCH v2 2/8] migration: fix counting normal page for compression

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

The compressed page is not normal page

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 0ad234c692..1b016e048d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1903,7 +1903,6 @@ retry:
             qemu_cond_signal(&comp_param[idx].cond);
             qemu_mutex_unlock(&comp_param[idx].mutex);
             pages = 1;
-            ram_counters.normal++;
             ram_counters.transferred += bytes_xmit;
             break;
         }
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 2/8] migration: fix counting normal page for compression

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:14PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> The compressed page is not normal page
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>

I think it'll depend on how we are defining the "normal" page.  AFAIU
it's the count of raw pages, then I think it's correct.

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

> ---
>  migration/ram.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0ad234c692..1b016e048d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1903,7 +1903,6 @@ retry:
>              qemu_cond_signal(&comp_param[idx].cond);
>              qemu_mutex_unlock(&comp_param[idx].mutex);
>              pages = 1;
> -            ram_counters.normal++;
>              ram_counters.transferred += bytes_xmit;
>              break;
>          }
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Currently, it includes:
pages: amount of pages compressed and transferred to the target VM
busy: amount of count that no free thread to compress data
busy-rate: rate of thread busy
reduced-size: amount of bytes reduced by compression
compression-rate: rate of compressed size

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hmp.c                 | 13 +++++++++++++
 migration/migration.c | 11 +++++++++++
 migration/ram.c       | 37 +++++++++++++++++++++++++++++++++++++
 migration/ram.h       |  1 +
 qapi/migration.json   | 26 +++++++++++++++++++++++++-
 5 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index 47d36e3ccf..a9e2776d60 100644
--- a/hmp.c
+++ b/hmp.c
@@ -271,6 +271,19 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->overflow);
     }
 
+    if (info->has_compression) {
+        monitor_printf(mon, "compression pages: %" PRIu64 " pages\n",
+                       info->compression->pages);
+        monitor_printf(mon, "compression busy: %" PRIu64 "\n",
+                       info->compression->busy);
+        monitor_printf(mon, "compression busy rate: %0.2f\n",
+                       info->compression->busy_rate);
+        monitor_printf(mon, "compression reduced size: %" PRIu64 "\n",
+                       info->compression->reduced_size);
+        monitor_printf(mon, "compression rate: %0.2f\n",
+                       info->compression->compression_rate);
+    }
+
     if (info->has_cpu_throttle_percentage) {
         monitor_printf(mon, "cpu throttle percentage: %" PRIu64 "\n",
                        info->cpu_throttle_percentage);
diff --git a/migration/migration.c b/migration/migration.c
index 0af75465b3..8fedf13889 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -754,6 +754,17 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->xbzrle_cache->overflow = xbzrle_counters.overflow;
     }
 
+    if (migrate_use_compression()) {
+        info->has_compression = true;
+        info->compression = g_malloc0(sizeof(*info->compression));
+        info->compression->pages = compression_counters.pages;
+        info->compression->busy = compression_counters.busy;
+        info->compression->busy_rate = compression_counters.busy_rate;
+        info->compression->reduced_size = compression_counters.reduced_size;
+        info->compression->compression_rate =
+                                    compression_counters.compression_rate;
+    }
+
     if (cpu_throttle_active()) {
         info->has_cpu_throttle_percentage = true;
         info->cpu_throttle_percentage = cpu_throttle_get_percentage();
diff --git a/migration/ram.c b/migration/ram.c
index 1b016e048d..e68b0e6dec 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -300,6 +300,15 @@ struct RAMState {
     uint64_t num_dirty_pages_period;
     /* xbzrle misses since the beginning of the period */
     uint64_t xbzrle_cache_miss_prev;
+
+    /* compression statistics since the beginning of the period */
+    /* amount of count that no free thread to compress data */
+    uint64_t compress_thread_busy_prev;
+    /* amount bytes reduced by compression */
+    uint64_t compress_reduced_size_prev;
+    /* amount of compressed pages */
+    uint64_t compress_pages_prev;
+
     /* number of iterations at the beginning of period */
     uint64_t iterations_prev;
     /* Iterations since start */
@@ -337,6 +346,8 @@ struct PageSearchStatus {
 };
 typedef struct PageSearchStatus PageSearchStatus;
 
+CompressionStats compression_counters;
+
 struct CompressParam {
     bool done;
     bool quit;
@@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
             rs->xbzrle_cache_miss_prev) / iter_count;
         rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
     }
+
+    if (migrate_use_compression()) {
+        uint64_t comp_pages;
+
+        compression_counters.busy_rate = (double)(compression_counters.busy -
+            rs->compress_thread_busy_prev) / iter_count;
+        rs->compress_thread_busy_prev = compression_counters.busy;
+
+        comp_pages = compression_counters.pages - rs->compress_pages_prev;
+        if (comp_pages) {
+            compression_counters.compression_rate =
+                (double)(compression_counters.reduced_size -
+                rs->compress_reduced_size_prev) /
+                (comp_pages * TARGET_PAGE_SIZE);
+            rs->compress_pages_prev = compression_counters.pages;
+            rs->compress_reduced_size_prev = compression_counters.reduced_size;
+        }
+    }
 }
 
 static void migration_bitmap_sync(RAMState *rs)
@@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
         qemu_mutex_lock(&comp_param[idx].mutex);
         if (!comp_param[idx].quit) {
             len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
+            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
+            compression_counters.pages++;
             ram_counters.transferred += len;
         }
         qemu_mutex_unlock(&comp_param[idx].mutex);
@@ -1903,6 +1935,10 @@ retry:
             qemu_cond_signal(&comp_param[idx].cond);
             qemu_mutex_unlock(&comp_param[idx].mutex);
             pages = 1;
+            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+            compression_counters.reduced_size += TARGET_PAGE_SIZE -
+                                                 bytes_xmit + 8;
+            compression_counters.pages++;
             ram_counters.transferred += bytes_xmit;
             break;
         }
@@ -2233,6 +2269,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
         if (res > 0) {
             return res;
         }
+        compression_counters.busy++;
     } else if (migrate_use_multifd()) {
         return ram_save_multifd_page(rs, block, offset);
     }
diff --git a/migration/ram.h b/migration/ram.h
index 457bf54b8c..a139066846 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -36,6 +36,7 @@
 
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
+extern CompressionStats compression_counters;
 
 int xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index b4f394844b..5a34aa1754 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -75,6 +75,27 @@
            'cache-miss': 'int', 'cache-miss-rate': 'number',
            'overflow': 'int' } }
 
+##
+# @CompressionStats:
+#
+# Detailed migration compression statistics
+#
+# @pages: amount of pages compressed and transferred to the target VM
+#
+# @busy: count of times that no free thread was available to compress data
+#
+# @busy-rate: rate of thread busy
+#
+# @reduced-size: amount of bytes reduced by compression
+#
+# @compression-rate: rate of compressed size
+#
+# Since: 3.0
+##
+{ 'struct': 'CompressionStats',
+  'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
+	   'reduced-size': 'int', 'compression-rate': 'number' } }
+
 ##
 # @MigrationStatus:
 #
@@ -172,6 +193,8 @@
 #           only present when the postcopy-blocktime migration capability
 #           is enabled. (Since 3.0)
 #
+# @compression: migration compression statistics, only returned if compression
+#           feature is on and status is 'active' or 'completed' (Since 3.0)
 #
 # Since: 0.14.0
 ##
@@ -186,7 +209,8 @@
            '*cpu-throttle-percentage': 'int',
            '*error-desc': 'str',
            '*postcopy-blocktime' : 'uint32',
-           '*postcopy-vcpu-blocktime': ['uint32']} }
+           '*postcopy-vcpu-blocktime': ['uint32'],
+           '*compression': 'CompressionStats'} }
 
 ##
 # @query-migrate:
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>              rs->xbzrle_cache_miss_prev) / iter_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>      }
> +
> +    if (migrate_use_compression()) {
> +        uint64_t comp_pages;
> +
> +        compression_counters.busy_rate = (double)(compression_counters.busy -
> +            rs->compress_thread_busy_prev) / iter_count;

Here I'm not sure it's correct...

"iter_count" stands for ramstate.iterations.  It's increased per
ram_find_and_save_block(), so IMHO it might contain multiple guest
pages.  However compression_counters.busy should be per guest page.

> +        rs->compress_thread_busy_prev = compression_counters.busy;
> +
> +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
> +        if (comp_pages) {
> +            compression_counters.compression_rate =
> +                (double)(compression_counters.reduced_size -
> +                rs->compress_reduced_size_prev) /
> +                (comp_pages * TARGET_PAGE_SIZE);
> +            rs->compress_pages_prev = compression_counters.pages;
> +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
> +        }
> +    }
>  }
>  
>  static void migration_bitmap_sync(RAMState *rs)
> @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
>          qemu_mutex_lock(&comp_param[idx].mutex);
>          if (!comp_param[idx].quit) {
>              len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;

I would agree with Dave here - why we store the "reduced size" instead
of the size of the compressed data (which I think should be len - 8)?

Meanwhile, would a helper be nicer? Like:

        void migrate_compressed_page_stats_update(int xfer_size)
        {
                /* Removing the offset header in save_page_header() */
                compression_counters.compressed_size = xfer_size - 8;
                compression_counters.pages++;
                ram_counters.transferred += bytes_xmit;
        }

> +            compression_counters.pages++;
>              ram_counters.transferred += len;
>          }
>          qemu_mutex_unlock(&comp_param[idx].mutex);
> @@ -1903,6 +1935,10 @@ retry:
>              qemu_cond_signal(&comp_param[idx].cond);
>              qemu_mutex_unlock(&comp_param[idx].mutex);
>              pages = 1;
> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +            compression_counters.reduced_size += TARGET_PAGE_SIZE -
> +                                                 bytes_xmit + 8;
> +            compression_counters.pages++;
>              ram_counters.transferred += bytes_xmit;
>              break;
>          }

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Xiao Guangrong]

On 07/23/2018 12:36 PM, Peter Xu wrote:
> On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
>> @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>>               rs->xbzrle_cache_miss_prev) / iter_count;
>>           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>>       }
>> +
>> +    if (migrate_use_compression()) {
>> +        uint64_t comp_pages;
>> +
>> +        compression_counters.busy_rate = (double)(compression_counters.busy -
>> +            rs->compress_thread_busy_prev) / iter_count;
> 
> Here I'm not sure it's correct...
> 
> "iter_count" stands for ramstate.iterations.  It's increased per
> ram_find_and_save_block(), so IMHO it might contain multiple guest

ram_find_and_save_block() returns if a page is successfully posted and
it only posts 1 page out at one time.

> pages.  However compression_counters.busy should be per guest page.
> 

Actually, it's derived from xbzrle_counters.cache_miss_rate:
         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
             rs->xbzrle_cache_miss_prev) / iter_count;

>> +        rs->compress_thread_busy_prev = compression_counters.busy;
>> +
>> +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
>> +        if (comp_pages) {
>> +            compression_counters.compression_rate =
>> +                (double)(compression_counters.reduced_size -
>> +                rs->compress_reduced_size_prev) /
>> +                (comp_pages * TARGET_PAGE_SIZE);
>> +            rs->compress_pages_prev = compression_counters.pages;
>> +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
>> +        }
>> +    }
>>   }
>>   
>>   static void migration_bitmap_sync(RAMState *rs)
>> @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
>>           qemu_mutex_lock(&comp_param[idx].mutex);
>>           if (!comp_param[idx].quit) {
>>               len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
>> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>> +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
> 
> I would agree with Dave here - why we store the "reduced size" instead
> of the size of the compressed data (which I think should be len - 8)?
> 

len-8 is the size of data after compressed rather than the data improved
by compression that is not straightforward for the user to see how much
the improvement is by applying compression.

Hmm... but it is not a big deal to me... :)

> Meanwhile, would a helper be nicer? Like:

Yup, that's nicer indeed.

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Peter Xu]

On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
> 
> 
> On 07/23/2018 12:36 PM, Peter Xu wrote:
> > On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> > > @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
> > >               rs->xbzrle_cache_miss_prev) / iter_count;
> > >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > >       }
> > > +
> > > +    if (migrate_use_compression()) {
> > > +        uint64_t comp_pages;
> > > +
> > > +        compression_counters.busy_rate = (double)(compression_counters.busy -
> > > +            rs->compress_thread_busy_prev) / iter_count;
> > 
> > Here I'm not sure it's correct...
> > 
> > "iter_count" stands for ramstate.iterations.  It's increased per
> > ram_find_and_save_block(), so IMHO it might contain multiple guest
> 
> ram_find_and_save_block() returns if a page is successfully posted and
> it only posts 1 page out at one time.

ram_find_and_save_block() calls ram_save_host_page(), and we should be
sending multiple guest pages in ram_save_host_page() if the host page
is a huge page?

> 
> > pages.  However compression_counters.busy should be per guest page.
> > 
> 
> Actually, it's derived from xbzrle_counters.cache_miss_rate:
>         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>             rs->xbzrle_cache_miss_prev) / iter_count;

Then this is suspecious to me too...

> 
> > > +        rs->compress_thread_busy_prev = compression_counters.busy;
> > > +
> > > +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
> > > +        if (comp_pages) {
> > > +            compression_counters.compression_rate =
> > > +                (double)(compression_counters.reduced_size -
> > > +                rs->compress_reduced_size_prev) /
> > > +                (comp_pages * TARGET_PAGE_SIZE);
> > > +            rs->compress_pages_prev = compression_counters.pages;
> > > +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
> > > +        }
> > > +    }
> > >   }
> > >   static void migration_bitmap_sync(RAMState *rs)
> > > @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
> > >           qemu_mutex_lock(&comp_param[idx].mutex);
> > >           if (!comp_param[idx].quit) {
> > >               len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> > > +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> > > +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
> > 
> > I would agree with Dave here - why we store the "reduced size" instead
> > of the size of the compressed data (which I think should be len - 8)?
> > 
> 
> len-8 is the size of data after compressed rather than the data improved
> by compression that is not straightforward for the user to see how much
> the improvement is by applying compression.
> 
> Hmm... but it is not a big deal to me... :)

Yeah it might be a personal preference indeed. :)

It's just natural to do that this way for me since AFAIU the
compression ratio is defined as:

                           compressed data size
  compression ratio =    ------------------------
                            original data size

> 
> > Meanwhile, would a helper be nicer? Like:
> 
> Yup, that's nicer indeed.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Xiao Guangrong]

On 07/23/2018 04:05 PM, Peter Xu wrote:
> On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 07/23/2018 12:36 PM, Peter Xu wrote:
>>> On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
>>>> @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>>>>                rs->xbzrle_cache_miss_prev) / iter_count;
>>>>            rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>>>>        }
>>>> +
>>>> +    if (migrate_use_compression()) {
>>>> +        uint64_t comp_pages;
>>>> +
>>>> +        compression_counters.busy_rate = (double)(compression_counters.busy -
>>>> +            rs->compress_thread_busy_prev) / iter_count;
>>>
>>> Here I'm not sure it's correct...
>>>
>>> "iter_count" stands for ramstate.iterations.  It's increased per
>>> ram_find_and_save_block(), so IMHO it might contain multiple guest
>>
>> ram_find_and_save_block() returns if a page is successfully posted and
>> it only posts 1 page out at one time.
> 
> ram_find_and_save_block() calls ram_save_host_page(), and we should be
> sending multiple guest pages in ram_save_host_page() if the host page
> is a huge page?
> 

You're right, thank you for pointing it out.

So, how about introduce a filed, posted_pages, into RAMState that is used
to track total pages posted out.

Then will use this filed to replace 'iter_count' for compression and use
'RAMState.posted_pages - ram_counters.duplicate' to calculate
xbzrle_cache_miss as the zero page is not handled by xbzrle.

Or introduce a new function, total_posted_pages, which returns the
sum of all page counts:

    static total_posted_pages(void)
    {
        return ram_counters.normal + ram_counters.duplicate + compression_counters.pages
               +  xbzrle_counters.pages;
    }

that would be a bit more complexity...

>>
>>> pages.  However compression_counters.busy should be per guest page.
>>>
>>
>> Actually, it's derived from xbzrle_counters.cache_miss_rate:
>>          xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>>              rs->xbzrle_cache_miss_prev) / iter_count;
> 
> Then this is suspecious to me too...
> 
>>
>>>> +        rs->compress_thread_busy_prev = compression_counters.busy;
>>>> +
>>>> +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
>>>> +        if (comp_pages) {
>>>> +            compression_counters.compression_rate =
>>>> +                (double)(compression_counters.reduced_size -
>>>> +                rs->compress_reduced_size_prev) /
>>>> +                (comp_pages * TARGET_PAGE_SIZE);
>>>> +            rs->compress_pages_prev = compression_counters.pages;
>>>> +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
>>>> +        }
>>>> +    }
>>>>    }
>>>>    static void migration_bitmap_sync(RAMState *rs)
>>>> @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
>>>>            qemu_mutex_lock(&comp_param[idx].mutex);
>>>>            if (!comp_param[idx].quit) {
>>>>                len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
>>>> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>>>> +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
>>>
>>> I would agree with Dave here - why we store the "reduced size" instead
>>> of the size of the compressed data (which I think should be len - 8)?
>>>
>>
>> len-8 is the size of data after compressed rather than the data improved
>> by compression that is not straightforward for the user to see how much
>> the improvement is by applying compression.
>>
>> Hmm... but it is not a big deal to me... :)
> 
> Yeah it might be a personal preference indeed. :)
> 
> It's just natural to do that this way for me since AFAIU the
> compression ratio is defined as:
> 
>                             compressed data size
>    compression ratio =    ------------------------
>                              original data size
> 

Er, we do it as following:
             compression_counters.compression_rate =
                 (double)(compression_counters.reduced_size -
                 rs->compress_reduced_size_prev) /
                 (comp_pages * TARGET_PAGE_SIZE);

We use reduced_size, i.e,:

                              original data size - compressed data size
     compression ratio =    ------------------------
                               original data size

for example, for 100 bytes raw data, if we posted 99 bytes out, then
the compression ration should be 1%.

So if i understand correctly, the reduced_size is really you want? :)

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Peter Xu]

On Mon, Jul 23, 2018 at 04:40:29PM +0800, Xiao Guangrong wrote:
> 
> 
> On 07/23/2018 04:05 PM, Peter Xu wrote:
> > On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
> > > 
> > > 
> > > On 07/23/2018 12:36 PM, Peter Xu wrote:
> > > > On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> > > > > @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
> > > > >                rs->xbzrle_cache_miss_prev) / iter_count;
> > > > >            rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > > > >        }
> > > > > +
> > > > > +    if (migrate_use_compression()) {
> > > > > +        uint64_t comp_pages;
> > > > > +
> > > > > +        compression_counters.busy_rate = (double)(compression_counters.busy -
> > > > > +            rs->compress_thread_busy_prev) / iter_count;
> > > > 
> > > > Here I'm not sure it's correct...
> > > > 
> > > > "iter_count" stands for ramstate.iterations.  It's increased per
> > > > ram_find_and_save_block(), so IMHO it might contain multiple guest
> > > 
> > > ram_find_and_save_block() returns if a page is successfully posted and
> > > it only posts 1 page out at one time.
> > 
> > ram_find_and_save_block() calls ram_save_host_page(), and we should be
> > sending multiple guest pages in ram_save_host_page() if the host page
> > is a huge page?
> > 
> 
> You're right, thank you for pointing it out.
> 
> So, how about introduce a filed, posted_pages, into RAMState that is used
> to track total pages posted out.
> 
> Then will use this filed to replace 'iter_count' for compression and use
> 'RAMState.posted_pages - ram_counters.duplicate' to calculate
> xbzrle_cache_miss as the zero page is not handled by xbzrle.
> 
> Or introduce a new function, total_posted_pages, which returns the
> sum of all page counts:
> 
>    static total_posted_pages(void)
>    {
>        return ram_counters.normal + ram_counters.duplicate + compression_counters.pages
>               +  xbzrle_counters.pages;
>    }
> 
> that would be a bit more complexity...

If below [1] is wrong too, then I'm thinking whether we could just
move the rs->iterations++ from ram_save_iterate() loop to
ram_save_host_page() loop, then we possibly fix both places...

After all I don't see any other usages of rs->iterations so it seems
fine.  Dave/Juan?

> 
> > > 
> > > > pages.  However compression_counters.busy should be per guest page.
> > > > 
> > > 
> > > Actually, it's derived from xbzrle_counters.cache_miss_rate:
> > >          xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
> > >              rs->xbzrle_cache_miss_prev) / iter_count;
> > 
> > Then this is suspecious to me too...

[1]

> > 
> > > 
> > > > > +        rs->compress_thread_busy_prev = compression_counters.busy;
> > > > > +
> > > > > +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
> > > > > +        if (comp_pages) {
> > > > > +            compression_counters.compression_rate =
> > > > > +                (double)(compression_counters.reduced_size -
> > > > > +                rs->compress_reduced_size_prev) /
> > > > > +                (comp_pages * TARGET_PAGE_SIZE);
> > > > > +            rs->compress_pages_prev = compression_counters.pages;
> > > > > +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
> > > > > +        }
> > > > > +    }
> > > > >    }
> > > > >    static void migration_bitmap_sync(RAMState *rs)
> > > > > @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
> > > > >            qemu_mutex_lock(&comp_param[idx].mutex);
> > > > >            if (!comp_param[idx].quit) {
> > > > >                len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> > > > > +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> > > > > +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
> > > > 
> > > > I would agree with Dave here - why we store the "reduced size" instead
> > > > of the size of the compressed data (which I think should be len - 8)?
> > > > 
> > > 
> > > len-8 is the size of data after compressed rather than the data improved
> > > by compression that is not straightforward for the user to see how much
> > > the improvement is by applying compression.
> > > 
> > > Hmm... but it is not a big deal to me... :)
> > 
> > Yeah it might be a personal preference indeed. :)
> > 
> > It's just natural to do that this way for me since AFAIU the
> > compression ratio is defined as:
> > 
> >                             compressed data size
> >    compression ratio =    ------------------------
> >                              original data size
> > 
> 
> Er, we do it as following:
>             compression_counters.compression_rate =
>                 (double)(compression_counters.reduced_size -
>                 rs->compress_reduced_size_prev) /
>                 (comp_pages * TARGET_PAGE_SIZE);
> 
> We use reduced_size, i.e,:
> 
>                              original data size - compressed data size
>     compression ratio =    ------------------------
>                               original data size
> 
> for example, for 100 bytes raw data, if we posted 99 bytes out, then
> the compression ration should be 1%.

I think it should be 99%...

> 
> So if i understand correctly, the reduced_size is really you want? :)
> 

Here's the "offical" definition. :)

https://en.wikipedia.org/wiki/Data_compression_ratio

But obviously I reverted the molecular and denominator... So maybe we
can follow what the wiki page said (then I think you'll just store the
summation of len-8)?

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Xiao Guangrong]

On 07/23/2018 05:15 PM, Peter Xu wrote:
> On Mon, Jul 23, 2018 at 04:40:29PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 07/23/2018 04:05 PM, Peter Xu wrote:
>>> On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
>>>>
>>>>
>>>> On 07/23/2018 12:36 PM, Peter Xu wrote:
>>>>> On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
>>>>>> @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>>>>>>                 rs->xbzrle_cache_miss_prev) / iter_count;
>>>>>>             rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>>>>>>         }
>>>>>> +
>>>>>> +    if (migrate_use_compression()) {
>>>>>> +        uint64_t comp_pages;
>>>>>> +
>>>>>> +        compression_counters.busy_rate = (double)(compression_counters.busy -
>>>>>> +            rs->compress_thread_busy_prev) / iter_count;
>>>>>
>>>>> Here I'm not sure it's correct...
>>>>>
>>>>> "iter_count" stands for ramstate.iterations.  It's increased per
>>>>> ram_find_and_save_block(), so IMHO it might contain multiple guest
>>>>
>>>> ram_find_and_save_block() returns if a page is successfully posted and
>>>> it only posts 1 page out at one time.
>>>
>>> ram_find_and_save_block() calls ram_save_host_page(), and we should be
>>> sending multiple guest pages in ram_save_host_page() if the host page
>>> is a huge page?
>>>
>>
>> You're right, thank you for pointing it out.
>>
>> So, how about introduce a filed, posted_pages, into RAMState that is used
>> to track total pages posted out.
>>
>> Then will use this filed to replace 'iter_count' for compression and use
>> 'RAMState.posted_pages - ram_counters.duplicate' to calculate
>> xbzrle_cache_miss as the zero page is not handled by xbzrle.
>>
>> Or introduce a new function, total_posted_pages, which returns the
>> sum of all page counts:
>>
>>     static total_posted_pages(void)
>>     {
>>         return ram_counters.normal + ram_counters.duplicate + compression_counters.pages
>>                +  xbzrle_counters.pages;
>>     }
>>
>> that would be a bit more complexity...
> 
> If below [1] is wrong too, then I'm thinking whether we could just
> move the rs->iterations++ from ram_save_iterate() loop to
> ram_save_host_page() loop, then we possibly fix both places...
> 

Beside that, even if we fix iterations, xbzrle is painful still as
the zero should not be counted in the cache miss, i.e, xbzrle does
not handle zero page at all.

Anyway, fixing iterations is a good start. :)

> After all I don't see any other usages of rs->iterations so it seems
> fine.  Dave/Juan?
> 
>>
>>>>
>>>>> pages.  However compression_counters.busy should be per guest page.
>>>>>
>>>>
>>>> Actually, it's derived from xbzrle_counters.cache_miss_rate:
>>>>           xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>>>>               rs->xbzrle_cache_miss_prev) / iter_count;
>>>
>>> Then this is suspecious to me too...
> 
> [1]
> 
>>>
>>>>
>>>>>> +        rs->compress_thread_busy_prev = compression_counters.busy;
>>>>>> +
>>>>>> +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
>>>>>> +        if (comp_pages) {
>>>>>> +            compression_counters.compression_rate =
>>>>>> +                (double)(compression_counters.reduced_size -
>>>>>> +                rs->compress_reduced_size_prev) /
>>>>>> +                (comp_pages * TARGET_PAGE_SIZE);
>>>>>> +            rs->compress_pages_prev = compression_counters.pages;
>>>>>> +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
>>>>>> +        }
>>>>>> +    }
>>>>>>     }
>>>>>>     static void migration_bitmap_sync(RAMState *rs)
>>>>>> @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
>>>>>>             qemu_mutex_lock(&comp_param[idx].mutex);
>>>>>>             if (!comp_param[idx].quit) {
>>>>>>                 len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
>>>>>> +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>>>>>> +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
>>>>>
>>>>> I would agree with Dave here - why we store the "reduced size" instead
>>>>> of the size of the compressed data (which I think should be len - 8)?
>>>>>
>>>>
>>>> len-8 is the size of data after compressed rather than the data improved
>>>> by compression that is not straightforward for the user to see how much
>>>> the improvement is by applying compression.
>>>>
>>>> Hmm... but it is not a big deal to me... :)
>>>
>>> Yeah it might be a personal preference indeed. :)
>>>
>>> It's just natural to do that this way for me since AFAIU the
>>> compression ratio is defined as:
>>>
>>>                              compressed data size
>>>     compression ratio =    ------------------------
>>>                               original data size
>>>
>>
>> Er, we do it as following:
>>              compression_counters.compression_rate =
>>                  (double)(compression_counters.reduced_size -
>>                  rs->compress_reduced_size_prev) /
>>                  (comp_pages * TARGET_PAGE_SIZE);
>>
>> We use reduced_size, i.e,:
>>
>>                               original data size - compressed data size
>>      compression ratio =    ------------------------
>>                                original data size
>>
>> for example, for 100 bytes raw data, if we posted 99 bytes out, then
>> the compression ration should be 1%.
> 
> I think it should be 99%...
> 
>>
>> So if i understand correctly, the reduced_size is really you want? :)
>>
> 
> Here's the "offical" definition. :)
> 
> https://en.wikipedia.org/wiki/Data_compression_ratio
> 
> But obviously I reverted the molecular and denominator... So maybe we
> can follow what the wiki page said (then I think you'll just store the
> summation of len-8)?

Thank you for fixing my knowledge, what i did is "spacing saving" rather
than "compression ratio". As this "compression ratio" is popularly used
in compression benchmarks, then your suggestion is fine to me.

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
> > 
> > 
> > On 07/23/2018 12:36 PM, Peter Xu wrote:
> > > On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> > > > @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
> > > >               rs->xbzrle_cache_miss_prev) / iter_count;
> > > >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > > >       }
> > > > +
> > > > +    if (migrate_use_compression()) {
> > > > +        uint64_t comp_pages;
> > > > +
> > > > +        compression_counters.busy_rate = (double)(compression_counters.busy -
> > > > +            rs->compress_thread_busy_prev) / iter_count;
> > > 
> > > Here I'm not sure it's correct...
> > > 
> > > "iter_count" stands for ramstate.iterations.  It's increased per
> > > ram_find_and_save_block(), so IMHO it might contain multiple guest
> > 
> > ram_find_and_save_block() returns if a page is successfully posted and
> > it only posts 1 page out at one time.
> 
> ram_find_and_save_block() calls ram_save_host_page(), and we should be
> sending multiple guest pages in ram_save_host_page() if the host page
> is a huge page?
> 
> > 
> > > pages.  However compression_counters.busy should be per guest page.
> > > 
> > 
> > Actually, it's derived from xbzrle_counters.cache_miss_rate:
> >         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
> >             rs->xbzrle_cache_miss_prev) / iter_count;
> 
> Then this is suspecious to me too...

Actually; I think this isn't totally wrong;  iter_count is the *difference* in
iterations since the last time it was updated:

   uint64_t iter_count = rs->iterations - rs->iterations_prev;

        xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
            rs->xbzrle_cache_miss_prev) / iter_count;

so this is:
      cache-misses-since-last-update
      ------------------------------
        iterations since last-update

so the 'miss_rate' is ~misses / iteration.
Although that doesn't really correspond to time.

Dave

> > 
> > > > +        rs->compress_thread_busy_prev = compression_counters.busy;
> > > > +
> > > > +        comp_pages = compression_counters.pages - rs->compress_pages_prev;
> > > > +        if (comp_pages) {
> > > > +            compression_counters.compression_rate =
> > > > +                (double)(compression_counters.reduced_size -
> > > > +                rs->compress_reduced_size_prev) /
> > > > +                (comp_pages * TARGET_PAGE_SIZE);
> > > > +            rs->compress_pages_prev = compression_counters.pages;
> > > > +            rs->compress_reduced_size_prev = compression_counters.reduced_size;
> > > > +        }
> > > > +    }
> > > >   }
> > > >   static void migration_bitmap_sync(RAMState *rs)
> > > > @@ -1872,6 +1901,9 @@ static void flush_compressed_data(RAMState *rs)
> > > >           qemu_mutex_lock(&comp_param[idx].mutex);
> > > >           if (!comp_param[idx].quit) {
> > > >               len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> > > > +            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> > > > +            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
> > > 
> > > I would agree with Dave here - why we store the "reduced size" instead
> > > of the size of the compressed data (which I think should be len - 8)?
> > > 
> > 
> > len-8 is the size of data after compressed rather than the data improved
> > by compression that is not straightforward for the user to see how much
> > the improvement is by applying compression.
> > 
> > Hmm... but it is not a big deal to me... :)
> 
> Yeah it might be a personal preference indeed. :)
> 
> It's just natural to do that this way for me since AFAIU the
> compression ratio is defined as:
> 
>                            compressed data size
>   compression ratio =    ------------------------
>                             original data size
> 
> > 
> > > Meanwhile, would a helper be nicer? Like:
> > 
> > Yup, that's nicer indeed.
> 
> Regards,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/8] migration: show the statistics of compression

[Peter Xu]

On Wed, Jul 25, 2018 at 05:44:02PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Mon, Jul 23, 2018 at 03:39:18PM +0800, Xiao Guangrong wrote:
> > > 
> > > 
> > > On 07/23/2018 12:36 PM, Peter Xu wrote:
> > > > On Thu, Jul 19, 2018 at 08:15:15PM +0800, guangrong.xiao@gmail.com wrote:
> > > > > @@ -1597,6 +1608,24 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
> > > > >               rs->xbzrle_cache_miss_prev) / iter_count;
> > > > >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > > > >       }
> > > > > +
> > > > > +    if (migrate_use_compression()) {
> > > > > +        uint64_t comp_pages;
> > > > > +
> > > > > +        compression_counters.busy_rate = (double)(compression_counters.busy -
> > > > > +            rs->compress_thread_busy_prev) / iter_count;
> > > > 
> > > > Here I'm not sure it's correct...
> > > > 
> > > > "iter_count" stands for ramstate.iterations.  It's increased per
> > > > ram_find_and_save_block(), so IMHO it might contain multiple guest
> > > 
> > > ram_find_and_save_block() returns if a page is successfully posted and
> > > it only posts 1 page out at one time.
> > 
> > ram_find_and_save_block() calls ram_save_host_page(), and we should be
> > sending multiple guest pages in ram_save_host_page() if the host page
> > is a huge page?
> > 
> > > 
> > > > pages.  However compression_counters.busy should be per guest page.
> > > > 
> > > 
> > > Actually, it's derived from xbzrle_counters.cache_miss_rate:
> > >         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
> > >             rs->xbzrle_cache_miss_prev) / iter_count;
> > 
> > Then this is suspecious to me too...
> 
> Actually; I think this isn't totally wrong;  iter_count is the *difference* in
> iterations since the last time it was updated:
> 
>    uint64_t iter_count = rs->iterations - rs->iterations_prev;
> 
>         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>             rs->xbzrle_cache_miss_prev) / iter_count;
> 
> so this is:
>       cache-misses-since-last-update
>       ------------------------------
>         iterations since last-update
> 
> so the 'miss_rate' is ~misses / iteration.
> Although that doesn't really correspond to time.

I'm not sure I got the idea here, the thing is that I think the
counters are for different granularities which might be problematic:

- xbzrle_counters.cache_miss is done in save_xbzrle_page(), so it's
  per-guest-page granularity

- RAMState.iterations is done for each ram_find_and_save_block(), so
  it's per-host-page granularity

An example is that when we migrate a 2M huge page in the guest, we
will only increase the RAMState.iterations by 1 (since
ram_find_and_save_block() will be called once), but we might increase
xbzrle_counters.cache_miss for 2M/4K=512 times (we'll call
save_xbzrle_page() that many times) if all the pages got cache miss.
Then IMHO the cache miss rate will be 512/1=51200% (while it should
actually be just 100% cache miss).

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 4/8] migration: introduce save_zero_page_to_file

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

It will be used by the compression threads

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index e68b0e6dec..ce6e69b649 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1696,27 +1696,47 @@ static void migration_bitmap_sync(RAMState *rs)
 /**
  * save_zero_page: send the zero page to the stream
  *
- * Returns the number of pages written.
+ * Returns the size of data written to the file, 0 means the page is not
+ * a zero page
  *
  * @rs: current RAM state
+ * @file: the file where the data is saved
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+static int save_zero_page_to_file(RAMState *rs, QEMUFile *file,
+                                  RAMBlock *block, ram_addr_t offset)
 {
     uint8_t *p = block->host + offset;
-    int pages = -1;
+    int len = 0;
 
     if (is_zero_range(p, TARGET_PAGE_SIZE)) {
-        ram_counters.duplicate++;
-        ram_counters.transferred +=
-            save_page_header(rs, rs->f, block, offset | RAM_SAVE_FLAG_ZERO);
-        qemu_put_byte(rs->f, 0);
-        ram_counters.transferred += 1;
-        pages = 1;
+        len += save_page_header(rs, file, block, offset | RAM_SAVE_FLAG_ZERO);
+        qemu_put_byte(file, 0);
+        len += 1;
     }
+    return len;
+}
 
-    return pages;
+/**
+ * save_zero_page: send the zero page to the stream
+ *
+ * Returns the number of pages written.
+ *
+ * @rs: current RAM state
+ * @block: block that contains the page we want to send
+ * @offset: offset inside the block for the page
+ */
+static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+{
+    int len = save_zero_page_to_file(rs, rs->f, block, offset);
+
+    if (len) {
+        ram_counters.duplicate++;
+        ram_counters.transferred += len;
+        return 1;
+    }
+    return -1;
 }
 
 static void ram_release_pages(const char *rbname, uint64_t offset, int pages)
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 4/8] migration: introduce save_zero_page_to_file

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:16PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> It will be used by the compression threads
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>

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

-- 
Peter Xu

[Qemu-devel] [PATCH v2 5/8] migration: drop the return value of do_compress_ram_page

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

It is not used and cleans the code up a little

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index ce6e69b649..5aa624b3b9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -392,8 +392,8 @@ static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
 static QemuCond decomp_done_cond;
 
-static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
-                                ram_addr_t offset, uint8_t *source_buf);
+static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+                                 ram_addr_t offset, uint8_t *source_buf);
 
 static void *do_data_compress(void *opaque)
 {
@@ -1871,15 +1871,14 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
     return 1;
 }
 
-static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
-                                ram_addr_t offset, uint8_t *source_buf)
+static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+                                 ram_addr_t offset, uint8_t *source_buf)
 {
     RAMState *rs = ram_state;
-    int bytes_sent, blen;
     uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
+    int ret;
 
-    bytes_sent = save_page_header(rs, f, block, offset |
-                                  RAM_SAVE_FLAG_COMPRESS_PAGE);
+    save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
 
     /*
      * copy it to a internal buffer to avoid it being modified by VM
@@ -1887,17 +1886,14 @@ static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
      * decompression
      */
     memcpy(source_buf, p, TARGET_PAGE_SIZE);
-    blen = qemu_put_compression_data(f, stream, source_buf, TARGET_PAGE_SIZE);
-    if (blen < 0) {
-        bytes_sent = 0;
-        qemu_file_set_error(migrate_get_current()->to_dst_file, blen);
+    ret = qemu_put_compression_data(f, stream, source_buf, TARGET_PAGE_SIZE);
+    if (ret < 0) {
+        qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
         error_report("compressed data failed!");
-    } else {
-        bytes_sent += blen;
-        ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+        return;
     }
 
-    return bytes_sent;
+    ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
 }
 
 static void flush_compressed_data(RAMState *rs)
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 5/8] migration: drop the return value of do_compress_ram_page

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:17PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> It is not used and cleans the code up a little
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>

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

-- 
Peter Xu

[Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Detecting zero page is not a light work, moving it to the thread to
speed the main thread up

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 112 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 34 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5aa624b3b9..e1909502da 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -351,6 +351,7 @@ CompressionStats compression_counters;
 struct CompressParam {
     bool done;
     bool quit;
+    bool zero_page;
     QEMUFile *file;
     QemuMutex mutex;
     QemuCond cond;
@@ -392,7 +393,7 @@ static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
 static QemuCond decomp_done_cond;
 
-static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf);
 
 static void *do_data_compress(void *opaque)
@@ -400,6 +401,7 @@ static void *do_data_compress(void *opaque)
     CompressParam *param = opaque;
     RAMBlock *block;
     ram_addr_t offset;
+    bool zero_page;
 
     qemu_mutex_lock(&param->mutex);
     while (!param->quit) {
@@ -409,11 +411,12 @@ static void *do_data_compress(void *opaque)
             param->block = NULL;
             qemu_mutex_unlock(&param->mutex);
 
-            do_compress_ram_page(param->file, &param->stream, block, offset,
-                                 param->originbuf);
+            zero_page = do_compress_ram_page(param->file, &param->stream,
+                                             block, offset, param->originbuf);
 
             qemu_mutex_lock(&comp_done_lock);
             param->done = true;
+            param->zero_page = zero_page;
             qemu_cond_signal(&comp_done_cond);
             qemu_mutex_unlock(&comp_done_lock);
 
@@ -1871,13 +1874,19 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
     return 1;
 }
 
-static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf)
 {
     RAMState *rs = ram_state;
     uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
+    bool zero_page = false;
     int ret;
 
+    if (save_zero_page_to_file(rs, f, block, offset)) {
+        zero_page = true;
+        goto exit;
+    }
+
     save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
 
     /*
@@ -1890,10 +1899,12 @@ static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
     if (ret < 0) {
         qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
         error_report("compressed data failed!");
-        return;
+        return false;
     }
 
+exit:
     ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+    return zero_page;
 }
 
 static void flush_compressed_data(RAMState *rs)
@@ -1917,10 +1928,20 @@ static void flush_compressed_data(RAMState *rs)
         qemu_mutex_lock(&comp_param[idx].mutex);
         if (!comp_param[idx].quit) {
             len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
-            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
-            compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
-            compression_counters.pages++;
             ram_counters.transferred += len;
+
+            /*
+             * it's safe to fetch zero_page without holding comp_done_lock
+             * as there is no further request submitted to the thread,
+             * i.e, the thread should be waiting for a request at this point.
+             */
+            if (comp_param[idx].zero_page) {
+                ram_counters.duplicate++;
+            } else {
+                /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+                compression_counters.reduced_size += TARGET_PAGE_SIZE - len + 8;
+                compression_counters.pages++;
+            }
         }
         qemu_mutex_unlock(&comp_param[idx].mutex);
     }
@@ -1950,12 +1971,16 @@ retry:
             set_compress_params(&comp_param[idx], block, offset);
             qemu_cond_signal(&comp_param[idx].cond);
             qemu_mutex_unlock(&comp_param[idx].mutex);
-            pages = 1;
-            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
-            compression_counters.reduced_size += TARGET_PAGE_SIZE -
-                                                 bytes_xmit + 8;
-            compression_counters.pages++;
             ram_counters.transferred += bytes_xmit;
+            pages = 1;
+            if (comp_param[idx].zero_page) {
+                ram_counters.duplicate++;
+            } else {
+                /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+                compression_counters.reduced_size += TARGET_PAGE_SIZE -
+                                                     bytes_xmit + 8;
+                compression_counters.pages++;
+            }
             break;
         }
     }
@@ -2229,6 +2254,40 @@ static bool save_page_use_compression(RAMState *rs)
     return false;
 }
 
+/*
+ * try to compress the page before post it out, return true if the page
+ * has been properly handled by compression, otherwise needs other
+ * paths to handle it
+ */
+static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+{
+    if (!save_page_use_compression(rs)) {
+        return false;
+    }
+
+    /*
+     * When starting the process of a new block, the first page of
+     * the block should be sent out before other pages in the same
+     * block, and all the pages in last block should have been sent
+     * out, keeping this order is important, because the 'cont' flag
+     * is used to avoid resending the block name.
+     *
+     * We post the fist page as normal page as compression will take
+     * much CPU resource.
+     */
+    if (block != rs->last_sent_block) {
+        flush_compressed_data(rs);
+        return false;
+    }
+
+    if (compress_page_with_multi_thread(rs, block, offset) > 0) {
+        return true;
+    }
+
+    compression_counters.busy++;
+    return false;
+}
+
 /**
  * ram_save_target_page: save one target page
  *
@@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
         return res;
     }
 
-    /*
-     * When starting the process of a new block, the first page of
-     * the block should be sent out before other pages in the same
-     * block, and all the pages in last block should have been sent
-     * out, keeping this order is important, because the 'cont' flag
-     * is used to avoid resending the block name.
-     */
-    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
-            flush_compressed_data(rs);
+    if (save_compress_page(rs, block, offset)) {
+        return 1;
     }
 
     res = save_zero_page(rs, block, offset);
@@ -2275,18 +2327,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
     }
 
     /*
-     * Make sure the first page is sent out before other pages.
-     *
-     * we post it as normal page as compression will take much
-     * CPU resource.
-     */
-    if (block == rs->last_sent_block && save_page_use_compression(rs)) {
-        res = compress_page_with_multi_thread(rs, block, offset);
-        if (res > 0) {
-            return res;
-        }
-        compression_counters.busy++;
-    } else if (migrate_use_multifd()) {
+    * do not use multifd for compression as the first page in the new
+    * block should be posted out before sending the compressed page
+    */
+    if (!save_page_use_compression(rs) && migrate_use_multifd()) {
         return ram_save_multifd_page(rs, block, offset);
     }
 
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:18PM +0800, guangrong.xiao@gmail.com wrote:

[...]

> @@ -1950,12 +1971,16 @@ retry:
>              set_compress_params(&comp_param[idx], block, offset);
>              qemu_cond_signal(&comp_param[idx].cond);
>              qemu_mutex_unlock(&comp_param[idx].mutex);
> -            pages = 1;
> -            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> -            compression_counters.reduced_size += TARGET_PAGE_SIZE -
> -                                                 bytes_xmit + 8;
> -            compression_counters.pages++;
>              ram_counters.transferred += bytes_xmit;
> +            pages = 1;

(moving of this line seems irrelevant; meanwhile more duplicated codes
so even better to have a helper now)

> +            if (comp_param[idx].zero_page) {
> +                ram_counters.duplicate++;
> +            } else {
> +                /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +                compression_counters.reduced_size += TARGET_PAGE_SIZE -
> +                                                     bytes_xmit + 8;
> +                compression_counters.pages++;
> +            }
>              break;
>          }
>      }

[...]

> @@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>          return res;
>      }
>  
> -    /*
> -     * When starting the process of a new block, the first page of
> -     * the block should be sent out before other pages in the same
> -     * block, and all the pages in last block should have been sent
> -     * out, keeping this order is important, because the 'cont' flag
> -     * is used to avoid resending the block name.
> -     */
> -    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
> -            flush_compressed_data(rs);
> +    if (save_compress_page(rs, block, offset)) {
> +        return 1;

It's a bit tricky (though it seems to be a good idea too) to move the
zero detect into the compression thread, though I noticed that we also
do something else for zero pages:

    res = save_zero_page(rs, block, offset);
    if (res > 0) {
        /* Must let xbzrle know, otherwise a previous (now 0'd) cached
         * page would be stale
         */
        if (!save_page_use_compression(rs)) {
            XBZRLE_cache_lock();
            xbzrle_cache_zero_page(rs, block->offset + offset);
            XBZRLE_cache_unlock();
        }
        ram_release_pages(block->idstr, offset, res);
        return res;
    }

I'd guess that the xbzrle update of the zero page is not needed for
compression since after all xbzrle is not enabled when compression is
enabled, however do we need to do ram_release_pages() somehow?

>      }
>  
>      res = save_zero_page(rs, block, offset);
> @@ -2275,18 +2327,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>      }
>  
>      /*
> -     * Make sure the first page is sent out before other pages.
> -     *
> -     * we post it as normal page as compression will take much
> -     * CPU resource.
> -     */
> -    if (block == rs->last_sent_block && save_page_use_compression(rs)) {
> -        res = compress_page_with_multi_thread(rs, block, offset);
> -        if (res > 0) {
> -            return res;
> -        }
> -        compression_counters.busy++;
> -    } else if (migrate_use_multifd()) {
> +    * do not use multifd for compression as the first page in the new
> +    * block should be posted out before sending the compressed page
> +    */
> +    if (!save_page_use_compression(rs) && migrate_use_multifd()) {
>          return ram_save_multifd_page(rs, block, offset);
>      }
>  
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Xiao Guangrong]

On 07/23/2018 01:03 PM, Peter Xu wrote:
> On Thu, Jul 19, 2018 at 08:15:18PM +0800, guangrong.xiao@gmail.com wrote:
> 
> [...]
> 
>> @@ -1950,12 +1971,16 @@ retry:
>>               set_compress_params(&comp_param[idx], block, offset);
>>               qemu_cond_signal(&comp_param[idx].cond);
>>               qemu_mutex_unlock(&comp_param[idx].mutex);
>> -            pages = 1;
>> -            /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>> -            compression_counters.reduced_size += TARGET_PAGE_SIZE -
>> -                                                 bytes_xmit + 8;
>> -            compression_counters.pages++;
>>               ram_counters.transferred += bytes_xmit;
>> +            pages = 1;
> 
> (moving of this line seems irrelevant; meanwhile more duplicated codes
> so even better to have a helper now)
> 

Good to me. :)

>> +            if (comp_param[idx].zero_page) {
>> +                ram_counters.duplicate++;
>> +            } else {
>> +                /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
>> +                compression_counters.reduced_size += TARGET_PAGE_SIZE -
>> +                                                     bytes_xmit + 8;
>> +                compression_counters.pages++;
>> +            }
>>               break;
>>           }
>>       }
> 
> [...]
> 
>> @@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>           return res;
>>       }
>>   
>> -    /*
>> -     * When starting the process of a new block, the first page of
>> -     * the block should be sent out before other pages in the same
>> -     * block, and all the pages in last block should have been sent
>> -     * out, keeping this order is important, because the 'cont' flag
>> -     * is used to avoid resending the block name.
>> -     */
>> -    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
>> -            flush_compressed_data(rs);
>> +    if (save_compress_page(rs, block, offset)) {
>> +        return 1;
> 
> It's a bit tricky (though it seems to be a good idea too) to move the
> zero detect into the compression thread, though I noticed that we also
> do something else for zero pages:
> 
>      res = save_zero_page(rs, block, offset);
>      if (res > 0) {
>          /* Must let xbzrle know, otherwise a previous (now 0'd) cached
>           * page would be stale
>           */
>          if (!save_page_use_compression(rs)) {
>              XBZRLE_cache_lock();
>              xbzrle_cache_zero_page(rs, block->offset + offset);
>              XBZRLE_cache_unlock();
>          }
>          ram_release_pages(block->idstr, offset, res);
>          return res;
>      }
> 
> I'd guess that the xbzrle update of the zero page is not needed for
> compression since after all xbzrle is not enabled when compression is

Yup. if they are both enabled, compression works only for the first
iteration (i.e, ram_bulk_stage), at that point, nothing is cached
in xbzrle's cahe, in other words, xbzrle has posted nothing to the
destination.

> enabled, however do we need to do ram_release_pages() somehow?
> 

We have done it in the thread:

+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                   ram_addr_t offset, uint8_t *source_buf)
  {


+    if (save_zero_page_to_file(rs, f, block, offset)) {
+        zero_page = true;
+        goto exit;
+    }
......

+exit:
      ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+    return zero_page;
  }

However, it is not safe to do ram_release_pages in the thread as it's
not protected it multithreads. Fortunately, compression will be disabled
if it switches to post-copy, so i preferred to keep current behavior and
deferred to fix it after this patchset has been merged.

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Peter Xu]

On Mon, Jul 23, 2018 at 03:56:33PM +0800, Xiao Guangrong wrote:

[...]

> > > @@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> > >           return res;
> > >       }
> > > -    /*
> > > -     * When starting the process of a new block, the first page of
> > > -     * the block should be sent out before other pages in the same
> > > -     * block, and all the pages in last block should have been sent
> > > -     * out, keeping this order is important, because the 'cont' flag
> > > -     * is used to avoid resending the block name.
> > > -     */
> > > -    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
> > > -            flush_compressed_data(rs);
> > > +    if (save_compress_page(rs, block, offset)) {
> > > +        return 1;
> > 
> > It's a bit tricky (though it seems to be a good idea too) to move the
> > zero detect into the compression thread, though I noticed that we also
> > do something else for zero pages:
> > 
> >      res = save_zero_page(rs, block, offset);
> >      if (res > 0) {
> >          /* Must let xbzrle know, otherwise a previous (now 0'd) cached
> >           * page would be stale
> >           */
> >          if (!save_page_use_compression(rs)) {
> >              XBZRLE_cache_lock();
> >              xbzrle_cache_zero_page(rs, block->offset + offset);
> >              XBZRLE_cache_unlock();
> >          }
> >          ram_release_pages(block->idstr, offset, res);
> >          return res;
> >      }
> > 
> > I'd guess that the xbzrle update of the zero page is not needed for
> > compression since after all xbzrle is not enabled when compression is
> 
> Yup. if they are both enabled, compression works only for the first
> iteration (i.e, ram_bulk_stage), at that point, nothing is cached
> in xbzrle's cahe, in other words, xbzrle has posted nothing to the
> destination.
> 
> > enabled, however do we need to do ram_release_pages() somehow?
> > 
> 
> We have done it in the thread:
> 
> +static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>                                   ram_addr_t offset, uint8_t *source_buf)
>  {
> 
> 
> +    if (save_zero_page_to_file(rs, f, block, offset)) {
> +        zero_page = true;
> +        goto exit;
> +    }
> ......
> 
> +exit:
>      ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
> +    return zero_page;
>  }

Ah, then it seems fine.  Though I'd suggest you comment these into the
commit message in case people won't get it easily.

> 
> However, it is not safe to do ram_release_pages in the thread as it's
> not protected it multithreads. Fortunately, compression will be disabled
> if it switches to post-copy, so i preferred to keep current behavior and
> deferred to fix it after this patchset has been merged.

Do you mean ram_release_pages() is not thread-safe?  Why?  I didn't
notice it before but I feel like it is safe.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Xiao Guangrong]

On 07/23/2018 04:28 PM, Peter Xu wrote:
> On Mon, Jul 23, 2018 at 03:56:33PM +0800, Xiao Guangrong wrote:
> 
> [...]
> 
>>>> @@ -2249,15 +2308,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>>>            return res;
>>>>        }
>>>> -    /*
>>>> -     * When starting the process of a new block, the first page of
>>>> -     * the block should be sent out before other pages in the same
>>>> -     * block, and all the pages in last block should have been sent
>>>> -     * out, keeping this order is important, because the 'cont' flag
>>>> -     * is used to avoid resending the block name.
>>>> -     */
>>>> -    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
>>>> -            flush_compressed_data(rs);
>>>> +    if (save_compress_page(rs, block, offset)) {
>>>> +        return 1;
>>>
>>> It's a bit tricky (though it seems to be a good idea too) to move the
>>> zero detect into the compression thread, though I noticed that we also
>>> do something else for zero pages:
>>>
>>>       res = save_zero_page(rs, block, offset);
>>>       if (res > 0) {
>>>           /* Must let xbzrle know, otherwise a previous (now 0'd) cached
>>>            * page would be stale
>>>            */
>>>           if (!save_page_use_compression(rs)) {
>>>               XBZRLE_cache_lock();
>>>               xbzrle_cache_zero_page(rs, block->offset + offset);
>>>               XBZRLE_cache_unlock();
>>>           }
>>>           ram_release_pages(block->idstr, offset, res);
>>>           return res;
>>>       }
>>>
>>> I'd guess that the xbzrle update of the zero page is not needed for
>>> compression since after all xbzrle is not enabled when compression is
>>
>> Yup. if they are both enabled, compression works only for the first
>> iteration (i.e, ram_bulk_stage), at that point, nothing is cached
>> in xbzrle's cahe, in other words, xbzrle has posted nothing to the
>> destination.
>>
>>> enabled, however do we need to do ram_release_pages() somehow?
>>>
>>
>> We have done it in the thread:
>>
>> +static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>>                                    ram_addr_t offset, uint8_t *source_buf)
>>   {
>>
>>
>> +    if (save_zero_page_to_file(rs, f, block, offset)) {
>> +        zero_page = true;
>> +        goto exit;
>> +    }
>> ......
>>
>> +exit:
>>       ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
>> +    return zero_page;
>>   }
> 
> Ah, then it seems fine.  Though I'd suggest you comment these into the
> commit message in case people won't get it easily.
> 

Okay, will update the commit log addressed your comments.

>>
>> However, it is not safe to do ram_release_pages in the thread as it's
>> not protected it multithreads. Fortunately, compression will be disabled
>> if it switches to post-copy, so i preferred to keep current behavior and
>> deferred to fix it after this patchset has been merged.
> 
> Do you mean ram_release_pages() is not thread-safe?  Why?  I didn't
> notice it before but I feel like it is safe.

bitmap_clear() called in the function is not safe.

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Peter Xu]

On Mon, Jul 23, 2018 at 04:44:49PM +0800, Xiao Guangrong wrote:

[...]

> > > 
> > > However, it is not safe to do ram_release_pages in the thread as it's
> > > not protected it multithreads. Fortunately, compression will be disabled
> > > if it switches to post-copy, so i preferred to keep current behavior and
> > > deferred to fix it after this patchset has been merged.
> > 
> > Do you mean ram_release_pages() is not thread-safe?  Why?  I didn't
> > notice it before but I feel like it is safe.
> 
> bitmap_clear() called in the function is not safe.

Yeah, and the funny thing is that I don't think ram_release_pages()
should even touch the receivedmap...  It's possible that the
release-ram feature for postcopy is broken.

Never mind on that.  I'll post a patch to fix it, then I think the
ram_release_pages() will be thread safe.

Then this patch shouldn't be affected and it should be fine after that
fix.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 6/8] migration: move handle of zero page to the thread

[Xiao Guangrong]

On 07/23/2018 05:40 PM, Peter Xu wrote:
> On Mon, Jul 23, 2018 at 04:44:49PM +0800, Xiao Guangrong wrote:
> 
> [...]
> 
>>>>
>>>> However, it is not safe to do ram_release_pages in the thread as it's
>>>> not protected it multithreads. Fortunately, compression will be disabled
>>>> if it switches to post-copy, so i preferred to keep current behavior and
>>>> deferred to fix it after this patchset has been merged.
>>>
>>> Do you mean ram_release_pages() is not thread-safe?  Why?  I didn't
>>> notice it before but I feel like it is safe.
>>
>> bitmap_clear() called in the function is not safe.
> 
> Yeah, and the funny thing is that I don't think ram_release_pages()
> should even touch the receivedmap...  It's possible that the
> release-ram feature for postcopy is broken.
> 
> Never mind on that.  I'll post a patch to fix it, then I think the
> ram_release_pages() will be thread safe.
> 
> Then this patch shouldn't be affected and it should be fine after that
> fix

That would be great, thanks for your work.

[Qemu-devel] [PATCH v2 7/8] migration: hold the lock only if it is really needed

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Try to hold src_page_req_mutex only if the queue is not
empty

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 include/qemu/queue.h | 1 +
 migration/ram.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 59fd1203a1..ac418efc43 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -341,6 +341,7 @@ struct {                                                                \
 /*
  * Simple queue access methods.
  */
+#define QSIMPLEQ_EMPTY_ATOMIC(head) (atomic_read(&((head)->sqh_first)) == NULL)
 #define QSIMPLEQ_EMPTY(head)        ((head)->sqh_first == NULL)
 #define QSIMPLEQ_FIRST(head)        ((head)->sqh_first)
 #define QSIMPLEQ_NEXT(elm, field)   ((elm)->field.sqe_next)
diff --git a/migration/ram.c b/migration/ram.c
index e1909502da..89305c7af5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2062,6 +2062,10 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
 {
     RAMBlock *block = NULL;
 
+    if (QSIMPLEQ_EMPTY_ATOMIC(&rs->src_page_requests)) {
+        return NULL;
+    }
+
     qemu_mutex_lock(&rs->src_page_req_mutex);
     if (!QSIMPLEQ_EMPTY(&rs->src_page_requests)) {
         struct RAMSrcPageRequest *entry =
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 7/8] migration: hold the lock only if it is really needed

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:19PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Try to hold src_page_req_mutex only if the queue is not
> empty
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>

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

-- 
Peter Xu

[Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

flush_compressed_data() needs to wait all compression threads to
finish their work, after that all threads are free until the
migration feeds new request to them, reducing its call can improve
the throughput and use CPU resource more effectively

We do not need to flush all threads at the end of iteration, the
data can be kept locally until the memory block is changed or
memory migration starts over in that case we will meet a dirtied
page which may still exists in compression threads's ring

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 89305c7af5..fdab13821d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -315,6 +315,8 @@ struct RAMState {
     uint64_t iterations;
     /* number of dirty bits in the bitmap */
     uint64_t migration_dirty_pages;
+    /* last dirty_sync_count we have seen */
+    uint64_t dirty_sync_count;
     /* protects modification of the bitmap */
     QemuMutex bitmap_mutex;
     /* The RAMBlock used in the last src_page_requests */
@@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
     }
 
     xbzrle_cleanup();
+    flush_compressed_data(*rsp);
     compress_threads_save_cleanup();
     ram_state_cleanup(rsp);
 }
@@ -3203,6 +3206,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
     ram_control_before_iterate(f, RAM_CONTROL_ROUND);
 
+    /*
+     * if memory migration starts over, we will meet a dirtied page which
+     * may still exists in compression threads's ring, so we should flush
+     * the compressed data to make sure the new page is not overwritten by
+     * the old one in the destination.
+     */
+    if (ram_counters.dirty_sync_count != rs->dirty_sync_count) {
+        rs->dirty_sync_count = ram_counters.dirty_sync_count;
+        flush_compressed_data(rs);
+    }
+
     t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     i = 0;
     while ((ret = qemu_file_rate_limit(f)) == 0 ||
@@ -3235,7 +3249,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         }
         i++;
     }
-    flush_compressed_data(rs);
     rcu_read_unlock();
 
     /*
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Peter Xu]

On Thu, Jul 19, 2018 at 08:15:20PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> flush_compressed_data() needs to wait all compression threads to
> finish their work, after that all threads are free until the
> migration feeds new request to them, reducing its call can improve
> the throughput and use CPU resource more effectively
> 
> We do not need to flush all threads at the end of iteration, the
> data can be kept locally until the memory block is changed or
> memory migration starts over in that case we will meet a dirtied
> page which may still exists in compression threads's ring
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  migration/ram.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 89305c7af5..fdab13821d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -315,6 +315,8 @@ struct RAMState {
>      uint64_t iterations;
>      /* number of dirty bits in the bitmap */
>      uint64_t migration_dirty_pages;
> +    /* last dirty_sync_count we have seen */
> +    uint64_t dirty_sync_count;

Better suffix it with "_prev" as well?  So that we can quickly
identify that it's only a cache and it can be different from the one
in the ram_counters.

>      /* protects modification of the bitmap */
>      QemuMutex bitmap_mutex;
>      /* The RAMBlock used in the last src_page_requests */
> @@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
>      }
>  
>      xbzrle_cleanup();
> +    flush_compressed_data(*rsp);

Could I ask why do we need this considering that we have
compress_threads_save_cleanup() right down there?

>      compress_threads_save_cleanup();
>      ram_state_cleanup(rsp);
>  }
> @@ -3203,6 +3206,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  
>      ram_control_before_iterate(f, RAM_CONTROL_ROUND);
>  
> +    /*
> +     * if memory migration starts over, we will meet a dirtied page which
> +     * may still exists in compression threads's ring, so we should flush
> +     * the compressed data to make sure the new page is not overwritten by
> +     * the old one in the destination.
> +     */
> +    if (ram_counters.dirty_sync_count != rs->dirty_sync_count) {
> +        rs->dirty_sync_count = ram_counters.dirty_sync_count;
> +        flush_compressed_data(rs);
> +    }
> +
>      t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      i = 0;
>      while ((ret = qemu_file_rate_limit(f)) == 0 ||
> @@ -3235,7 +3249,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          }
>          i++;
>      }
> -    flush_compressed_data(rs);

This looks sane to me, but I'd like to see how other people would
think about it too...

>      rcu_read_unlock();
>  
>      /*
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Xiao Guangrong]

On 07/23/2018 01:49 PM, Peter Xu wrote:
> On Thu, Jul 19, 2018 at 08:15:20PM +0800, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> flush_compressed_data() needs to wait all compression threads to
>> finish their work, after that all threads are free until the
>> migration feeds new request to them, reducing its call can improve
>> the throughput and use CPU resource more effectively
>>
>> We do not need to flush all threads at the end of iteration, the
>> data can be kept locally until the memory block is changed or
>> memory migration starts over in that case we will meet a dirtied
>> page which may still exists in compression threads's ring
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   migration/ram.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 89305c7af5..fdab13821d 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -315,6 +315,8 @@ struct RAMState {
>>       uint64_t iterations;
>>       /* number of dirty bits in the bitmap */
>>       uint64_t migration_dirty_pages;
>> +    /* last dirty_sync_count we have seen */
>> +    uint64_t dirty_sync_count;
> 
> Better suffix it with "_prev" as well?  So that we can quickly
> identify that it's only a cache and it can be different from the one
> in the ram_counters.

Indeed, will update it.

> 
>>       /* protects modification of the bitmap */
>>       QemuMutex bitmap_mutex;
>>       /* The RAMBlock used in the last src_page_requests */
>> @@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
>>       }
>>   
>>       xbzrle_cleanup();
>> +    flush_compressed_data(*rsp);
> 
> Could I ask why do we need this considering that we have
> compress_threads_save_cleanup() right down there?

Dave ask it too. :(

"This is for the error condition, if any error occurred during live migration,
there is no chance to call ram_save_complete. After using the lockless
multithreads model, we assert all requests have been handled before destroy
the work threads."

That makes sure there is nothing left in the threads before doing
compress_threads_save_cleanup() as current behavior. For lockless
mutilthread model, we check if all requests are free before destroy
them.

> 
>>       compress_threads_save_cleanup();
>>       ram_state_cleanup(rsp);
>>   }
>> @@ -3203,6 +3206,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>   
>>       ram_control_before_iterate(f, RAM_CONTROL_ROUND);
>>   
>> +    /*
>> +     * if memory migration starts over, we will meet a dirtied page which
>> +     * may still exists in compression threads's ring, so we should flush
>> +     * the compressed data to make sure the new page is not overwritten by
>> +     * the old one in the destination.
>> +     */
>> +    if (ram_counters.dirty_sync_count != rs->dirty_sync_count) {
>> +        rs->dirty_sync_count = ram_counters.dirty_sync_count;
>> +        flush_compressed_data(rs);
>> +    }
>> +
>>       t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>       i = 0;
>>       while ((ret = qemu_file_rate_limit(f)) == 0 ||
>> @@ -3235,7 +3249,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>           }
>>           i++;
>>       }
>> -    flush_compressed_data(rs);
> 
> This looks sane to me, but I'd like to see how other people would
> think about it too...

Thank you a lot, Peter! :)

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Peter Xu]

On Mon, Jul 23, 2018 at 04:05:21PM +0800, Xiao Guangrong wrote:
> 
> 
> On 07/23/2018 01:49 PM, Peter Xu wrote:
> > On Thu, Jul 19, 2018 at 08:15:20PM +0800, guangrong.xiao@gmail.com wrote:
> > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > 
> > > flush_compressed_data() needs to wait all compression threads to
> > > finish their work, after that all threads are free until the
> > > migration feeds new request to them, reducing its call can improve
> > > the throughput and use CPU resource more effectively
> > > 
> > > We do not need to flush all threads at the end of iteration, the
> > > data can be kept locally until the memory block is changed or
> > > memory migration starts over in that case we will meet a dirtied
> > > page which may still exists in compression threads's ring
> > > 
> > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > ---
> > >   migration/ram.c | 15 ++++++++++++++-
> > >   1 file changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 89305c7af5..fdab13821d 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -315,6 +315,8 @@ struct RAMState {
> > >       uint64_t iterations;
> > >       /* number of dirty bits in the bitmap */
> > >       uint64_t migration_dirty_pages;
> > > +    /* last dirty_sync_count we have seen */
> > > +    uint64_t dirty_sync_count;
> > 
> > Better suffix it with "_prev" as well?  So that we can quickly
> > identify that it's only a cache and it can be different from the one
> > in the ram_counters.
> 
> Indeed, will update it.
> 
> > 
> > >       /* protects modification of the bitmap */
> > >       QemuMutex bitmap_mutex;
> > >       /* The RAMBlock used in the last src_page_requests */
> > > @@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
> > >       }
> > >       xbzrle_cleanup();
> > > +    flush_compressed_data(*rsp);
> > 
> > Could I ask why do we need this considering that we have
> > compress_threads_save_cleanup() right down there?
> 
> Dave ask it too. :(
> 
> "This is for the error condition, if any error occurred during live migration,
> there is no chance to call ram_save_complete. After using the lockless
> multithreads model, we assert all requests have been handled before destroy
> the work threads."
> 
> That makes sure there is nothing left in the threads before doing
> compress_threads_save_cleanup() as current behavior. For lockless
> mutilthread model, we check if all requests are free before destroy
> them.

But why do we need to explicitly flush it here?  Now in
compress_threads_save_cleanup() we have qemu_fclose() on the buffers,
which logically will flush the data and clean up everything too.
Would that suffice?

> 
> > 
> > >       compress_threads_save_cleanup();
> > >       ram_state_cleanup(rsp);
> > >   }
> > > @@ -3203,6 +3206,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> > >       ram_control_before_iterate(f, RAM_CONTROL_ROUND);
> > > +    /*
> > > +     * if memory migration starts over, we will meet a dirtied page which
> > > +     * may still exists in compression threads's ring, so we should flush
> > > +     * the compressed data to make sure the new page is not overwritten by
> > > +     * the old one in the destination.
> > > +     */
> > > +    if (ram_counters.dirty_sync_count != rs->dirty_sync_count) {
> > > +        rs->dirty_sync_count = ram_counters.dirty_sync_count;
> > > +        flush_compressed_data(rs);
> > > +    }
> > > +
> > >       t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> > >       i = 0;
> > >       while ((ret = qemu_file_rate_limit(f)) == 0 ||
> > > @@ -3235,7 +3249,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> > >           }
> > >           i++;
> > >       }
> > > -    flush_compressed_data(rs);
> > 
> > This looks sane to me, but I'd like to see how other people would
> > think about it too...
> 
> Thank you a lot, Peter! :)

Welcome. :)

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Xiao Guangrong]

On 07/23/2018 04:35 PM, Peter Xu wrote:
> On Mon, Jul 23, 2018 at 04:05:21PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 07/23/2018 01:49 PM, Peter Xu wrote:
>>> On Thu, Jul 19, 2018 at 08:15:20PM +0800, guangrong.xiao@gmail.com wrote:
>>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>>
>>>> flush_compressed_data() needs to wait all compression threads to
>>>> finish their work, after that all threads are free until the
>>>> migration feeds new request to them, reducing its call can improve
>>>> the throughput and use CPU resource more effectively
>>>>
>>>> We do not need to flush all threads at the end of iteration, the
>>>> data can be kept locally until the memory block is changed or
>>>> memory migration starts over in that case we will meet a dirtied
>>>> page which may still exists in compression threads's ring
>>>>
>>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>> ---
>>>>    migration/ram.c | 15 ++++++++++++++-
>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 89305c7af5..fdab13821d 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -315,6 +315,8 @@ struct RAMState {
>>>>        uint64_t iterations;
>>>>        /* number of dirty bits in the bitmap */
>>>>        uint64_t migration_dirty_pages;
>>>> +    /* last dirty_sync_count we have seen */
>>>> +    uint64_t dirty_sync_count;
>>>
>>> Better suffix it with "_prev" as well?  So that we can quickly
>>> identify that it's only a cache and it can be different from the one
>>> in the ram_counters.
>>
>> Indeed, will update it.
>>
>>>
>>>>        /* protects modification of the bitmap */
>>>>        QemuMutex bitmap_mutex;
>>>>        /* The RAMBlock used in the last src_page_requests */
>>>> @@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
>>>>        }
>>>>        xbzrle_cleanup();
>>>> +    flush_compressed_data(*rsp);
>>>
>>> Could I ask why do we need this considering that we have
>>> compress_threads_save_cleanup() right down there?
>>
>> Dave ask it too. :(
>>
>> "This is for the error condition, if any error occurred during live migration,
>> there is no chance to call ram_save_complete. After using the lockless
>> multithreads model, we assert all requests have been handled before destroy
>> the work threads."
>>
>> That makes sure there is nothing left in the threads before doing
>> compress_threads_save_cleanup() as current behavior. For lockless
>> mutilthread model, we check if all requests are free before destroy
>> them.
> 
> But why do we need to explicitly flush it here?  Now in
> compress_threads_save_cleanup() we have qemu_fclose() on the buffers,
> which logically will flush the data and clean up everything too.
> Would that suffice?
> 

Yes, it's sufficient for current thread model, will drop it for now
and add it at the time when the lockless mutilthread model is applied. :)

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Peter Xu]

On Mon, Jul 23, 2018 at 04:53:11PM +0800, Xiao Guangrong wrote:
> 
> 
> On 07/23/2018 04:35 PM, Peter Xu wrote:
> > On Mon, Jul 23, 2018 at 04:05:21PM +0800, Xiao Guangrong wrote:
> > > 
> > > 
> > > On 07/23/2018 01:49 PM, Peter Xu wrote:
> > > > On Thu, Jul 19, 2018 at 08:15:20PM +0800, guangrong.xiao@gmail.com wrote:
> > > > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > > > 
> > > > > flush_compressed_data() needs to wait all compression threads to
> > > > > finish their work, after that all threads are free until the
> > > > > migration feeds new request to them, reducing its call can improve
> > > > > the throughput and use CPU resource more effectively
> > > > > 
> > > > > We do not need to flush all threads at the end of iteration, the
> > > > > data can be kept locally until the memory block is changed or
> > > > > memory migration starts over in that case we will meet a dirtied
> > > > > page which may still exists in compression threads's ring
> > > > > 
> > > > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > > > ---
> > > > >    migration/ram.c | 15 ++++++++++++++-
> > > > >    1 file changed, 14 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > > index 89305c7af5..fdab13821d 100644
> > > > > --- a/migration/ram.c
> > > > > +++ b/migration/ram.c
> > > > > @@ -315,6 +315,8 @@ struct RAMState {
> > > > >        uint64_t iterations;
> > > > >        /* number of dirty bits in the bitmap */
> > > > >        uint64_t migration_dirty_pages;
> > > > > +    /* last dirty_sync_count we have seen */
> > > > > +    uint64_t dirty_sync_count;
> > > > 
> > > > Better suffix it with "_prev" as well?  So that we can quickly
> > > > identify that it's only a cache and it can be different from the one
> > > > in the ram_counters.
> > > 
> > > Indeed, will update it.
> > > 
> > > > 
> > > > >        /* protects modification of the bitmap */
> > > > >        QemuMutex bitmap_mutex;
> > > > >        /* The RAMBlock used in the last src_page_requests */
> > > > > @@ -2532,6 +2534,7 @@ static void ram_save_cleanup(void *opaque)
> > > > >        }
> > > > >        xbzrle_cleanup();
> > > > > +    flush_compressed_data(*rsp);
> > > > 
> > > > Could I ask why do we need this considering that we have
> > > > compress_threads_save_cleanup() right down there?
> > > 
> > > Dave ask it too. :(
> > > 
> > > "This is for the error condition, if any error occurred during live migration,
> > > there is no chance to call ram_save_complete. After using the lockless
> > > multithreads model, we assert all requests have been handled before destroy
> > > the work threads."
> > > 
> > > That makes sure there is nothing left in the threads before doing
> > > compress_threads_save_cleanup() as current behavior. For lockless
> > > mutilthread model, we check if all requests are free before destroy
> > > them.
> > 
> > But why do we need to explicitly flush it here?  Now in
> > compress_threads_save_cleanup() we have qemu_fclose() on the buffers,
> > which logically will flush the data and clean up everything too.
> > Would that suffice?
> > 
> 
> Yes, it's sufficient for current thread model, will drop it for now
> and add it at the time when the lockless mutilthread model is applied. :)

Ah I think I see your point.  Even if so I would think it better to do
any extra cleanup directly in compress_threads_save_cleanup() if
possible.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 8/8] migration: do not flush_compressed_data at the end of each iteration

[Xiao Guangrong]

On 07/23/2018 05:01 PM, Peter Xu wrote:

>> Yes, it's sufficient for current thread model, will drop it for now
>> and add it at the time when the lockless mutilthread model is applied. :)
> 
> Ah I think I see your point.  Even if so I would think it better to do
> any extra cleanup directly in compress_threads_save_cleanup() if
> possible.
> 

Okay, got it.