[Qemu-devel] [PATCH 0/2] Check migration enabled features on both sides

[Qemu-devel] [PATCH 0/2] Check migration enabled features on both sides

[Juan Quintela]

Hi

This was part of my multifd series, it just "requires" that
migartion features being enabled on both sides.
- XBZRLE: libvirt only enables it on source, so we don't test it

- compression threads: we check at reception when we receive a
                       compression page that we have compression
                       threads enabled.  Otherwise we exit with one
                       error.

- Now that we check if compression threads are enabled, we don't
  create the threads if they are not enabled.

  It makes debugging much easier, because I have less threads on
  destination to search where things are hang up.

Please review,

Thanks, Juan.

Juan Quintela (2):
  migration: Test for disabled features on reception
  migration: Don't create decompression threads if not enabled

 migration/ram.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Juan Quintela]

Right now, if we receive a compressed page while this features are
disabled, Bad Things (TM) can happen.  Just add a test for them.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

--

I had XBZRLE here also, but it don't need extra resources on
destination, only on source.  Additionally libvirt don't enable it on
destination, so don't put it here.
---
 migration/ram.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index f35d65a..f2d1bce 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
 
 static int ram_load(QEMUFile *f, void *opaque, int version_id)
 {
-    int flags = 0, ret = 0;
+    int flags = 0, ret = 0, invalid_flags;
     static uint64_t seq_iter;
     int len = 0;
     /*
@@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         ret = -EINVAL;
     }
 
+    invalid_flags = 0;
+
+    if (!migrate_use_compression()) {
+        invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
+    }
     /* This RCU critical section can be very long running.
      * When RCU reclaims in the code start to become numerous,
      * it will be necessary to reduce the granularity of this
@@ -2514,6 +2519,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         flags = addr & ~TARGET_PAGE_MASK;
         addr &= TARGET_PAGE_MASK;
 
+        if (flags & invalid_flags) {
+            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
+                error_report("Received an unexpected compressed page");
+            }
+
+            ret = -EINVAL;
+            break;
+        }
+
         if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
                      RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
             RAMBlock *block = ram_block_from_stream(f, flags);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Right now, if we receive a compressed page while this features are
> disabled, Bad Things (TM) can happen.  Just add a test for them.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Yes, my R-b stands, there could be someone somewhere driving this by
hand who might get caught out, but seems saner this way.

> --
> 
> I had XBZRLE here also, but it don't need extra resources on
> destination, only on source.  Additionally libvirt don't enable it on
> destination, so don't put it here.
> ---
>  migration/ram.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f35d65a..f2d1bce 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
>  
>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
> -    int flags = 0, ret = 0;
> +    int flags = 0, ret = 0, invalid_flags;
>      static uint64_t seq_iter;
>      int len = 0;
>      /*
> @@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          ret = -EINVAL;
>      }
>  
> +    invalid_flags = 0;

why didn't you use = 0 in the declaration like the rest of the flags?
(Only minor)

Dave

> +
> +    if (!migrate_use_compression()) {
> +        invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
> +    }
>      /* This RCU critical section can be very long running.
>       * When RCU reclaims in the code start to become numerous,
>       * it will be necessary to reduce the granularity of this
> @@ -2514,6 +2519,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          flags = addr & ~TARGET_PAGE_MASK;
>          addr &= TARGET_PAGE_MASK;
>  
> +        if (flags & invalid_flags) {
> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
> +                error_report("Received an unexpected compressed page");
> +            }
> +
> +            ret = -EINVAL;
> +            break;
> +        }
> +
>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>              RAMBlock *block = ram_block_from_stream(f, flags);
> -- 
> 2.9.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Right now, if we receive a compressed page while this features are
>> disabled, Bad Things (TM) can happen.  Just add a test for them.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Yes, my R-b stands, there could be someone somewhere driving this by
> hand who might get caught out, but seems saner this way.
>
>> --
>> 
>> I had XBZRLE here also, but it don't need extra resources on
>> destination, only on source.  Additionally libvirt don't enable it on
>> destination, so don't put it here.
>> ---
>>  migration/ram.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index f35d65a..f2d1bce 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>  
>>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>  {
>> -    int flags = 0, ret = 0;
>> +    int flags = 0, ret = 0, invalid_flags;
>>      static uint64_t seq_iter;
>>      int len = 0;
>>      /*
>> @@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          ret = -EINVAL;
>>      }
>>  
>> +    invalid_flags = 0;
>
> why didn't you use = 0 in the declaration like the rest of the flags?
> (Only minor)

Costume :-(

Changed to common style there.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Peter Xu]

On Tue, Jun 13, 2017 at 11:54:31AM +0200, Juan Quintela wrote:
> Right now, if we receive a compressed page while this features are
> disabled, Bad Things (TM) can happen.  Just add a test for them.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> --
> 
> I had XBZRLE here also, but it don't need extra resources on
> destination, only on source.  Additionally libvirt don't enable it on
> destination, so don't put it here.
> ---
>  migration/ram.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f35d65a..f2d1bce 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
>  
>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
> -    int flags = 0, ret = 0;
> +    int flags = 0, ret = 0, invalid_flags;
>      static uint64_t seq_iter;
>      int len = 0;
>      /*
> @@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          ret = -EINVAL;
>      }
>  
> +    invalid_flags = 0;

Nit: set zero when declare (just like flags, ret)?

> +
> +    if (!migrate_use_compression()) {
> +        invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
> +    }
>      /* This RCU critical section can be very long running.
>       * When RCU reclaims in the code start to become numerous,
>       * it will be necessary to reduce the granularity of this
> @@ -2514,6 +2519,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          flags = addr & ~TARGET_PAGE_MASK;
>          addr &= TARGET_PAGE_MASK;
>  
> +        if (flags & invalid_flags) {
> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
                                        ^
Nit: one extra space -------------------+

> +                error_report("Received an unexpected compressed page");
> +            }
> +
> +            ret = -EINVAL;
> +            break;
> +        }
> +
>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>              RAMBlock *block = ram_block_from_stream(f, flags);
> -- 
> 2.9.4
> 

Besides the nits:

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

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Jun 13, 2017 at 11:54:31AM +0200, Juan Quintela wrote:
>> Right now, if we receive a compressed page while this features are
>> disabled, Bad Things (TM) can happen.  Just add a test for them.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> --
>> 
>> I had XBZRLE here also, but it don't need extra resources on
>> destination, only on source.  Additionally libvirt don't enable it on
>> destination, so don't put it here.
>> ---
>>  migration/ram.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index f35d65a..f2d1bce 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>  
>>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>  {
>> -    int flags = 0, ret = 0;
>> +    int flags = 0, ret = 0, invalid_flags;
>>      static uint64_t seq_iter;
>>      int len = 0;
>>      /*
>> @@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          ret = -EINVAL;
>>      }
>>  
>> +    invalid_flags = 0;
>
> Nit: set zero when declare (just like flags, ret)?

Fixed.

As answered to dave, I use to do it that way.
But here it is more consistant to do it the other way.

>> +
>> +    if (!migrate_use_compression()) {
>> +        invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>> +    }
>>      /* This RCU critical section can be very long running.
>>       * When RCU reclaims in the code start to become numerous,
>>       * it will be necessary to reduce the granularity of this
>> @@ -2514,6 +2519,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          flags = addr & ~TARGET_PAGE_MASK;
>>          addr &= TARGET_PAGE_MASK;
>>  
>> +        if (flags & invalid_flags) {
>> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
>                                         ^
> Nit: one extra space -------------------+

Fixed.

>> +                error_report("Received an unexpected compressed page");
>> +            }
>> +
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>>              RAMBlock *block = ram_block_from_stream(f, flags);
>> -- 
>> 2.9.4
>> 
>
> Besides the nits:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks.

[Qemu-devel] [PATCH 2/2] migration: Don't create decompression threads if not enabled

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

--

I removed the [HACK] part because previous patch just check that
compression pages are not received.
---
 migration/ram.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index f2d1bce..d475cf5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2234,6 +2234,9 @@ static void compress_threads_load_setup(void)
 {
     int i, thread_count;
 
+    if (!migrate_use_compression()) {
+        return;
+    }
     thread_count = migrate_decompress_threads();
     decompress_threads = g_new0(QemuThread, thread_count);
     decomp_param = g_new0(DecompressParam, thread_count);
@@ -2255,6 +2258,9 @@ static void compress_threads_load_cleanup(void)
 {
     int i, thread_count;
 
+    if (!migrate_use_compression()) {
+        return;
+    }
     thread_count = migrate_decompress_threads();
     for (i = 0; i < thread_count; i++) {
         qemu_mutex_lock(&decomp_param[i].mutex);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 2/2] migration: Don't create decompression threads if not enabled

[Peter Xu]

On Tue, Jun 13, 2017 at 11:54:32AM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> --
> 
> I removed the [HACK] part because previous patch just check that
> compression pages are not received.
> ---
>  migration/ram.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f2d1bce..d475cf5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2234,6 +2234,9 @@ static void compress_threads_load_setup(void)

I cannot find compress_threads_load_setup() in my repo, but
migrate_decompress_threads_create()?

Thanks,

>  {
>      int i, thread_count;
>  
> +    if (!migrate_use_compression()) {
> +        return;
> +    }
>      thread_count = migrate_decompress_threads();
>      decompress_threads = g_new0(QemuThread, thread_count);
>      decomp_param = g_new0(DecompressParam, thread_count);
> @@ -2255,6 +2258,9 @@ static void compress_threads_load_cleanup(void)
>  {
>      int i, thread_count;
>  
> +    if (!migrate_use_compression()) {
> +        return;
> +    }
>      thread_count = migrate_decompress_threads();
>      for (i = 0; i < thread_count; i++) {
>          qemu_mutex_lock(&decomp_param[i].mutex);
> -- 
> 2.9.4
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] migration: Don't create decompression threads if not enabled

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Jun 13, 2017 at 11:54:32AM +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> --
>> 
>> I removed the [HACK] part because previous patch just check that
>> compression pages are not received.
>> ---
>>  migration/ram.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index f2d1bce..d475cf5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2234,6 +2234,9 @@ static void compress_threads_load_setup(void)
>
> I cannot find compress_threads_load_setup() in my repo, but
> migrate_decompress_threads_create()?


Sorry, this patches are on top of load_setup/cleanup series.

Later, Juan.

>
> Thanks,
>
>>  {
>>      int i, thread_count;
>>  
>> +    if (!migrate_use_compression()) {
>> +        return;
>> +    }
>>      thread_count = migrate_decompress_threads();
>>      decompress_threads = g_new0(QemuThread, thread_count);
>>      decomp_param = g_new0(DecompressParam, thread_count);
>> @@ -2255,6 +2258,9 @@ static void compress_threads_load_cleanup(void)
>>  {
>>      int i, thread_count;
>>  
>> +    if (!migrate_use_compression()) {
>> +        return;
>> +    }
>>      thread_count = migrate_decompress_threads();
>>      for (i = 0; i < thread_count; i++) {
>>          qemu_mutex_lock(&decomp_param[i].mutex);
>> -- 
>> 2.9.4
>> 

Re: [Qemu-devel] [PATCH 2/2] migration: Don't create decompression threads if not enabled

[Peter Xu]

On Wed, Jun 14, 2017 at 09:46:50AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Tue, Jun 13, 2017 at 11:54:32AM +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> 
> >> --
> >> 
> >> I removed the [HACK] part because previous patch just check that
> >> compression pages are not received.
> >> ---
> >>  migration/ram.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index f2d1bce..d475cf5 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -2234,6 +2234,9 @@ static void compress_threads_load_setup(void)
> >
> > I cannot find compress_threads_load_setup() in my repo, but
> > migrate_decompress_threads_create()?
> 
> 
> Sorry, this patches are on top of load_setup/cleanup series.

Got it. After all the content of the patch looks good to me, so please
take my r-b as long as the patch can apply cleanly and correctly on
where it should be based:

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

Thanks,

-- 
Peter Xu