All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] Migration fixes
@ 2016-11-14 20:55 Juan Quintela
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time Juan Quintela
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Juan Quintela @ 2016-11-14 20:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, dgilbert


Hi

This are the fixes that were of the multifd patches.

The most important one is the second patch, that one that checks for valid flags on reception.

Please, review.

Juan Quintela (3):
  migration: create Migration Incoming State at init time
  migration: Test for disabled features on reception
  migration: Don't create decompression threads if not enabled

 include/migration/migration.h |  1 -
 migration/migration.c         | 38 +++++++++++++++++---------------------
 migration/ram.c               | 29 ++++++++++++++++++++++++++++-
 migration/savevm.c            |  4 ++--
 4 files changed, 47 insertions(+), 25 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time
  2016-11-14 20:55 [Qemu-devel] [PATCH 0/3] Migration fixes Juan Quintela
@ 2016-11-14 20:55 ` Juan Quintela
  2016-11-15  9:50   ` Dr. David Alan Gilbert
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception Juan Quintela
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 3/3] migration: Don't create decompression threads if not enabled Juan Quintela
  2 siblings, 1 reply; 8+ messages in thread
From: Juan Quintela @ 2016-11-14 20:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, dgilbert

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  1 -
 migration/migration.c         | 38 +++++++++++++++++---------------------
 migration/savevm.c            |  4 ++--
 3 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index c309d23..a184509 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -119,7 +119,6 @@ struct MigrationIncomingState {
 };

 MigrationIncomingState *migration_incoming_get_current(void);
-MigrationIncomingState *migration_incoming_state_new(QEMUFile *f);
 void migration_incoming_state_destroy(void);

 /*
diff --git a/migration/migration.c b/migration/migration.c
index e331f28..51ca9b4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -111,32 +111,28 @@ MigrationState *migrate_get_current(void)
     return &current_migration;
 }

-/* For incoming */
-static MigrationIncomingState *mis_current;
-
 MigrationIncomingState *migration_incoming_get_current(void)
 {
-    return mis_current;
-}
+    static bool once;
+    static MigrationIncomingState mis_current;

-MigrationIncomingState *migration_incoming_state_new(QEMUFile* f)
-{
-    mis_current = g_new0(MigrationIncomingState, 1);
-    mis_current->from_src_file = f;
-    mis_current->state = MIGRATION_STATUS_NONE;
-    QLIST_INIT(&mis_current->loadvm_handlers);
-    qemu_mutex_init(&mis_current->rp_mutex);
-    qemu_event_init(&mis_current->main_thread_load_event, false);
-
-    return mis_current;
+    if (!once) {
+        mis_current.state = MIGRATION_STATUS_NONE;
+        memset(&mis_current, 0, sizeof(MigrationIncomingState));
+        QLIST_INIT(&mis_current.loadvm_handlers);
+        qemu_mutex_init(&mis_current.rp_mutex);
+        qemu_event_init(&mis_current.main_thread_load_event, false);
+        once = true;
+    }
+    return &mis_current;
 }

 void migration_incoming_state_destroy(void)
 {
-    qemu_event_destroy(&mis_current->main_thread_load_event);
-    loadvm_free_handlers(mis_current);
-    g_free(mis_current);
-    mis_current = NULL;
+    struct MigrationIncomingState *mis = migration_incoming_get_current();
+
+    qemu_event_destroy(&mis->main_thread_load_event);
+    loadvm_free_handlers(mis);
 }


@@ -382,11 +378,11 @@ static void process_incoming_migration_bh(void *opaque)
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
-    MigrationIncomingState *mis;
+    MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
     int ret;

-    mis = migration_incoming_state_new(f);
+    mis->from_src_file = f;
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_ACTIVE);
diff --git a/migration/savevm.c b/migration/savevm.c
index 0363372..d44a38c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2159,7 +2159,6 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
     f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));

-    migration_incoming_state_new(f);
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
@@ -2175,6 +2174,7 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;
     AioContext *aio_context;
+    MigrationIncomingState *mis = migration_incoming_get_current();

     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
@@ -2225,7 +2225,7 @@ int load_vmstate(const char *name)
     }

     qemu_system_reset(VMRESET_SILENT);
-    migration_incoming_state_new(f);
+    mis->from_src_file = f;

     aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception
  2016-11-14 20:55 [Qemu-devel] [PATCH 0/3] Migration fixes Juan Quintela
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time Juan Quintela
@ 2016-11-14 20:55 ` Juan Quintela
  2016-11-15  9:54   ` Dr. David Alan Gilbert
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 3/3] migration: Don't create decompression threads if not enabled Juan Quintela
  2 siblings, 1 reply; 8+ messages in thread
From: Juan Quintela @ 2016-11-14 20:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, dgilbert

Right now, if we receive a compressed page or a xbzrle 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>
---
 migration/ram.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index fb9252d..4bb707c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2464,7 +2464,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;
     /*
@@ -2479,6 +2479,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         ret = -EINVAL;
     }

+    invalid_flags = 0;
+
+    if (!migrate_use_xbzrle()) {
+        invalid_flags |= RAM_SAVE_FLAG_XBZRLE;
+    }
+
+    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
@@ -2499,6 +2508,18 @@ 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_XBZRLE) {
+                error_report("Received an unexpected XBRLE page");
+            }
+            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
+                error_report("Received an unexpected compressed page");
+            }
+
+            ret = -EINVAL;
+            break;
+        }
+
         if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
                      RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
             RAMBlock *block = ram_block_from_stream(f, flags);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 3/3] migration: Don't create decompression threads if not enabled
  2016-11-14 20:55 [Qemu-devel] [PATCH 0/3] Migration fixes Juan Quintela
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time Juan Quintela
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception Juan Quintela
@ 2016-11-14 20:55 ` Juan Quintela
  2 siblings, 0 replies; 8+ messages in thread
From: Juan Quintela @ 2016-11-14 20:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, dgilbert

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

--

I removed the [HACK] part because previous patch just check that
compression pages are not received.

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

diff --git a/migration/ram.c b/migration/ram.c
index 4bb707c..24e2591 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2260,6 +2260,9 @@ void migrate_decompress_threads_create(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);
@@ -2281,6 +2284,9 @@ void migrate_decompress_threads_join(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.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time Juan Quintela
@ 2016-11-15  9:50   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2016-11-15  9:50 UTC (permalink / raw)
  To: Juan Quintela; +Cc: qemu-devel, amit.shah

* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  1 -
>  migration/migration.c         | 38 +++++++++++++++++---------------------
>  migration/savevm.c            |  4 ++--
>  3 files changed, 19 insertions(+), 24 deletions(-)

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

> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index c309d23..a184509 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -119,7 +119,6 @@ struct MigrationIncomingState {
>  };
> 
>  MigrationIncomingState *migration_incoming_get_current(void);
> -MigrationIncomingState *migration_incoming_state_new(QEMUFile *f);
>  void migration_incoming_state_destroy(void);
> 
>  /*
> diff --git a/migration/migration.c b/migration/migration.c
> index e331f28..51ca9b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -111,32 +111,28 @@ MigrationState *migrate_get_current(void)
>      return &current_migration;
>  }
> 
> -/* For incoming */
> -static MigrationIncomingState *mis_current;
> -
>  MigrationIncomingState *migration_incoming_get_current(void)
>  {
> -    return mis_current;
> -}
> +    static bool once;
> +    static MigrationIncomingState mis_current;
> 
> -MigrationIncomingState *migration_incoming_state_new(QEMUFile* f)
> -{
> -    mis_current = g_new0(MigrationIncomingState, 1);
> -    mis_current->from_src_file = f;
> -    mis_current->state = MIGRATION_STATUS_NONE;
> -    QLIST_INIT(&mis_current->loadvm_handlers);
> -    qemu_mutex_init(&mis_current->rp_mutex);
> -    qemu_event_init(&mis_current->main_thread_load_event, false);
> -
> -    return mis_current;
> +    if (!once) {
> +        mis_current.state = MIGRATION_STATUS_NONE;
> +        memset(&mis_current, 0, sizeof(MigrationIncomingState));
> +        QLIST_INIT(&mis_current.loadvm_handlers);
> +        qemu_mutex_init(&mis_current.rp_mutex);
> +        qemu_event_init(&mis_current.main_thread_load_event, false);
> +        once = true;
> +    }
> +    return &mis_current;
>  }
> 
>  void migration_incoming_state_destroy(void)
>  {
> -    qemu_event_destroy(&mis_current->main_thread_load_event);
> -    loadvm_free_handlers(mis_current);
> -    g_free(mis_current);
> -    mis_current = NULL;
> +    struct MigrationIncomingState *mis = migration_incoming_get_current();
> +
> +    qemu_event_destroy(&mis->main_thread_load_event);
> +    loadvm_free_handlers(mis);
>  }
> 
> 
> @@ -382,11 +378,11 @@ static void process_incoming_migration_bh(void *opaque)
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> -    MigrationIncomingState *mis;
> +    MigrationIncomingState *mis = migration_incoming_get_current();
>      PostcopyState ps;
>      int ret;
> 
> -    mis = migration_incoming_state_new(f);
> +    mis->from_src_file = f;
>      postcopy_state_set(POSTCOPY_INCOMING_NONE);
>      migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
>                        MIGRATION_STATUS_ACTIVE);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0363372..d44a38c 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2159,7 +2159,6 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
>      f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
> 
> -    migration_incoming_state_new(f);
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> @@ -2175,6 +2174,7 @@ int load_vmstate(const char *name)
>      QEMUFile *f;
>      int ret;
>      AioContext *aio_context;
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> 
>      if (!bdrv_all_can_snapshot(&bs)) {
>          error_report("Device '%s' is writable but does not support snapshots.",
> @@ -2225,7 +2225,7 @@ int load_vmstate(const char *name)
>      }
> 
>      qemu_system_reset(VMRESET_SILENT);
> -    migration_incoming_state_new(f);
> +    mis->from_src_file = f;
> 
>      aio_context_acquire(aio_context);
>      ret = qemu_loadvm_state(f);
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception
  2016-11-14 20:55 ` [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception Juan Quintela
@ 2016-11-15  9:54   ` Dr. David Alan Gilbert
  2016-11-15 10:07     ` Juan Quintela
  0 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2016-11-15  9:54 UTC (permalink / raw)
  To: Juan Quintela; +Cc: qemu-devel, amit.shah

* Juan Quintela (quintela@redhat.com) wrote:
> Right now, if we receive a compressed page or a xbzrle page while this
> features are disabled, Bad Things (TM) can happen.  Just add a test for
> them.

This confuses me; I didn't think until recently we could
guarantee anything about having the capabilities set on the destination
side.  Until -incoming defer the destination didn't have a way of setting
capabilities in a known state before starting the reception.

Dave

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index fb9252d..4bb707c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2464,7 +2464,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;
>      /*
> @@ -2479,6 +2479,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          ret = -EINVAL;
>      }
> 
> +    invalid_flags = 0;
> +
> +    if (!migrate_use_xbzrle()) {
> +        invalid_flags |= RAM_SAVE_FLAG_XBZRLE;
> +    }
> +
> +    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
> @@ -2499,6 +2508,18 @@ 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_XBZRLE) {
> +                error_report("Received an unexpected XBRLE page");
> +            }
> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
> +                error_report("Received an unexpected compressed page");
> +            }
> +
> +            ret = -EINVAL;
> +            break;
> +        }
> +
>          if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>              RAMBlock *block = ram_block_from_stream(f, flags);
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception
  2016-11-15  9:54   ` Dr. David Alan Gilbert
@ 2016-11-15 10:07     ` Juan Quintela
  2016-11-15 10:42       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 8+ messages in thread
From: Juan Quintela @ 2016-11-15 10:07 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, amit.shah

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Right now, if we receive a compressed page or a xbzrle page while this
>> features are disabled, Bad Things (TM) can happen.  Just add a test for
>> them.
>
> This confuses me; I didn't think until recently we could
> guarantee anything about having the capabilities set on the destination
> side.  Until -incoming defer the destination didn't have a way of setting
> capabilities in a known state before starting the reception.

Ouch.

So, here we are after further investigation.

- xbzrle: we don't need anything special on the reception side, we just
          decode inplace.  So we are good.  If we don't use xbzrle, we
          don't waste any resources either.
- compression: We create the decompression threads always, with all its
          associated configuration.


Why do I wanted to do this?  For multifd, I don't really want to create
multiple fd's except if configured, because I wait for the fd's to be
"accepted" before continue, so I need a way to know if it is configured
or no.  So I need something like this.

I started doing this for compression because when I was debugging I had
too many threads waiting on qemu_cond_wait(), and I only wanted to look
at the multifd ones.

I had not througth that we didn't need to set capabilities on
destination.

Suggestions?

Later, Juan.

>
> Dave
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/ram.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index fb9252d..4bb707c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2464,7 +2464,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;
>>      /*
>> @@ -2479,6 +2479,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          ret = -EINVAL;
>>      }
>> 
>> +    invalid_flags = 0;
>> +
>> +    if (!migrate_use_xbzrle()) {
>> +        invalid_flags |= RAM_SAVE_FLAG_XBZRLE;
>> +    }
>> +
>> +    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
>> @@ -2499,6 +2508,18 @@ 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_XBZRLE) {
>> +                error_report("Received an unexpected XBRLE page");
>> +            }
>> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
>> +                error_report("Received an unexpected compressed page");
>> +            }
>> +
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>>          if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
>>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>>              RAMBlock *block = ram_block_from_stream(f, flags);
>> -- 
>> 2.7.4
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception
  2016-11-15 10:07     ` Juan Quintela
@ 2016-11-15 10:42       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2016-11-15 10:42 UTC (permalink / raw)
  To: Juan Quintela; +Cc: qemu-devel, amit.shah

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> Right now, if we receive a compressed page or a xbzrle page while this
> >> features are disabled, Bad Things (TM) can happen.  Just add a test for
> >> them.
> >
> > This confuses me; I didn't think until recently we could
> > guarantee anything about having the capabilities set on the destination
> > side.  Until -incoming defer the destination didn't have a way of setting
> > capabilities in a known state before starting the reception.
> 
> Ouch.
> 
> So, here we are after further investigation.
> 
> - xbzrle: we don't need anything special on the reception side, we just
>           decode inplace.  So we are good.  If we don't use xbzrle, we
>           don't waste any resources either.
> - compression: We create the decompression threads always, with all its
>           associated configuration.
> 
> 
> Why do I wanted to do this?  For multifd, I don't really want to create
> multiple fd's except if configured, because I wait for the fd's to be
> "accepted" before continue, so I need a way to know if it is configured
> or no.  So I need something like this.
> 
> I started doing this for compression because when I was debugging I had
> too many threads waiting on qemu_cond_wait(), and I only wanted to look
> at the multifd ones.
> 
> I had not througth that we didn't need to set capabilities on
> destination.
> 
> Suggestions?

It's fine for new features to require it; newer libvirt already does
it for postcopy and does:
    -incoming defer
    sets the capabilities and parameters
    migrate_incoming URI

However, enforcing it for old features sounds something we can't do.

Dave

> Later, Juan.
> 
> >
> > Dave
> >
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/ram.c | 23 ++++++++++++++++++++++-
> >>  1 file changed, 22 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index fb9252d..4bb707c 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -2464,7 +2464,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;
> >>      /*
> >> @@ -2479,6 +2479,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >>          ret = -EINVAL;
> >>      }
> >> 
> >> +    invalid_flags = 0;
> >> +
> >> +    if (!migrate_use_xbzrle()) {
> >> +        invalid_flags |= RAM_SAVE_FLAG_XBZRLE;
> >> +    }
> >> +
> >> +    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
> >> @@ -2499,6 +2508,18 @@ 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_XBZRLE) {
> >> +                error_report("Received an unexpected XBRLE page");
> >> +            }
> >> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
> >> +                error_report("Received an unexpected compressed page");
> >> +            }
> >> +
> >> +            ret = -EINVAL;
> >> +            break;
> >> +        }
> >> +
> >>          if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
> >>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> >>              RAMBlock *block = ram_block_from_stream(f, flags);
> >> -- 
> >> 2.7.4
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-11-15 10:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-14 20:55 [Qemu-devel] [PATCH 0/3] Migration fixes Juan Quintela
2016-11-14 20:55 ` [Qemu-devel] [PATCH 1/3] migration: create Migration Incoming State at init time Juan Quintela
2016-11-15  9:50   ` Dr. David Alan Gilbert
2016-11-14 20:55 ` [Qemu-devel] [PATCH 2/3] migration: Test for disabled features on reception Juan Quintela
2016-11-15  9:54   ` Dr. David Alan Gilbert
2016-11-15 10:07     ` Juan Quintela
2016-11-15 10:42       ` Dr. David Alan Gilbert
2016-11-14 20:55 ` [Qemu-devel] [PATCH 3/3] migration: Don't create decompression threads if not enabled Juan Quintela

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.