[PATCH 00/18] Migration compression cleanup

[PATCH 00/18] Migration compression cleanup

[Juan Quintela]

Hi

On this series:

- We got errors when setting incompatible migration methods
  (i.e. compression and RDMA)

- Remove the posibility of running COMPRESSION and XBZRLE together
  (remember that compression don't work)

- This series is the preliminar work to be able to compile out
  compression (almost there, but wanted to get this first)

- Move everyting that touches compression_counters to ram-compress.c

- simplify pass of block/file parameter.  We can get them trough the
  pss.

Please, review.

Juan Quintela (18):
  migration: Give one error if trying to set MULTIFD and XBZRLE
  migration: Give one error if trying to set COMPRESSION and XBZRLE
  migration: RDMA is not compatible with anything else
  migration: Remove save_page_use_compression()
  migration: Move compression_counters cleanup ram-compress.c
  migration: Create populate_compress()
  migration: Create ram_compressed_pages()
  migration: Move update_compress_threads_counts() to ram-compress.c
  migration: Make compress_data_with_multithreads return bool
  migration: Simplify compress_page_with_multithread()
  migration: Move busy++ to migrate_with_multithread
  migration: Create compression_update_rates()
  migration: Simplify decompress_data_with_multi_threads()
  migration: Use "i" as an for index in ram-compress.c
  migration: save_compress_page() can take block through pss
  migration: control_save_page() can take block through pss
  migration: Remove not needed block parameter for save_zero_page*
  migration: Remove not needed file parameter for save_zero_page*

 migration/ram-compress.h |  10 +-
 migration/ram.h          |   1 -
 migration/migration.c    |  27 +++---
 migration/options.c      |  14 +++
 migration/ram-compress.c | 198 +++++++++++++++++++++++++++------------
 migration/ram.c          | 121 +++++-------------------
 migration/savevm.c       |   1 -
 7 files changed, 198 insertions(+), 174 deletions(-)

-- 
2.40.1

[PATCH 01/18] migration: Give one error if trying to set MULTIFD and XBZRLE

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/options.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index b62ab30cd5..c6674a4753 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -547,6 +547,13 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
         }
     }
 
+    if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
+        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
+            error_setg(errp, "Multifd is not compatible with xbzrle");
+            return false;
+        }
+    }
+
     return true;
 }
 
-- 
2.40.1

Re: [PATCH 01/18] migration: Give one error if trying to set MULTIFD and XBZRLE

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Tue, 13 Jun 2023 16:57:40 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/options.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/migration/options.c b/migration/options.c
> index b62ab30cd5..c6674a4753 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -547,6 +547,13 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>          }
>      }
>  
> +    if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
> +        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
> +            error_setg(errp, "Multifd is not compatible with xbzrle");
> +            return false;
> +        }
> +    }
> +
>      return true;
>  }
>  

Hmm, I think supporting xbzrle + multifd might be worthwhile as an
alternative to xbzrle + compress.
I don't use it, but it should be easy to do by just letting xbzlre try
to handle the page before multifd. 
There shouldn't be any incompatibilities other than that, as long as
xbzlre sees the pages before multifd and the zero-page case is still
handled the old way.

Best Regards,
Lukas Straub

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/18] migration: Give one error if trying to set MULTIFD and XBZRLE

[Juan Quintela]

Lukas Straub <lukasstraub2@web.de> wrote:
> On Tue, 13 Jun 2023 16:57:40 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/options.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index b62ab30cd5..c6674a4753 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -547,6 +547,13 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>          }
>>      }
>>  
>> +    if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
>> +        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
>> +            error_setg(errp, "Multifd is not compatible with xbzrle");
>> +            return false;
>> +        }
>> +    }
>> +
>>      return true;
>>  }
>>  
>
> Hmm, I think supporting xbzrle + multifd might be worthwhile as an
> alternative to xbzrle + compress.

Nack.

> I don't use it, but it should be easy to do by just letting xbzlre try
> to handle the page before multifd.

My idea here is to implement xbzrle as we implement zlib/zstd on
multifd.  Just go from there.

> There shouldn't be any incompatibilities other than that, as long as
> xbzlre sees the pages before multifd and the zero-page case is still
> handled the old way.

The way that I say is going to be easier and probably also faster.

Later, Juan.

[PATCH 02/18] migration: Give one error if trying to set COMPRESSION and XBZRLE

[Juan Quintela]

As we have disable to use both together in the previous patch, we can
remove this check.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/options.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index c6674a4753..a18689c314 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -554,6 +554,13 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
         }
     }
 
+    if (new_caps[MIGRATION_CAPABILITY_COMPRESS]) {
+        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
+            error_setg(errp, "Compression is not compatible with xbzrle");
+            return false;
+        }
+    }
+
     return true;
 }
 
-- 
2.40.1

Re: [PATCH 02/18] migration: Give one error if trying to set COMPRESSION and XBZRLE

[Lukas Straub]

On Tue, 13 Jun 2023 16:57:41 +0200
Juan Quintela <quintela@redhat.com> wrote:

> As we have disable to use both together in the previous patch, we can
> remove this check.

This commit message is off.

>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/options.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/migration/options.c b/migration/options.c
> index c6674a4753..a18689c314 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -554,6 +554,13 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>          }
>      }
>
> +    if (new_caps[MIGRATION_CAPABILITY_COMPRESS]) {
> +        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
> +            error_setg(errp, "Compression is not compatible with xbzrle");
> +            return false;
> +        }
> +    }
> +
>      return true;
>  }
>

[PATCH 03/18] migration: RDMA is not compatible with anything else

[Juan Quintela]

So give an error instead of just ignoring the other methods.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index dc05c6f6ea..84551f040c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -436,6 +436,18 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
         socket_start_incoming_migration(p ? p : uri, errp);
 #ifdef CONFIG_RDMA
     } else if (strstart(uri, "rdma:", &p)) {
+        if (migrate_compress()) {
+            error_setg(errp, "RDMA and compression can't be used together");
+            return;
+        }
+        if (migrate_xbzrle()) {
+            error_setg(errp, "RDMA and XBZRLE can't be used together");
+            return;
+        }
+        if (migrate_multifd()) {
+            error_setg(errp, "RDMA and multifd can't be used together");
+            return;
+        }
         rdma_start_incoming_migration(p, errp);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
-- 
2.40.1

Re: [PATCH 03/18] migration: RDMA is not compatible with anything else

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On Tue, 13 Jun 2023 16:57:42 +0200
Juan Quintela <quintela@redhat.com> wrote:

> So give an error instead of just ignoring the other methods.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/migration.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index dc05c6f6ea..84551f040c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -436,6 +436,18 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>          socket_start_incoming_migration(p ? p : uri, errp);
>  #ifdef CONFIG_RDMA
>      } else if (strstart(uri, "rdma:", &p)) {
> +        if (migrate_compress()) {
> +            error_setg(errp, "RDMA and compression can't be used together");
> +            return;
> +        }
> +        if (migrate_xbzrle()) {
> +            error_setg(errp, "RDMA and XBZRLE can't be used together");
> +            return;
> +        }
> +        if (migrate_multifd()) {
> +            error_setg(errp, "RDMA and multifd can't be used together");
> +            return;
> +        }
>          rdma_start_incoming_migration(p, errp);
>  #endif
>      } else if (strstart(uri, "exec:", &p)) {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 04/18] migration: Remove save_page_use_compression()

[Juan Quintela]

After previous patch, we disable the posiblity that we use compression
together with xbzrle.  So we can use directly migrate_compress().

Once there, now we don't need the rs parameter, so remove it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 88a6c82e63..d4943b982f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1274,8 +1274,6 @@ update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
     compression_counters.pages++;
 }
 
-static bool save_page_use_compression(RAMState *rs);
-
 static int send_queued_data(CompressParam *param)
 {
     PageSearchStatus *pss = &ram_state->pss[RAM_CHANNEL_PRECOPY];
@@ -1312,9 +1310,9 @@ static int send_queued_data(CompressParam *param)
     return len;
 }
 
-static void ram_flush_compressed_data(RAMState *rs)
+static void ram_flush_compressed_data(void)
 {
-    if (!save_page_use_compression(rs)) {
+    if (!migrate_compress()) {
         return;
     }
 
@@ -1375,7 +1373,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
              * Also If xbzrle is on, stop using the data compression at this
              * point. In theory, xbzrle can do better than compression.
              */
-            ram_flush_compressed_data(rs);
+            ram_flush_compressed_data();
 
             /* Hit the end of the list */
             pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
@@ -2024,24 +2022,6 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
     return 0;
 }
 
-static bool save_page_use_compression(RAMState *rs)
-{
-    if (!migrate_compress()) {
-        return false;
-    }
-
-    /*
-     * If xbzrle is enabled (e.g., after first round of migration), stop
-     * using the data compression. In theory, xbzrle can do better than
-     * compression.
-     */
-    if (rs->xbzrle_started) {
-        return false;
-    }
-
-    return true;
-}
-
 /*
  * try to compress the page before posting it out, return true if the page
  * has been properly handled by compression, otherwise needs other
@@ -2050,7 +2030,7 @@ static bool save_page_use_compression(RAMState *rs)
 static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
                                RAMBlock *block, ram_addr_t offset)
 {
-    if (!save_page_use_compression(rs)) {
+    if (!migrate_compress()) {
         return false;
     }
 
@@ -2065,7 +2045,7 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
      * much CPU resource.
      */
     if (block != pss->last_sent_block) {
-        ram_flush_compressed_data(rs);
+        ram_flush_compressed_data();
         return false;
     }
 
@@ -3115,7 +3095,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
              * page is sent in one chunk.
              */
             if (migrate_postcopy_ram()) {
-                ram_flush_compressed_data(rs);
+                ram_flush_compressed_data();
             }
 
             /*
@@ -3210,7 +3190,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
         qemu_mutex_unlock(&rs->bitmap_mutex);
 
-        ram_flush_compressed_data(rs);
+        ram_flush_compressed_data();
         ram_control_after_iterate(f, RAM_CONTROL_FINISH);
     }
 
-- 
2.40.1

[PATCH 05/18] migration: Move compression_counters cleanup ram-compress.c

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c    | 4 +---
 migration/ram-compress.c | 5 +++++
 migration/savevm.c       | 1 -
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 84551f040c..5105ad6825 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1628,11 +1628,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
 
     migrate_init(s);
     /*
-     * set mig_stats compression_counters memory to zero for a
-     * new migration
+     * set mig_stats memory to zero for a new migration
      */
     memset(&mig_stats, 0, sizeof(mig_stats));
-    memset(&compression_counters, 0, sizeof(compression_counters));
 
     return true;
 }
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 06254d8c69..a0f6a56470 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -430,6 +430,11 @@ int compress_threads_load_setup(QEMUFile *f)
         return 0;
     }
 
+    /*
+     * set compression_counters memory to zero for a new migration
+     */
+    memset(&compression_counters, 0, sizeof(compression_counters));
+
     thread_count = migrate_decompress_threads();
     decompress_threads = g_new0(QemuThread, thread_count);
     decomp_param = g_new0(DecompressParam, thread_count);
diff --git a/migration/savevm.c b/migration/savevm.c
index bc284087f9..aa31d29f23 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1621,7 +1621,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     migrate_init(ms);
     memset(&mig_stats, 0, sizeof(mig_stats));
-    memset(&compression_counters, 0, sizeof(compression_counters));
     ms->to_dst_file = f;
 
     qemu_mutex_unlock_iothread();
-- 
2.40.1

Re: [PATCH 05/18] migration: Move compression_counters cleanup ram-compress.c

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

On Tue, 13 Jun 2023 16:57:44 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/migration.c    | 4 +---
>  migration/ram-compress.c | 5 +++++
>  migration/savevm.c       | 1 -
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 84551f040c..5105ad6825 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1628,11 +1628,9 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>  
>      migrate_init(s);
>      /*
> -     * set mig_stats compression_counters memory to zero for a
> -     * new migration
> +     * set mig_stats memory to zero for a new migration
>       */
>      memset(&mig_stats, 0, sizeof(mig_stats));
> -    memset(&compression_counters, 0, sizeof(compression_counters));
>  
>      return true;
>  }
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 06254d8c69..a0f6a56470 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -430,6 +430,11 @@ int compress_threads_load_setup(QEMUFile *f)
>          return 0;
>      }
>  
> +    /*
> +     * set compression_counters memory to zero for a new migration
> +     */
> +    memset(&compression_counters, 0, sizeof(compression_counters));
> +
>      thread_count = migrate_decompress_threads();
>      decompress_threads = g_new0(QemuThread, thread_count);
>      decomp_param = g_new0(DecompressParam, thread_count);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index bc284087f9..aa31d29f23 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1621,7 +1621,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  
>      migrate_init(ms);
>      memset(&mig_stats, 0, sizeof(mig_stats));
> -    memset(&compression_counters, 0, sizeof(compression_counters));
>      ms->to_dst_file = f;
>  
>      qemu_mutex_unlock_iothread();


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 06/18] migration: Create populate_compress()

[Juan Quintela]

So we don't have to access compression_counters from outside
ram-compress.c.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.h |  3 +++
 migration/migration.c    | 11 +----------
 migration/ram-compress.c | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/migration/ram-compress.h b/migration/ram-compress.h
index 6f7fe2f472..43ff44c0ba 100644
--- a/migration/ram-compress.h
+++ b/migration/ram-compress.h
@@ -30,6 +30,7 @@
 #define QEMU_MIGRATION_COMPRESS_H
 
 #include "qemu-file.h"
+#include "qapi/qapi-types-migration.h"
 
 enum CompressResult {
     RES_NONE = 0,
@@ -67,4 +68,6 @@ void compress_threads_load_cleanup(void);
 int compress_threads_load_setup(QEMUFile *f);
 void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
 
+void populate_compress(MigrationInfo *info);
+
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 5105ad6825..402bcd0d67 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -956,16 +956,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->xbzrle_cache->overflow = xbzrle_counters.overflow;
     }
 
-    if (migrate_compress()) {
-        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->compressed_size =
-                                    compression_counters.compressed_size;
-        info->compression->compression_rate =
-                                    compression_counters.compression_rate;
-    }
+    populate_compress(info);
 
     if (cpu_throttle_active()) {
         info->has_cpu_throttle_percentage = true;
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index a0f6a56470..a91c32588e 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -488,3 +488,17 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
         }
     }
 }
+
+void populate_compress(MigrationInfo *info)
+{
+    if (migrate_compress()) {
+        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->compressed_size =
+            compression_counters.compressed_size;
+        info->compression->compression_rate =
+            compression_counters.compression_rate;
+    }
+}
-- 
2.40.1

Re: [PATCH 06/18] migration: Create populate_compress()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

On Tue, 13 Jun 2023 16:57:45 +0200
Juan Quintela <quintela@redhat.com> wrote:

> So we don't have to access compression_counters from outside
> ram-compress.c.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

With the comment below fixed:
Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.h |  3 +++
>  migration/migration.c    | 11 +----------
>  migration/ram-compress.c | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/ram-compress.h b/migration/ram-compress.h
> index 6f7fe2f472..43ff44c0ba 100644
> --- a/migration/ram-compress.h
> +++ b/migration/ram-compress.h
> @@ -30,6 +30,7 @@
>  #define QEMU_MIGRATION_COMPRESS_H
>  
>  #include "qemu-file.h"
> +#include "qapi/qapi-types-migration.h"
>  
>  enum CompressResult {
>      RES_NONE = 0,
> @@ -67,4 +68,6 @@ void compress_threads_load_cleanup(void);
>  int compress_threads_load_setup(QEMUFile *f);
>  void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
>  
> +void populate_compress(MigrationInfo *info);
> +
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 5105ad6825..402bcd0d67 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -956,16 +956,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>          info->xbzrle_cache->overflow = xbzrle_counters.overflow;
>      }
>  
> -    if (migrate_compress()) {
> -        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->compressed_size =
> -                                    compression_counters.compressed_size;
> -        info->compression->compression_rate =
> -                                    compression_counters.compression_rate;
> -    }
> +    populate_compress(info);
>  
>      if (cpu_throttle_active()) {
>          info->has_cpu_throttle_percentage = true;
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index a0f6a56470..a91c32588e 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -488,3 +488,17 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
>          }
>      }
>  }
> +
> +void populate_compress(MigrationInfo *info)
> +{
> +    if (migrate_compress()) {

Use early return:

if (!migrate_compress()) {
	return;
}

> +        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->compressed_size =
> +            compression_counters.compressed_size;
> +        info->compression->compression_rate =
> +            compression_counters.compression_rate;
> +    }
> +}


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 07/18] migration: Create ram_compressed_pages()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>

fix ram_compress

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.h | 1 +
 migration/ram-compress.c | 6 ++++++
 migration/ram.c          | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/migration/ram-compress.h b/migration/ram-compress.h
index 43ff44c0ba..77465dad4b 100644
--- a/migration/ram-compress.h
+++ b/migration/ram-compress.h
@@ -69,5 +69,6 @@ int compress_threads_load_setup(QEMUFile *f);
 void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
 
 void populate_compress(MigrationInfo *info);
+uint64_t ram_compressed_pages(void);
 
 #endif
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index a91c32588e..c48d5c53f6 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -502,3 +502,9 @@ void populate_compress(MigrationInfo *info)
             compression_counters.compression_rate;
     }
 }
+
+uint64_t ram_compressed_pages(void)
+{
+    return compression_counters.pages;
+}
+
diff --git a/migration/ram.c b/migration/ram.c
index d4943b982f..13c518f81a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -927,7 +927,7 @@ uint64_t ram_get_total_transferred_pages(void)
 {
     return stat64_get(&mig_stats.normal_pages) +
         stat64_get(&mig_stats.zero_pages) +
-        compression_counters.pages + xbzrle_counters.pages;
+        ram_compressed_pages() + xbzrle_counters.pages;
 }
 
 static void migration_update_rates(RAMState *rs, int64_t end_time)
-- 
2.40.1

Re: [PATCH 07/18] migration: Create ram_compressed_pages()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

On Tue, 13 Jun 2023 16:57:46 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> fix ram_compress
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Commit message is off. Other than that:

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.h | 1 +
>  migration/ram-compress.c | 6 ++++++
>  migration/ram.c          | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram-compress.h b/migration/ram-compress.h
> index 43ff44c0ba..77465dad4b 100644
> --- a/migration/ram-compress.h
> +++ b/migration/ram-compress.h
> @@ -69,5 +69,6 @@ int compress_threads_load_setup(QEMUFile *f);
>  void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
>  
>  void populate_compress(MigrationInfo *info);
> +uint64_t ram_compressed_pages(void);
>  
>  #endif
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index a91c32588e..c48d5c53f6 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -502,3 +502,9 @@ void populate_compress(MigrationInfo *info)
>              compression_counters.compression_rate;
>      }
>  }
> +
> +uint64_t ram_compressed_pages(void)
> +{
> +    return compression_counters.pages;
> +}
> +
> diff --git a/migration/ram.c b/migration/ram.c
> index d4943b982f..13c518f81a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -927,7 +927,7 @@ uint64_t ram_get_total_transferred_pages(void)
>  {
>      return stat64_get(&mig_stats.normal_pages) +
>          stat64_get(&mig_stats.zero_pages) +
> -        compression_counters.pages + xbzrle_counters.pages;
> +        ram_compressed_pages() + xbzrle_counters.pages;
>  }
>  
>  static void migration_update_rates(RAMState *rs, int64_t end_time)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 08/18] migration: Move update_compress_threads_counts() to ram-compress.c

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.h |  1 +
 migration/ram-compress.c | 17 +++++++++++++++++
 migration/ram.c          | 15 ---------------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/migration/ram-compress.h b/migration/ram-compress.h
index 77465dad4b..e55d3b50bd 100644
--- a/migration/ram-compress.h
+++ b/migration/ram-compress.h
@@ -70,5 +70,6 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
 
 void populate_compress(MigrationInfo *info);
 uint64_t ram_compressed_pages(void);
+void update_compress_thread_counts(const CompressParam *param, int bytes_xmit);
 
 #endif
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index c48d5c53f6..47b600d62b 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -32,11 +32,14 @@
 #include "ram-compress.h"
 
 #include "qemu/error-report.h"
+#include "qemu/stats64.h"
 #include "migration.h"
 #include "options.h"
 #include "io/channel-null.h"
 #include "exec/target_page.h"
 #include "exec/ramblock.h"
+#include "ram.h"
+#include "migration-stats.h"
 
 CompressionStats compression_counters;
 
@@ -508,3 +511,17 @@ uint64_t ram_compressed_pages(void)
     return compression_counters.pages;
 }
 
+void update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
+{
+    ram_transferred_add(bytes_xmit);
+
+    if (param->result == RES_ZEROPAGE) {
+        stat64_add(&mig_stats.zero_pages, 1);
+        return;
+    }
+
+    /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+    compression_counters.compressed_size += bytes_xmit - 8;
+    compression_counters.pages++;
+}
+
diff --git a/migration/ram.c b/migration/ram.c
index 13c518f81a..53dec4d305 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1259,21 +1259,6 @@ static int ram_save_multifd_page(QEMUFile *file, RAMBlock *block,
     return 1;
 }
 
-static void
-update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
-{
-    ram_transferred_add(bytes_xmit);
-
-    if (param->result == RES_ZEROPAGE) {
-        stat64_add(&mig_stats.zero_pages, 1);
-        return;
-    }
-
-    /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
-    compression_counters.compressed_size += bytes_xmit - 8;
-    compression_counters.pages++;
-}
-
 static int send_queued_data(CompressParam *param)
 {
     PageSearchStatus *pss = &ram_state->pss[RAM_CHANNEL_PRECOPY];
-- 
2.40.1

Re: [PATCH 08/18] migration: Move update_compress_threads_counts() to ram-compress.c

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]

On Tue, 13 Jun 2023 16:57:47 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.h |  1 +
>  migration/ram-compress.c | 17 +++++++++++++++++
>  migration/ram.c          | 15 ---------------
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/ram-compress.h b/migration/ram-compress.h
> index 77465dad4b..e55d3b50bd 100644
> --- a/migration/ram-compress.h
> +++ b/migration/ram-compress.h
> @@ -70,5 +70,6 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
>  
>  void populate_compress(MigrationInfo *info);
>  uint64_t ram_compressed_pages(void);
> +void update_compress_thread_counts(const CompressParam *param, int bytes_xmit);
>  
>  #endif
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index c48d5c53f6..47b600d62b 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -32,11 +32,14 @@
>  #include "ram-compress.h"
>  
>  #include "qemu/error-report.h"
> +#include "qemu/stats64.h"
>  #include "migration.h"
>  #include "options.h"
>  #include "io/channel-null.h"
>  #include "exec/target_page.h"
>  #include "exec/ramblock.h"
> +#include "ram.h"
> +#include "migration-stats.h"
>  
>  CompressionStats compression_counters;
>  
> @@ -508,3 +511,17 @@ uint64_t ram_compressed_pages(void)
>      return compression_counters.pages;
>  }
>  
> +void update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
> +{
> +    ram_transferred_add(bytes_xmit);
> +
> +    if (param->result == RES_ZEROPAGE) {
> +        stat64_add(&mig_stats.zero_pages, 1);
> +        return;
> +    }
> +
> +    /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> +    compression_counters.compressed_size += bytes_xmit - 8;
> +    compression_counters.pages++;
> +}
> +
> diff --git a/migration/ram.c b/migration/ram.c
> index 13c518f81a..53dec4d305 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1259,21 +1259,6 @@ static int ram_save_multifd_page(QEMUFile *file, RAMBlock *block,
>      return 1;
>  }
>  
> -static void
> -update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
> -{
> -    ram_transferred_add(bytes_xmit);
> -
> -    if (param->result == RES_ZEROPAGE) {
> -        stat64_add(&mig_stats.zero_pages, 1);
> -        return;
> -    }
> -
> -    /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
> -    compression_counters.compressed_size += bytes_xmit - 8;
> -    compression_counters.pages++;
> -}
> -
>  static int send_queued_data(CompressParam *param)
>  {
>      PageSearchStatus *pss = &ram_state->pss[RAM_CHANNEL_PRECOPY];


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 09/18] migration: Make compress_data_with_multithreads return bool

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.h |  4 ++--
 migration/ram-compress.c | 17 ++++++++++-------
 migration/ram.c          |  2 +-
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/migration/ram-compress.h b/migration/ram-compress.h
index e55d3b50bd..b228640092 100644
--- a/migration/ram-compress.h
+++ b/migration/ram-compress.h
@@ -60,8 +60,8 @@ void compress_threads_save_cleanup(void);
 int compress_threads_save_setup(void);
 
 void flush_compressed_data(int (send_queued_data(CompressParam *)));
-int compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
-                                int (send_queued_data(CompressParam *)));
+bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
+                                      int (send_queued_data(CompressParam *)));
 
 int wait_for_decompress_done(void);
 void compress_threads_load_cleanup(void);
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 47b600d62b..8ce9519f3d 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -262,10 +262,13 @@ static inline void set_compress_params(CompressParam *param, RAMBlock *block,
     param->trigger = true;
 }
 
-int compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
-                                int (send_queued_data(CompressParam *)))
+/*
+ * Return true when it compress a page
+ */
+bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
+                                     int (send_queued_data(CompressParam *)))
 {
-    int idx, thread_count, pages = -1;
+    int idx, thread_count;
     bool wait = migrate_compress_wait_thread();
 
     thread_count = migrate_compress_threads();
@@ -283,8 +286,8 @@ retry:
 
             qemu_cond_signal(&param->cond);
             qemu_mutex_unlock(&param->mutex);
-            pages = 1;
-            break;
+            qemu_mutex_unlock(&comp_done_lock);
+            return true;
         }
     }
 
@@ -292,13 +295,13 @@ retry:
      * wait for the free thread if the user specifies 'compress-wait-thread',
      * otherwise we will post the page out in the main thread as normal page.
      */
-    if (pages < 0 && wait) {
+    if (wait) {
         qemu_cond_wait(&comp_done_cond, &comp_done_lock);
         goto retry;
     }
     qemu_mutex_unlock(&comp_done_lock);
 
-    return pages;
+    return false;
 }
 
 /* return the size after decompression, or negative value on error */
diff --git a/migration/ram.c b/migration/ram.c
index 53dec4d305..2a3ff2c36e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2034,7 +2034,7 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
         return false;
     }
 
-    if (compress_page_with_multi_thread(block, offset, send_queued_data) > 0) {
+    if (compress_page_with_multi_thread(block, offset, send_queued_data)) {
         return true;
     }
 
-- 
2.40.1

Re: [PATCH 09/18] migration: Make compress_data_with_multithreads return bool

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 3446 bytes --]

On Tue, 13 Jun 2023 16:57:48 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Nit:
Probably makes sense to rename it to try_compress_page or similar.

And probably for zero_page and xbzrle (when factored out in it's own
function) too.

> ---
>  migration/ram-compress.h |  4 ++--
>  migration/ram-compress.c | 17 ++++++++++-------
>  migration/ram.c          |  2 +-
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/ram-compress.h b/migration/ram-compress.h
> index e55d3b50bd..b228640092 100644
> --- a/migration/ram-compress.h
> +++ b/migration/ram-compress.h
> @@ -60,8 +60,8 @@ void compress_threads_save_cleanup(void);
>  int compress_threads_save_setup(void);
>  
>  void flush_compressed_data(int (send_queued_data(CompressParam *)));
> -int compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
> -                                int (send_queued_data(CompressParam *)));
> +bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
> +                                      int (send_queued_data(CompressParam *)));
>  
>  int wait_for_decompress_done(void);
>  void compress_threads_load_cleanup(void);
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 47b600d62b..8ce9519f3d 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -262,10 +262,13 @@ static inline void set_compress_params(CompressParam *param, RAMBlock *block,
>      param->trigger = true;
>  }
>  
> -int compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
> -                                int (send_queued_data(CompressParam *)))
> +/*
> + * Return true when it compress a page
> + */
> +bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
> +                                     int (send_queued_data(CompressParam *)))
>  {
> -    int idx, thread_count, pages = -1;
> +    int idx, thread_count;
>      bool wait = migrate_compress_wait_thread();
>  
>      thread_count = migrate_compress_threads();
> @@ -283,8 +286,8 @@ retry:
>  
>              qemu_cond_signal(&param->cond);
>              qemu_mutex_unlock(&param->mutex);
> -            pages = 1;
> -            break;
> +            qemu_mutex_unlock(&comp_done_lock);
> +            return true;
>          }
>      }
>  
> @@ -292,13 +295,13 @@ retry:
>       * wait for the free thread if the user specifies 'compress-wait-thread',
>       * otherwise we will post the page out in the main thread as normal page.
>       */
> -    if (pages < 0 && wait) {
> +    if (wait) {
>          qemu_cond_wait(&comp_done_cond, &comp_done_lock);
>          goto retry;
>      }
>      qemu_mutex_unlock(&comp_done_lock);
>  
> -    return pages;
> +    return false;
>  }
>  
>  /* return the size after decompression, or negative value on error */
> diff --git a/migration/ram.c b/migration/ram.c
> index 53dec4d305..2a3ff2c36e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2034,7 +2034,7 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
>          return false;
>      }
>  
> -    if (compress_page_with_multi_thread(block, offset, send_queued_data) > 0) {
> +    if (compress_page_with_multi_thread(block, offset, send_queued_data)) {
>          return true;
>      }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 10/18] migration: Simplify compress_page_with_multithread()

[Juan Quintela]

Move the goto to a while true.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.c | 48 ++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 8ce9519f3d..3c44803132 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -273,35 +273,35 @@ bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
 
     thread_count = migrate_compress_threads();
     qemu_mutex_lock(&comp_done_lock);
-retry:
-    for (idx = 0; idx < thread_count; idx++) {
-        if (comp_param[idx].done) {
-            CompressParam *param = &comp_param[idx];
-            qemu_mutex_lock(&param->mutex);
-            param->done = false;
-            send_queued_data(param);
-            assert(qemu_file_buffer_empty(param->file));
-            compress_reset_result(param);
-            set_compress_params(param, block, offset);
 
-            qemu_cond_signal(&param->cond);
-            qemu_mutex_unlock(&param->mutex);
+    while (true) {
+        for (idx = 0; idx < thread_count; idx++) {
+            if (comp_param[idx].done) {
+                CompressParam *param = &comp_param[idx];
+                qemu_mutex_lock(&param->mutex);
+                param->done = false;
+                send_queued_data(param);
+                assert(qemu_file_buffer_empty(param->file));
+                compress_reset_result(param);
+                set_compress_params(param, block, offset);
+
+                qemu_cond_signal(&param->cond);
+                qemu_mutex_unlock(&param->mutex);
+                qemu_mutex_unlock(&comp_done_lock);
+                return true;
+            }
+        }
+        if (!wait) {
             qemu_mutex_unlock(&comp_done_lock);
-            return true;
+            return false;
         }
-    }
-
-    /*
-     * wait for the free thread if the user specifies 'compress-wait-thread',
-     * otherwise we will post the page out in the main thread as normal page.
-     */
-    if (wait) {
+        /*
+         * wait for a free thread if the user specifies
+         * 'compress-wait-thread', otherwise we will post the page out
+         * in the main thread as normal page.
+         */
         qemu_cond_wait(&comp_done_cond, &comp_done_lock);
-        goto retry;
     }
-    qemu_mutex_unlock(&comp_done_lock);
-
-    return false;
 }
 
 /* return the size after decompression, or negative value on error */
-- 
2.40.1

Re: [PATCH 10/18] migration: Simplify compress_page_with_multithread()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

On Tue, 13 Jun 2023 16:57:49 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Move the goto to a while true.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.c | 48 ++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 8ce9519f3d..3c44803132 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -273,35 +273,35 @@ bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
>  
>      thread_count = migrate_compress_threads();
>      qemu_mutex_lock(&comp_done_lock);
> -retry:
> -    for (idx = 0; idx < thread_count; idx++) {
> -        if (comp_param[idx].done) {
> -            CompressParam *param = &comp_param[idx];
> -            qemu_mutex_lock(&param->mutex);
> -            param->done = false;
> -            send_queued_data(param);
> -            assert(qemu_file_buffer_empty(param->file));
> -            compress_reset_result(param);
> -            set_compress_params(param, block, offset);
>  
> -            qemu_cond_signal(&param->cond);
> -            qemu_mutex_unlock(&param->mutex);
> +    while (true) {
> +        for (idx = 0; idx < thread_count; idx++) {
> +            if (comp_param[idx].done) {
> +                CompressParam *param = &comp_param[idx];
> +                qemu_mutex_lock(&param->mutex);
> +                param->done = false;
> +                send_queued_data(param);
> +                assert(qemu_file_buffer_empty(param->file));
> +                compress_reset_result(param);
> +                set_compress_params(param, block, offset);
> +
> +                qemu_cond_signal(&param->cond);
> +                qemu_mutex_unlock(&param->mutex);
> +                qemu_mutex_unlock(&comp_done_lock);
> +                return true;
> +            }
> +        }
> +        if (!wait) {
>              qemu_mutex_unlock(&comp_done_lock);
> -            return true;
> +            return false;
>          }
> -    }
> -
> -    /*
> -     * wait for the free thread if the user specifies 'compress-wait-thread',
> -     * otherwise we will post the page out in the main thread as normal page.
> -     */
> -    if (wait) {
> +        /*
> +         * wait for a free thread if the user specifies
> +         * 'compress-wait-thread', otherwise we will post the page out
> +         * in the main thread as normal page.
> +         */
>          qemu_cond_wait(&comp_done_cond, &comp_done_lock);
> -        goto retry;
>      }
> -    qemu_mutex_unlock(&comp_done_lock);
> -
> -    return false;
>  }
>  
>  /* return the size after decompression, or negative value on error */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 11/18] migration: Move busy++ to migrate_with_multithread

[Juan Quintela]

And now we can simplify save_compress_page().

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.c | 1 +
 migration/ram.c          | 7 +------
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 3c44803132..2652cdee8b 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -293,6 +293,7 @@ bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
         }
         if (!wait) {
             qemu_mutex_unlock(&comp_done_lock);
+            compression_counters.busy++;
             return false;
         }
         /*
diff --git a/migration/ram.c b/migration/ram.c
index 2a3ff2c36e..60f24006bc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2034,12 +2034,7 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
         return false;
     }
 
-    if (compress_page_with_multi_thread(block, offset, send_queued_data)) {
-        return true;
-    }
-
-    compression_counters.busy++;
-    return false;
+    return compress_page_with_multi_thread(block, offset, send_queued_data);
 }
 
 /**
-- 
2.40.1

Re: [PATCH 11/18] migration: Move busy++ to migrate_with_multithread

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On Tue, 13 Jun 2023 16:57:50 +0200
Juan Quintela <quintela@redhat.com> wrote:

> And now we can simplify save_compress_page().
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.c | 1 +
>  migration/ram.c          | 7 +------
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 3c44803132..2652cdee8b 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -293,6 +293,7 @@ bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
>          }
>          if (!wait) {
>              qemu_mutex_unlock(&comp_done_lock);
> +            compression_counters.busy++;
>              return false;
>          }
>          /*
> diff --git a/migration/ram.c b/migration/ram.c
> index 2a3ff2c36e..60f24006bc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2034,12 +2034,7 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
>          return false;
>      }
>  
> -    if (compress_page_with_multi_thread(block, offset, send_queued_data)) {
> -        return true;
> -    }
> -
> -    compression_counters.busy++;
> -    return false;
> +    return compress_page_with_multi_thread(block, offset, send_queued_data);
>  }
>  
>  /**


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 12/18] migration: Create compression_update_rates()

[Juan Quintela]

So we can move more compression_counters stuff to ram-compress.c.
Create compression_counters struct to add the stuff that was on
MigrationState.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.h |  1 +
 migration/ram.h          |  1 -
 migration/ram-compress.c | 41 +++++++++++++++++++++++++++++++++++++++-
 migration/ram.c          | 29 +---------------------------
 4 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/migration/ram-compress.h b/migration/ram-compress.h
index b228640092..a7eb028849 100644
--- a/migration/ram-compress.h
+++ b/migration/ram-compress.h
@@ -71,5 +71,6 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
 void populate_compress(MigrationInfo *info);
 uint64_t ram_compressed_pages(void);
 void update_compress_thread_counts(const CompressParam *param, int bytes_xmit);
+void compression_update_rates(uint64_t page_count);
 
 #endif
diff --git a/migration/ram.h b/migration/ram.h
index ea1f3c25b5..60bc4c9e3a 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -34,7 +34,6 @@
 #include "io/channel.h"
 
 extern XBZRLECacheStats xbzrle_counters;
-extern CompressionStats compression_counters;
 
 bool ramblock_is_ignored(RAMBlock *block);
 /* Should be holding either ram_list.mutex, or the RCU lock. */
diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 2652cdee8b..5e1bb82fcd 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -41,7 +41,20 @@
 #include "ram.h"
 #include "migration-stats.h"
 
-CompressionStats compression_counters;
+static struct {
+    int64_t pages;
+    int64_t busy;
+    double busy_rate;
+    int64_t compressed_size;
+    double compression_rate;
+    /* 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 after compression */
+    uint64_t compressed_size_prev;
+    /* amount of compressed pages */
+    uint64_t compress_pages_prev;
+} compression_counters;
 
 static CompressParam *comp_param;
 static QemuThread *compress_threads;
@@ -529,3 +542,29 @@ void update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
     compression_counters.pages++;
 }
 
+void compression_update_rates(uint64_t page_count)
+{
+    if (migrate_compress()) {
+        compression_counters.busy_rate = (double)(compression_counters.busy -
+            compression_counters.compress_thread_busy_prev) / page_count;
+        compression_counters.compress_thread_busy_prev =
+            compression_counters.busy;
+
+        double compressed_size = compression_counters.compressed_size -
+                                 compression_counters.compressed_size_prev;
+        if (compressed_size) {
+            double uncompressed_size = (compression_counters.pages -
+                compression_counters.compress_pages_prev) *
+                qemu_target_page_size();
+
+            /* Compression-Ratio = Uncompressed-size / Compressed-size */
+            compression_counters.compression_rate =
+                                        uncompressed_size / compressed_size;
+
+            compression_counters.compress_pages_prev =
+                compression_counters.pages;
+            compression_counters.compressed_size_prev =
+                compression_counters.compressed_size;
+        }
+    }
+}
diff --git a/migration/ram.c b/migration/ram.c
index 60f24006bc..1bd586c23a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -364,13 +364,6 @@ struct RAMState {
     bool xbzrle_started;
     /* Are we on the last stage of migration */
     bool last_stage;
-    /* 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 after compression */
-    uint64_t compressed_size_prev;
-    /* amount of compressed pages */
-    uint64_t compress_pages_prev;
 
     /* total handled target pages at the beginning of period */
     uint64_t target_page_count_prev;
@@ -933,7 +926,6 @@ uint64_t ram_get_total_transferred_pages(void)
 static void migration_update_rates(RAMState *rs, int64_t end_time)
 {
     uint64_t page_count = rs->target_page_count - rs->target_page_count_prev;
-    double compressed_size;
 
     /* calculate period counters */
     stat64_set(&mig_stats.dirty_pages_rate,
@@ -961,26 +953,7 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
         rs->xbzrle_pages_prev = xbzrle_counters.pages;
         rs->xbzrle_bytes_prev = xbzrle_counters.bytes;
     }
-
-    if (migrate_compress()) {
-        compression_counters.busy_rate = (double)(compression_counters.busy -
-            rs->compress_thread_busy_prev) / page_count;
-        rs->compress_thread_busy_prev = compression_counters.busy;
-
-        compressed_size = compression_counters.compressed_size -
-                          rs->compressed_size_prev;
-        if (compressed_size) {
-            double uncompressed_size = (compression_counters.pages -
-                                    rs->compress_pages_prev) * TARGET_PAGE_SIZE;
-
-            /* Compression-Ratio = Uncompressed-size / Compressed-size */
-            compression_counters.compression_rate =
-                                        uncompressed_size / compressed_size;
-
-            rs->compress_pages_prev = compression_counters.pages;
-            rs->compressed_size_prev = compression_counters.compressed_size;
-        }
-    }
+    compression_update_rates(page_count);
 }
 
 static void migration_trigger_throttle(RAMState *rs)
-- 
2.40.1

Re: [PATCH 12/18] migration: Create compression_update_rates()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 6296 bytes --]

On Tue, 13 Jun 2023 16:57:51 +0200
Juan Quintela <quintela@redhat.com> wrote:

> So we can move more compression_counters stuff to ram-compress.c.
> Create compression_counters struct to add the stuff that was on
> MigrationState.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

With the comment below fixed:
Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.h |  1 +
>  migration/ram.h          |  1 -
>  migration/ram-compress.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  migration/ram.c          | 29 +---------------------------
>  4 files changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/migration/ram-compress.h b/migration/ram-compress.h
> index b228640092..a7eb028849 100644
> --- a/migration/ram-compress.h
> +++ b/migration/ram-compress.h
> @@ -71,5 +71,6 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len);
>  void populate_compress(MigrationInfo *info);
>  uint64_t ram_compressed_pages(void);
>  void update_compress_thread_counts(const CompressParam *param, int bytes_xmit);
> +void compression_update_rates(uint64_t page_count);
>  
>  #endif
> diff --git a/migration/ram.h b/migration/ram.h
> index ea1f3c25b5..60bc4c9e3a 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -34,7 +34,6 @@
>  #include "io/channel.h"
>  
>  extern XBZRLECacheStats xbzrle_counters;
> -extern CompressionStats compression_counters;
>  
>  bool ramblock_is_ignored(RAMBlock *block);
>  /* Should be holding either ram_list.mutex, or the RCU lock. */
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 2652cdee8b..5e1bb82fcd 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -41,7 +41,20 @@
>  #include "ram.h"
>  #include "migration-stats.h"
>  
> -CompressionStats compression_counters;
> +static struct {
> +    int64_t pages;
> +    int64_t busy;
> +    double busy_rate;
> +    int64_t compressed_size;
> +    double compression_rate;
> +    /* 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 after compression */
> +    uint64_t compressed_size_prev;
> +    /* amount of compressed pages */
> +    uint64_t compress_pages_prev;
> +} compression_counters;
>  
>  static CompressParam *comp_param;
>  static QemuThread *compress_threads;
> @@ -529,3 +542,29 @@ void update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
>      compression_counters.pages++;
>  }
>  
> +void compression_update_rates(uint64_t page_count)
> +{
> +    if (migrate_compress()) {

Use early return.

> +        compression_counters.busy_rate = (double)(compression_counters.busy -
> +            compression_counters.compress_thread_busy_prev) / page_count;
> +        compression_counters.compress_thread_busy_prev =
> +            compression_counters.busy;
> +
> +        double compressed_size = compression_counters.compressed_size -
> +                                 compression_counters.compressed_size_prev;
> +        if (compressed_size) {
> +            double uncompressed_size = (compression_counters.pages -
> +                compression_counters.compress_pages_prev) *
> +                qemu_target_page_size();
> +
> +            /* Compression-Ratio = Uncompressed-size / Compressed-size */
> +            compression_counters.compression_rate =
> +                                        uncompressed_size / compressed_size;
> +
> +            compression_counters.compress_pages_prev =
> +                compression_counters.pages;
> +            compression_counters.compressed_size_prev =
> +                compression_counters.compressed_size;
> +        }
> +    }
> +}
> diff --git a/migration/ram.c b/migration/ram.c
> index 60f24006bc..1bd586c23a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -364,13 +364,6 @@ struct RAMState {
>      bool xbzrle_started;
>      /* Are we on the last stage of migration */
>      bool last_stage;
> -    /* 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 after compression */
> -    uint64_t compressed_size_prev;
> -    /* amount of compressed pages */
> -    uint64_t compress_pages_prev;
>  
>      /* total handled target pages at the beginning of period */
>      uint64_t target_page_count_prev;
> @@ -933,7 +926,6 @@ uint64_t ram_get_total_transferred_pages(void)
>  static void migration_update_rates(RAMState *rs, int64_t end_time)
>  {
>      uint64_t page_count = rs->target_page_count - rs->target_page_count_prev;
> -    double compressed_size;
>  
>      /* calculate period counters */
>      stat64_set(&mig_stats.dirty_pages_rate,
> @@ -961,26 +953,7 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>          rs->xbzrle_pages_prev = xbzrle_counters.pages;
>          rs->xbzrle_bytes_prev = xbzrle_counters.bytes;
>      }
> -
> -    if (migrate_compress()) {
> -        compression_counters.busy_rate = (double)(compression_counters.busy -
> -            rs->compress_thread_busy_prev) / page_count;
> -        rs->compress_thread_busy_prev = compression_counters.busy;
> -
> -        compressed_size = compression_counters.compressed_size -
> -                          rs->compressed_size_prev;
> -        if (compressed_size) {
> -            double uncompressed_size = (compression_counters.pages -
> -                                    rs->compress_pages_prev) * TARGET_PAGE_SIZE;
> -
> -            /* Compression-Ratio = Uncompressed-size / Compressed-size */
> -            compression_counters.compression_rate =
> -                                        uncompressed_size / compressed_size;
> -
> -            rs->compress_pages_prev = compression_counters.pages;
> -            rs->compressed_size_prev = compression_counters.compressed_size;
> -        }
> -    }
> +    compression_update_rates(page_count);
>  }
>  
>  static void migration_trigger_throttle(RAMState *rs)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 13/18] migration: Simplify decompress_data_with_multi_threads()

[Juan Quintela]

Doing a break to do another break is just confused.  Just call return
when we know we want to return.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index 5e1bb82fcd..cf3a6c238d 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -498,14 +498,10 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
                 decomp_param[idx].len = len;
                 qemu_cond_signal(&decomp_param[idx].cond);
                 qemu_mutex_unlock(&decomp_param[idx].mutex);
-                break;
+                return;
             }
         }
-        if (idx < thread_count) {
-            break;
-        } else {
-            qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
-        }
+        qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
     }
 }
 
-- 
2.40.1

Re: [PATCH 13/18] migration: Simplify decompress_data_with_multi_threads()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Tue, 13 Jun 2023 16:57:52 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Doing a break to do another break is just confused.  Just call return
> when we know we want to return.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index 5e1bb82fcd..cf3a6c238d 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -498,14 +498,10 @@ void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
>                  decomp_param[idx].len = len;
>                  qemu_cond_signal(&decomp_param[idx].cond);
>                  qemu_mutex_unlock(&decomp_param[idx].mutex);
> -                break;
> +                return;
>              }
>          }
> -        if (idx < thread_count) {
> -            break;
> -        } else {
> -            qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> -        }
> +        qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
>      }
>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 14/18] migration: Use "i" as an for index in ram-compress.c

[Juan Quintela]

It is used everywhere else in C.  Once there, make sure that we don't
use the index outside of the for declaring the variable there.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram-compress.c | 57 ++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/migration/ram-compress.c b/migration/ram-compress.c
index cf3a6c238d..a23531606e 100644
--- a/migration/ram-compress.c
+++ b/migration/ram-compress.c
@@ -243,27 +243,25 @@ static inline void compress_reset_result(CompressParam *param)
 
 void flush_compressed_data(int (send_queued_data(CompressParam *)))
 {
-    int idx, thread_count;
-
-    thread_count = migrate_compress_threads();
+    int thread_count = migrate_compress_threads();
 
     qemu_mutex_lock(&comp_done_lock);
-    for (idx = 0; idx < thread_count; idx++) {
-        while (!comp_param[idx].done) {
+    for (int i = 0; i < thread_count; i++) {
+        while (!comp_param[i].done) {
             qemu_cond_wait(&comp_done_cond, &comp_done_lock);
         }
     }
     qemu_mutex_unlock(&comp_done_lock);
 
-    for (idx = 0; idx < thread_count; idx++) {
-        qemu_mutex_lock(&comp_param[idx].mutex);
-        if (!comp_param[idx].quit) {
-            CompressParam *param = &comp_param[idx];
+    for (int i = 0; i < thread_count; i++) {
+        qemu_mutex_lock(&comp_param[i].mutex);
+        if (!comp_param[i].quit) {
+            CompressParam *param = &comp_param[i];
             send_queued_data(param);
             assert(qemu_file_buffer_empty(param->file));
             compress_reset_result(param);
         }
-        qemu_mutex_unlock(&comp_param[idx].mutex);
+        qemu_mutex_unlock(&comp_param[i].mutex);
     }
 }
 
@@ -281,16 +279,15 @@ static inline void set_compress_params(CompressParam *param, RAMBlock *block,
 bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
                                      int (send_queued_data(CompressParam *)))
 {
-    int idx, thread_count;
     bool wait = migrate_compress_wait_thread();
 
-    thread_count = migrate_compress_threads();
+    int 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) {
-                CompressParam *param = &comp_param[idx];
+        for (int i = 0; i < thread_count; i++) {
+            if (comp_param[i].done) {
+                CompressParam *param = &comp_param[i];
                 qemu_mutex_lock(&param->mutex);
                 param->done = false;
                 send_queued_data(param);
@@ -384,16 +381,14 @@ static void *do_data_decompress(void *opaque)
 
 int wait_for_decompress_done(void)
 {
-    int idx, thread_count;
-
     if (!migrate_compress()) {
         return 0;
     }
 
-    thread_count = migrate_decompress_threads();
+    int thread_count = migrate_decompress_threads();
     qemu_mutex_lock(&decomp_done_lock);
-    for (idx = 0; idx < thread_count; idx++) {
-        while (!decomp_param[idx].done) {
+    for (int i = 0; i < thread_count; i++) {
+        while (!decomp_param[i].done) {
             qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
         }
     }
@@ -484,20 +479,18 @@ exit:
 
 void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
 {
-    int idx, thread_count;
-
-    thread_count = migrate_decompress_threads();
+    int thread_count = migrate_decompress_threads();
     QEMU_LOCK_GUARD(&decomp_done_lock);
     while (true) {
-        for (idx = 0; idx < thread_count; idx++) {
-            if (decomp_param[idx].done) {
-                decomp_param[idx].done = false;
-                qemu_mutex_lock(&decomp_param[idx].mutex);
-                qemu_get_buffer(f, decomp_param[idx].compbuf, len);
-                decomp_param[idx].des = host;
-                decomp_param[idx].len = len;
-                qemu_cond_signal(&decomp_param[idx].cond);
-                qemu_mutex_unlock(&decomp_param[idx].mutex);
+        for (int i = 0; i < thread_count; i++) {
+            if (decomp_param[i].done) {
+                decomp_param[i].done = false;
+                qemu_mutex_lock(&decomp_param[i].mutex);
+                qemu_get_buffer(f, decomp_param[i].compbuf, len);
+                decomp_param[i].des = host;
+                decomp_param[i].len = len;
+                qemu_cond_signal(&decomp_param[i].cond);
+                qemu_mutex_unlock(&decomp_param[i].mutex);
                 return;
             }
         }
-- 
2.40.1

Re: [PATCH 14/18] migration: Use "i" as an for index in ram-compress.c

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]

On Tue, 13 Jun 2023 16:57:53 +0200
Juan Quintela <quintela@redhat.com> wrote:

> It is used everywhere else in C.  Once there, make sure that we don't
> use the index outside of the for declaring the variable there.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram-compress.c | 57 ++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/migration/ram-compress.c b/migration/ram-compress.c
> index cf3a6c238d..a23531606e 100644
> --- a/migration/ram-compress.c
> +++ b/migration/ram-compress.c
> @@ -243,27 +243,25 @@ static inline void compress_reset_result(CompressParam *param)
>  
>  void flush_compressed_data(int (send_queued_data(CompressParam *)))
>  {
> -    int idx, thread_count;
> -
> -    thread_count = migrate_compress_threads();
> +    int thread_count = migrate_compress_threads();
>  
>      qemu_mutex_lock(&comp_done_lock);
> -    for (idx = 0; idx < thread_count; idx++) {
> -        while (!comp_param[idx].done) {
> +    for (int i = 0; i < thread_count; i++) {
> +        while (!comp_param[i].done) {
>              qemu_cond_wait(&comp_done_cond, &comp_done_lock);
>          }
>      }
>      qemu_mutex_unlock(&comp_done_lock);
>  
> -    for (idx = 0; idx < thread_count; idx++) {
> -        qemu_mutex_lock(&comp_param[idx].mutex);
> -        if (!comp_param[idx].quit) {
> -            CompressParam *param = &comp_param[idx];
> +    for (int i = 0; i < thread_count; i++) {
> +        qemu_mutex_lock(&comp_param[i].mutex);
> +        if (!comp_param[i].quit) {
> +            CompressParam *param = &comp_param[i];
>              send_queued_data(param);
>              assert(qemu_file_buffer_empty(param->file));
>              compress_reset_result(param);
>          }
> -        qemu_mutex_unlock(&comp_param[idx].mutex);
> +        qemu_mutex_unlock(&comp_param[i].mutex);
>      }
>  }
>  
> @@ -281,16 +279,15 @@ static inline void set_compress_params(CompressParam *param, RAMBlock *block,
>  bool compress_page_with_multi_thread(RAMBlock *block, ram_addr_t offset,
>                                       int (send_queued_data(CompressParam *)))
>  {
> -    int idx, thread_count;
>      bool wait = migrate_compress_wait_thread();
>  
> -    thread_count = migrate_compress_threads();
> +    int 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) {
> -                CompressParam *param = &comp_param[idx];
> +        for (int i = 0; i < thread_count; i++) {
> +            if (comp_param[i].done) {
> +                CompressParam *param = &comp_param[i];
>                  qemu_mutex_lock(&param->mutex);
>                  param->done = false;
>                  send_queued_data(param);
> @@ -384,16 +381,14 @@ static void *do_data_decompress(void *opaque)
>  
>  int wait_for_decompress_done(void)
>  {
> -    int idx, thread_count;
> -
>      if (!migrate_compress()) {
>          return 0;
>      }
>  
> -    thread_count = migrate_decompress_threads();
> +    int thread_count = migrate_decompress_threads();
>      qemu_mutex_lock(&decomp_done_lock);
> -    for (idx = 0; idx < thread_count; idx++) {
> -        while (!decomp_param[idx].done) {
> +    for (int i = 0; i < thread_count; i++) {
> +        while (!decomp_param[i].done) {
>              qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
>          }
>      }
> @@ -484,20 +479,18 @@ exit:
>  
>  void decompress_data_with_multi_threads(QEMUFile *f, void *host, int len)
>  {
> -    int idx, thread_count;
> -
> -    thread_count = migrate_decompress_threads();
> +    int thread_count = migrate_decompress_threads();
>      QEMU_LOCK_GUARD(&decomp_done_lock);
>      while (true) {
> -        for (idx = 0; idx < thread_count; idx++) {
> -            if (decomp_param[idx].done) {
> -                decomp_param[idx].done = false;
> -                qemu_mutex_lock(&decomp_param[idx].mutex);
> -                qemu_get_buffer(f, decomp_param[idx].compbuf, len);
> -                decomp_param[idx].des = host;
> -                decomp_param[idx].len = len;
> -                qemu_cond_signal(&decomp_param[idx].cond);
> -                qemu_mutex_unlock(&decomp_param[idx].mutex);
> +        for (int i = 0; i < thread_count; i++) {
> +            if (decomp_param[i].done) {
> +                decomp_param[i].done = false;
> +                qemu_mutex_lock(&decomp_param[i].mutex);
> +                qemu_get_buffer(f, decomp_param[i].compbuf, len);
> +                decomp_param[i].des = host;
> +                decomp_param[i].len = len;
> +                qemu_cond_signal(&decomp_param[i].cond);
> +                qemu_mutex_unlock(&decomp_param[i].mutex);
>                  return;
>              }
>          }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 15/18] migration: save_compress_page() can take block through pss

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 1bd586c23a..e3ff98a6ce 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1986,7 +1986,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
  * paths to handle it
  */
 static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
-                               RAMBlock *block, ram_addr_t offset)
+                               ram_addr_t offset)
 {
     if (!migrate_compress()) {
         return false;
@@ -2002,12 +2002,13 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
      * We post the fist page as normal page as compression will take
      * much CPU resource.
      */
-    if (block != pss->last_sent_block) {
+    if (pss->block != pss->last_sent_block) {
         ram_flush_compressed_data();
         return false;
     }
 
-    return compress_page_with_multi_thread(block, offset, send_queued_data);
+    return compress_page_with_multi_thread(pss->block, offset,
+                                           send_queued_data);
 }
 
 /**
@@ -2028,7 +2029,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return res;
     }
 
-    if (save_compress_page(rs, pss, block, offset)) {
+    if (save_compress_page(rs, pss, offset)) {
         return 1;
     }
 
-- 
2.40.1

Re: [PATCH 15/18] migration: save_compress_page() can take block through pss

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Tue, 13 Jun 2023 16:57:54 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 1bd586c23a..e3ff98a6ce 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1986,7 +1986,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
>   * paths to handle it
>   */
>  static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
> -                               RAMBlock *block, ram_addr_t offset)
> +                               ram_addr_t offset)
>  {
>      if (!migrate_compress()) {
>          return false;
> @@ -2002,12 +2002,13 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
>       * We post the fist page as normal page as compression will take
>       * much CPU resource.
>       */
> -    if (block != pss->last_sent_block) {
> +    if (pss->block != pss->last_sent_block) {
>          ram_flush_compressed_data();
>          return false;
>      }
>  
> -    return compress_page_with_multi_thread(block, offset, send_queued_data);
> +    return compress_page_with_multi_thread(pss->block, offset,
> +                                           send_queued_data);
>  }
>  
>  /**
> @@ -2028,7 +2029,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return res;
>      }
>  
> -    if (save_compress_page(rs, pss, block, offset)) {
> +    if (save_compress_page(rs, pss, offset)) {
>          return 1;
>      }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 16/18] migration: control_save_page() can take block through pss

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index e3ff98a6ce..293ea87a70 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1114,14 +1114,14 @@ static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
  *
  * Return true if the pages has been saved, otherwise false is returned.
  */
-static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
+static bool control_save_page(PageSearchStatus *pss,
                               ram_addr_t offset, int *pages)
 {
     uint64_t bytes_xmit = 0;
     int ret;
 
     *pages = -1;
-    ret = ram_control_save_page(pss->pss_channel, block->offset, offset,
+    ret = ram_control_save_page(pss->pss_channel, pss->block->offset, offset,
                                 TARGET_PAGE_SIZE, &bytes_xmit);
     if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
         return false;
@@ -2025,7 +2025,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
     ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
     int res;
 
-    if (control_save_page(pss, block, offset, &res)) {
+    if (control_save_page(pss, offset, &res)) {
         return res;
     }
 
-- 
2.40.1

Re: [PATCH 16/18] migration: control_save_page() can take block through pss

[Lukas Straub]

On Tue, 13 Jun 2023 16:57:55 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index e3ff98a6ce..293ea87a70 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1114,14 +1114,14 @@ static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
>   *
>   * Return true if the pages has been saved, otherwise false is returned.
>   */
> -static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
> +static bool control_save_page(PageSearchStatus *pss,
>                                ram_addr_t offset, int *pages)
>  {
>      uint64_t bytes_xmit = 0;
>      int ret;
>
>      *pages = -1;
> -    ret = ram_control_save_page(pss->pss_channel, block->offset, offset,
> +    ret = ram_control_save_page(pss->pss_channel, pss->block->offset, offset,
>                                  TARGET_PAGE_SIZE, &bytes_xmit);
>      if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
>          return false;
> @@ -2025,7 +2025,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>      ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
>      int res;
>
> -    if (control_save_page(pss, block, offset, &res)) {
> +    if (control_save_page(pss, offset, &res)) {
>          return res;
>      }
>

[PATCH 17/18] migration: Remove not needed block parameter for save_zero_page*

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 293ea87a70..0259c33da7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1067,20 +1067,20 @@ void ram_release_page(const char *rbname, uint64_t offset)
  * a zero page
  *
  * @pss: current PSS channel
- * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
 static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
-                                  RAMBlock *block, ram_addr_t offset)
+                                  ram_addr_t offset)
 {
-    uint8_t *p = block->host + offset;
+    uint8_t *p = pss->block->host + offset;
     int len = 0;
 
     if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
-        len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
+        len += save_page_header(pss, file, pss->block,
+                                offset | RAM_SAVE_FLAG_ZERO);
         qemu_put_byte(file, 0);
         len += 1;
-        ram_release_page(block->idstr, offset);
+        ram_release_page(pss->block->idstr, offset);
     }
     return len;
 }
@@ -1091,13 +1091,11 @@ static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
  * Returns the number of pages written.
  *
  * @pss: current PSS channel
- * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
-                          ram_addr_t offset)
+static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, ram_addr_t offset)
 {
-    int len = save_zero_page_to_file(pss, f, block, offset);
+    int len = save_zero_page_to_file(pss, f, offset);
 
     if (len) {
         stat64_add(&mig_stats.zero_pages, 1);
@@ -2033,7 +2031,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return 1;
     }
 
-    res = save_zero_page(pss, pss->pss_channel, block, offset);
+    res = save_zero_page(pss, pss->pss_channel, offset);
     if (res > 0) {
         /* Must let xbzrle know, otherwise a previous (now 0'd) cached
          * page would be stale
-- 
2.40.1

Re: [PATCH 17/18] migration: Remove not needed block parameter for save_zero_page*

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

On Tue, 13 Jun 2023 16:57:56 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 293ea87a70..0259c33da7 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1067,20 +1067,20 @@ void ram_release_page(const char *rbname, uint64_t offset)
>   * a zero page
>   *
>   * @pss: current PSS channel
> - * @block: block that contains the page we want to send
>   * @offset: offset inside the block for the page
>   */
>  static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
> -                                  RAMBlock *block, ram_addr_t offset)
> +                                  ram_addr_t offset)
>  {
> -    uint8_t *p = block->host + offset;
> +    uint8_t *p = pss->block->host + offset;
>      int len = 0;
>  
>      if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> -        len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
> +        len += save_page_header(pss, file, pss->block,
> +                                offset | RAM_SAVE_FLAG_ZERO);
>          qemu_put_byte(file, 0);
>          len += 1;
> -        ram_release_page(block->idstr, offset);
> +        ram_release_page(pss->block->idstr, offset);
>      }
>      return len;
>  }
> @@ -1091,13 +1091,11 @@ static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
>   * Returns the number of pages written.
>   *
>   * @pss: current PSS channel
> - * @block: block that contains the page we want to send
>   * @offset: offset inside the block for the page
>   */
> -static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
> -                          ram_addr_t offset)
> +static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, ram_addr_t offset)
>  {
> -    int len = save_zero_page_to_file(pss, f, block, offset);
> +    int len = save_zero_page_to_file(pss, f, offset);
>  
>      if (len) {
>          stat64_add(&mig_stats.zero_pages, 1);
> @@ -2033,7 +2031,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return 1;
>      }
>  
> -    res = save_zero_page(pss, pss->pss_channel, block, offset);
> +    res = save_zero_page(pss, pss->pss_channel, offset);
>      if (res > 0) {
>          /* Must let xbzrle know, otherwise a previous (now 0'd) cached
>           * page would be stale


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 18/18] migration: Remove not needed file parameter for save_zero_page*

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 0259c33da7..ae11d0e992 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1069,16 +1069,15 @@ void ram_release_page(const char *rbname, uint64_t offset)
  * @pss: current PSS channel
  * @offset: offset inside the block for the page
  */
-static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
-                                  ram_addr_t offset)
+static int save_zero_page_to_file(PageSearchStatus *pss, ram_addr_t offset)
 {
     uint8_t *p = pss->block->host + offset;
     int len = 0;
 
     if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
-        len += save_page_header(pss, file, pss->block,
+        len += save_page_header(pss, pss->pss_channel, pss->block,
                                 offset | RAM_SAVE_FLAG_ZERO);
-        qemu_put_byte(file, 0);
+        qemu_put_byte(pss->pss_channel, 0);
         len += 1;
         ram_release_page(pss->block->idstr, offset);
     }
@@ -1093,9 +1092,9 @@ static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
  * @pss: current PSS channel
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, ram_addr_t offset)
+static int save_zero_page(PageSearchStatus *pss, ram_addr_t offset)
 {
-    int len = save_zero_page_to_file(pss, f, offset);
+    int len = save_zero_page_to_file(pss, offset);
 
     if (len) {
         stat64_add(&mig_stats.zero_pages, 1);
@@ -2031,7 +2030,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
         return 1;
     }
 
-    res = save_zero_page(pss, pss->pss_channel, offset);
+    res = save_zero_page(pss, offset);
     if (res > 0) {
         /* Must let xbzrle know, otherwise a previous (now 0'd) cached
          * page would be stale
-- 
2.40.1

Re: [PATCH 18/18] migration: Remove not needed file parameter for save_zero_page*

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

On Tue, 13 Jun 2023 16:57:57 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> ---
>  migration/ram.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0259c33da7..ae11d0e992 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1069,16 +1069,15 @@ void ram_release_page(const char *rbname, uint64_t offset)
>   * @pss: current PSS channel
>   * @offset: offset inside the block for the page
>   */
> -static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
> -                                  ram_addr_t offset)
> +static int save_zero_page_to_file(PageSearchStatus *pss, ram_addr_t offset)
>  {
>      uint8_t *p = pss->block->host + offset;
>      int len = 0;
>  
>      if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> -        len += save_page_header(pss, file, pss->block,
> +        len += save_page_header(pss, pss->pss_channel, pss->block,
>                                  offset | RAM_SAVE_FLAG_ZERO);
> -        qemu_put_byte(file, 0);
> +        qemu_put_byte(pss->pss_channel, 0);
>          len += 1;
>          ram_release_page(pss->block->idstr, offset);
>      }
> @@ -1093,9 +1092,9 @@ static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
>   * @pss: current PSS channel
>   * @offset: offset inside the block for the page
>   */
> -static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, ram_addr_t offset)
> +static int save_zero_page(PageSearchStatus *pss, ram_addr_t offset)
>  {
> -    int len = save_zero_page_to_file(pss, f, offset);
> +    int len = save_zero_page_to_file(pss, offset);
>  
>      if (len) {
>          stat64_add(&mig_stats.zero_pages, 1);
> @@ -2031,7 +2030,7 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return 1;
>      }
>  
> -    res = save_zero_page(pss, pss->pss_channel, offset);
> +    res = save_zero_page(pss, offset);
>      if (res > 0) {
>          /* Must let xbzrle know, otherwise a previous (now 0'd) cached
>           * page would be stale


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]