[Qemu-devel] [RFC PATCH] migration/postcopy: skip compression when postcopy is active

[Qemu-devel] [RFC PATCH] migration/postcopy: skip compression when postcopy is active

[Wei Yang]

Now postcopy is not compatible with compression. And we disable setting
these two capability at the same time. While we can still leverage
compress before postcopy is active, for example at the bulk stage.

This patch skips compression when postcopy is active instead of
forbidding setting these capability at the same time.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/migration.c | 11 -----------
 migration/ram.c       | 10 ++++++++++
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5a496addbd..33c373033d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -995,17 +995,6 @@ static bool migrate_caps_check(bool *cap_list,
 #endif
 
     if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
-        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
-            /* The decompression threads asynchronously write into RAM
-             * rather than use the atomic copies needed to avoid
-             * userfaulting.  It should be possible to fix the decompression
-             * threads for compatibility in future.
-             */
-            error_setg(errp, "Postcopy is not currently compatible "
-                       "with compression");
-            return false;
-        }
-
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
diff --git a/migration/ram.c b/migration/ram.c
index da12774a24..a0d3bc60b2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2384,6 +2384,16 @@ static bool save_page_use_compression(RAMState *rs)
         return false;
     }
 
+    /*
+     * The decompression threads asynchronously write into RAM
+     * rather than use the atomic copies needed to avoid
+     * userfaulting.  It should be possible to fix the decompression
+     * threads for compatibility in future.
+     */
+    if (migration_in_postcopy()) {
+        return false;
+    }
+
     /*
      * If xbzrle is on, stop using the data compression after first
      * round of migration even if compression is enabled. In theory,
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH] migration/postcopy: skip compression when postcopy is active

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Now postcopy is not compatible with compression. And we disable setting
> these two capability at the same time. While we can still leverage
> compress before postcopy is active, for example at the bulk stage.
> 
> This patch skips compression when postcopy is active instead of
> forbidding setting these capability at the same time.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

So I think this is OK, because ram_save_complete has a call to
flush_compressed_data in it.

However I think we should hold it until you figure out the other series
that really enables compress; the problem is that as soon as you
allow the capability, then there's nothing to distinguish this version
and the version that fully enables it.  So how would you stop this
version being used as the source to the version which fully enables it?

So I think if we do the other series, then you should start off like
this and then remove the capability check right at the end.

Dave

> ---
>  migration/migration.c | 11 -----------
>  migration/ram.c       | 10 ++++++++++
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5a496addbd..33c373033d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -995,17 +995,6 @@ static bool migrate_caps_check(bool *cap_list,
>  #endif
>  
>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> -        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
> -            /* The decompression threads asynchronously write into RAM
> -             * rather than use the atomic copies needed to avoid
> -             * userfaulting.  It should be possible to fix the decompression
> -             * threads for compatibility in future.
> -             */
> -            error_setg(errp, "Postcopy is not currently compatible "
> -                       "with compression");
> -            return false;
> -        }
> -
>          /* This check is reasonably expensive, so only when it's being
>           * set the first time, also it's only the destination that needs
>           * special support.
> diff --git a/migration/ram.c b/migration/ram.c
> index da12774a24..a0d3bc60b2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2384,6 +2384,16 @@ static bool save_page_use_compression(RAMState *rs)
>          return false;
>      }
>  
> +    /*
> +     * The decompression threads asynchronously write into RAM
> +     * rather than use the atomic copies needed to avoid
> +     * userfaulting.  It should be possible to fix the decompression
> +     * threads for compatibility in future.
> +     */
> +    if (migration_in_postcopy()) {
> +        return false;
> +    }
> +
>      /*
>       * If xbzrle is on, stop using the data compression after first
>       * round of migration even if compression is enabled. In theory,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH] migration/postcopy: skip compression when postcopy is active

[Wei Yang]

On Fri, Aug 23, 2019 at 05:27:01PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> Now postcopy is not compatible with compression. And we disable setting
>> these two capability at the same time. While we can still leverage
>> compress before postcopy is active, for example at the bulk stage.
>> 
>> This patch skips compression when postcopy is active instead of
>> forbidding setting these capability at the same time.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>So I think this is OK, because ram_save_complete has a call to
>flush_compressed_data in it.
>
>However I think we should hold it until you figure out the other series
>that really enables compress; the problem is that as soon as you
>allow the capability, then there's nothing to distinguish this version
>and the version that fully enables it.  So how would you stop this
>version being used as the source to the version which fully enables it?
>
>So I think if we do the other series, then you should start off like
>this and then remove the capability check right at the end.
>

Agree, it is not proper to leave this middle state.

>Dave
>
>> ---
>>  migration/migration.c | 11 -----------
>>  migration/ram.c       | 10 ++++++++++
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5a496addbd..33c373033d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -995,17 +995,6 @@ static bool migrate_caps_check(bool *cap_list,
>>  #endif
>>  
>>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>> -        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
>> -            /* The decompression threads asynchronously write into RAM
>> -             * rather than use the atomic copies needed to avoid
>> -             * userfaulting.  It should be possible to fix the decompression
>> -             * threads for compatibility in future.
>> -             */
>> -            error_setg(errp, "Postcopy is not currently compatible "
>> -                       "with compression");
>> -            return false;
>> -        }
>> -
>>          /* This check is reasonably expensive, so only when it's being
>>           * set the first time, also it's only the destination that needs
>>           * special support.
>> diff --git a/migration/ram.c b/migration/ram.c
>> index da12774a24..a0d3bc60b2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2384,6 +2384,16 @@ static bool save_page_use_compression(RAMState *rs)
>>          return false;
>>      }
>>  
>> +    /*
>> +     * The decompression threads asynchronously write into RAM
>> +     * rather than use the atomic copies needed to avoid
>> +     * userfaulting.  It should be possible to fix the decompression
>> +     * threads for compatibility in future.
>> +     */
>> +    if (migration_in_postcopy()) {
>> +        return false;
>> +    }
>> +
>>      /*
>>       * If xbzrle is on, stop using the data compression after first
>>       * round of migration even if compression is enabled. In theory,
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me