[Qemu-devel] [PATCH 40/54] ram: Remove compression_switch and inline its logic

Juan Quintela posted 54 patches 8 years, 10 months ago
Only 50 patches received!
[Qemu-devel] [PATCH 40/54] ram: Remove compression_switch and inline its logic
Posted by Juan Quintela 8 years, 10 months ago
We can calculate its value, so not create a varible for it.

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

--

After Peter and Dave review, I dropped the variable and just inlined
the condition.

Later, Juan.

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

diff --git a/migration/ram.c b/migration/ram.c
index 14d54fd..5dfc9bd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -336,7 +336,6 @@ static QemuCond comp_done_cond;
 /* The empty QEMUFileOps will be used by file in CompressParam */
 static const QEMUFileOps empty_ops = { };
 
-static bool compression_switch;
 static DecompressParam *decomp_param;
 static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
@@ -420,7 +419,6 @@ void migrate_compress_threads_create(void)
     if (!migrate_use_compression()) {
         return;
     }
-    compression_switch = true;
     thread_count = migrate_compress_threads();
     compress_threads = g_new0(QemuThread, thread_count);
     comp_param = g_new0(CompressParam, thread_count);
@@ -1092,7 +1090,6 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss,
                  * point. In theory, xbzrle can do better than compression.
                  */
                 flush_compressed_data(rs);
-                compression_switch = false;
             }
         }
         /* Didn't find anything this time, but try again on the new block */
@@ -1326,7 +1323,14 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms,
     /* Check the pages is dirty and if it is send it */
     if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) {
         unsigned long *unsentmap;
-        if (compression_switch && migrate_use_compression()) {
+        /*
+         * If xbzrle is on, stop using the data compression after first
+         * round of migration even if compression is enabled. In theory,
+         * xbzrle can do better than compression.
+         */
+
+        if (migrate_use_compression())
+            && (rs->ram_bulk_stage || !migrate_use_xbzrle()) {
             res = ram_save_compressed_page(rs, ms, pss, last_stage);
         } else {
             res = ram_save_page(rs, ms, pss, last_stage);
-- 
2.9.3


Re: [Qemu-devel] [PATCH 40/54] ram: Remove compression_switch and inline its logic
Posted by Dr. David Alan Gilbert 8 years, 10 months ago
* Juan Quintela (quintela@redhat.com) wrote:
> We can calculate its value, so not create a varible for it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

> --
> 
> After Peter and Dave review, I dropped the variable and just inlined
> the condition.
> 
> Later, Juan.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 14d54fd..5dfc9bd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -336,7 +336,6 @@ static QemuCond comp_done_cond;
>  /* The empty QEMUFileOps will be used by file in CompressParam */
>  static const QEMUFileOps empty_ops = { };
>  
> -static bool compression_switch;
>  static DecompressParam *decomp_param;
>  static QemuThread *decompress_threads;
>  static QemuMutex decomp_done_lock;
> @@ -420,7 +419,6 @@ void migrate_compress_threads_create(void)
>      if (!migrate_use_compression()) {
>          return;
>      }
> -    compression_switch = true;
>      thread_count = migrate_compress_threads();
>      compress_threads = g_new0(QemuThread, thread_count);
>      comp_param = g_new0(CompressParam, thread_count);
> @@ -1092,7 +1090,6 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss,
>                   * point. In theory, xbzrle can do better than compression.
>                   */
>                  flush_compressed_data(rs);
> -                compression_switch = false;
>              }
>          }
>          /* Didn't find anything this time, but try again on the new block */
> @@ -1326,7 +1323,14 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms,
>      /* Check the pages is dirty and if it is send it */
>      if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) {
>          unsigned long *unsentmap;
> -        if (compression_switch && migrate_use_compression()) {
> +        /*
> +         * If xbzrle is on, stop using the data compression after first
> +         * round of migration even if compression is enabled. In theory,
> +         * xbzrle can do better than compression.
> +         */
> +
> +        if (migrate_use_compression())
> +            && (rs->ram_bulk_stage || !migrate_use_xbzrle()) {
>              res = ram_save_compressed_page(rs, ms, pss, last_stage);
>          } else {
>              res = ram_save_page(rs, ms, pss, last_stage);
> -- 
> 2.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 40/54] ram: Remove compression_switch and inline its logic
Posted by Peter Xu 8 years, 10 months ago
On Thu, Apr 06, 2017 at 03:08:59PM +0200, Juan Quintela wrote:
> We can calculate its value, so not create a varible for it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> --
> 
> After Peter and Dave review, I dropped the variable and just inlined
> the condition.
> 
> Later, Juan.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 14d54fd..5dfc9bd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -336,7 +336,6 @@ static QemuCond comp_done_cond;
>  /* The empty QEMUFileOps will be used by file in CompressParam */
>  static const QEMUFileOps empty_ops = { };
>  
> -static bool compression_switch;
>  static DecompressParam *decomp_param;
>  static QemuThread *decompress_threads;
>  static QemuMutex decomp_done_lock;
> @@ -420,7 +419,6 @@ void migrate_compress_threads_create(void)
>      if (!migrate_use_compression()) {
>          return;
>      }
> -    compression_switch = true;
>      thread_count = migrate_compress_threads();
>      compress_threads = g_new0(QemuThread, thread_count);
>      comp_param = g_new0(CompressParam, thread_count);
> @@ -1092,7 +1090,6 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss,
>                   * point. In theory, xbzrle can do better than compression.
>                   */
>                  flush_compressed_data(rs);
> -                compression_switch = false;
>              }
>          }
>          /* Didn't find anything this time, but try again on the new block */
> @@ -1326,7 +1323,14 @@ static int ram_save_target_page(RAMState *rs, MigrationState *ms,
>      /* Check the pages is dirty and if it is send it */
>      if (migration_bitmap_clear_dirty(rs, dirty_ram_abs)) {
>          unsigned long *unsentmap;
> -        if (compression_switch && migrate_use_compression()) {
> +        /*
> +         * If xbzrle is on, stop using the data compression after first
> +         * round of migration even if compression is enabled. In theory,
> +         * xbzrle can do better than compression.
> +         */
> +
> +        if (migrate_use_compression())
                                        ^ is this a superfluous ")"?

> +            && (rs->ram_bulk_stage || !migrate_use_xbzrle()) {
>              res = ram_save_compressed_page(rs, ms, pss, last_stage);
>          } else {
>              res = ram_save_page(rs, ms, pss, last_stage);
> -- 
> 2.9.3

Thanks,

-- peterx