[Qemu-devel] [PULL 0/3] Migration pull

[Qemu-devel] [PULL 0/3] Migration pull

[Juan Quintela]

Hi Peter

This is a pull request with the small fixes from the list, please apply.

Later, Juan

The following changes since commit c49d3411faae8ffaab8f7e5db47405a008411c10:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12' into staging (2015-10-13 10:42:06 +0100)

are available in the git repository at:

  git://github.com/juanquintela/qemu.git tags/migration/20151015

for you to fetch changes up to 60be6340796e66b5ac8aac2d98dde5c79336a89c:

  migration: fix deadlock (2015-10-15 08:14:13 +0200)

----------------------------------------------------------------
migration/next for 20151015

----------------------------------------------------------------
Amit Shah (1):
      migration: announce VM's new home just before VM is runnable

Denis V. Lunev (1):
      migration: fix deadlock

Dr. David Alan Gilbert (1):
      Migration: Generate the completed event only when we complete

 migration/migration.c | 15 +++++++++++++--
 migration/ram.c       | 44 +++++++++++++++++++++++++++-----------------
 2 files changed, 40 insertions(+), 19 deletions(-)

[Qemu-devel] [PULL 1/3] Migration: Generate the completed event only when we complete

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The current migration-completed event is generated a bit too early,
which means that an eager libvirt that's ready to go as soon
as it sees the event ends up racing with the actual end of migration.

This corresponds to RH bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1271145

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Amit Shah <amit.shah@redhat.com>
xSigned-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index b7de9b7..3d40f24 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -294,12 +294,12 @@ static void process_incoming_migration_co(void *opaque)
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
     }
-    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
     qemu_announce_self();

     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
+        migrate_generate_event(MIGRATION_STATUS_FAILED);
         error_report_err(local_err);
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
@@ -320,6 +320,12 @@ static void process_incoming_migration_co(void *opaque)
         runstate_set(global_state_get_runstate());
     }
     migrate_decompress_threads_join();
+    /*
+     * This must happen after any state changes since as soon as an external
+     * observer sees this event they might start to prod at the VM assuming
+     * it's ready to use.
+     */
+    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
 }

 void process_incoming_migration(QEMUFile *f)
-- 
2.4.3

[Qemu-devel] [PULL 2/3] migration: announce VM's new home just before VM is runnable

[Juan Quintela]

From: Amit Shah <amit.shah@redhat.com>

We were announcing the dest host's IP as our new IP a bit too soon -- if
there were errors detected after this announcement was done, the
migration is failed and the VM could continue running on the src host --
causing problems later.

Move around the qemu_announce_self() call so it's done just before the
VM is runnable.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3d40f24..b092f38 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -294,7 +294,6 @@ static void process_incoming_migration_co(void *opaque)
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
     }
-    qemu_announce_self();

     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all(&local_err);
@@ -305,6 +304,12 @@ static void process_incoming_migration_co(void *opaque)
         exit(EXIT_FAILURE);
     }

+    /*
+     * This must happen after all error conditions are dealt with and
+     * we're sure the VM is going to be running on this host.
+     */
+    qemu_announce_self();
+
     /* If global state section was not received or we are in running
        state, we need to obey autostart. Any other state is set with
        runstate_set. */
-- 
2.4.3

[Qemu-devel] [PULL 3/3] migration: fix deadlock

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

Release qemu global mutex before call synchronize_rcu().
synchronize_rcu() waiting for all readers to finish their critical
sections. There is at least one critical section in which we try
to get QGM (critical section is in address_space_rw() and
prepare_mmio_access() is trying to aquire QGM).

Both functions (migration_end() and migration_bitmap_extend())
are called from main thread which is holding QGM.

Thus there is a race condition that ends up with deadlock:
main thread     working thread
Lock QGA                |
|             Call KVM_EXIT_IO handler
|                       |
|        Open rcu reader's critical section
Migration cleanup bh    |
|                       |
synchronize_rcu() is    |
waiting for readers     |
|            prepare_mmio_access() is waiting for QGM
  \                   /
         deadlock

The patch changes bitmap freeing from direct g_free after synchronize_rcu
to free inside call_rcu.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reported-by: Igor Redko <redkoi@virtuozzo.com>
Tested-by: Igor Redko <redkoi@virtuozzo.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>

CC: Anna Melekhova <annam@virtuozzo.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
---
 migration/ram.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 2d1d0b9..a25bcc7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -219,7 +219,6 @@ static RAMBlock *last_seen_block;
 /* This is the last block from where we have sent data */
 static RAMBlock *last_sent_block;
 static ram_addr_t last_offset;
-static unsigned long *migration_bitmap;
 static QemuMutex migration_bitmap_mutex;
 static uint64_t migration_dirty_pages;
 static uint32_t last_version;
@@ -236,6 +235,11 @@ struct PageSearchStatus {
 };
 typedef struct PageSearchStatus PageSearchStatus;

+static struct BitmapRcu {
+    struct rcu_head rcu;
+    unsigned long *bmap;
+} *migration_bitmap_rcu;
+
 struct CompressParam {
     bool start;
     bool done;
@@ -540,7 +544,7 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(RAMBlock *rb,

     unsigned long next;

-    bitmap = atomic_rcu_read(&migration_bitmap);
+    bitmap = atomic_rcu_read(&migration_bitmap_rcu)->bmap;
     if (ram_bulk_stage && nr > base) {
         next = nr + 1;
     } else {
@@ -558,7 +562,7 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(RAMBlock *rb,
 static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
 {
     unsigned long *bitmap;
-    bitmap = atomic_rcu_read(&migration_bitmap);
+    bitmap = atomic_rcu_read(&migration_bitmap_rcu)->bmap;
     migration_dirty_pages +=
         cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length);
 }
@@ -1090,17 +1094,22 @@ void free_xbzrle_decoded_buf(void)
     xbzrle_decoded_buf = NULL;
 }

+static void migration_bitmap_free(struct BitmapRcu *bmap)
+{
+    g_free(bmap->bmap);
+    g_free(bmap);
+}
+
 static void migration_end(void)
 {
     /* caller have hold iothread lock or is in a bh, so there is
      * no writing race against this migration_bitmap
      */
-    unsigned long *bitmap = migration_bitmap;
-    atomic_rcu_set(&migration_bitmap, NULL);
+    struct BitmapRcu *bitmap = migration_bitmap_rcu;
+    atomic_rcu_set(&migration_bitmap_rcu, NULL);
     if (bitmap) {
         memory_global_dirty_log_stop();
-        synchronize_rcu();
-        g_free(bitmap);
+        call_rcu(bitmap, migration_bitmap_free, rcu);
     }

     XBZRLE_cache_lock();
@@ -1136,9 +1145,10 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
     /* called in qemu main thread, so there is
      * no writing race against this migration_bitmap
      */
-    if (migration_bitmap) {
-        unsigned long *old_bitmap = migration_bitmap, *bitmap;
-        bitmap = bitmap_new(new);
+    if (migration_bitmap_rcu) {
+        struct BitmapRcu *old_bitmap = migration_bitmap_rcu, *bitmap;
+        bitmap = g_new(struct BitmapRcu, 1);
+        bitmap->bmap = bitmap_new(new);

         /* prevent migration_bitmap content from being set bit
          * by migration_bitmap_sync_range() at the same time.
@@ -1146,13 +1156,12 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
          * at the same time.
          */
         qemu_mutex_lock(&migration_bitmap_mutex);
-        bitmap_copy(bitmap, old_bitmap, old);
-        bitmap_set(bitmap, old, new - old);
-        atomic_rcu_set(&migration_bitmap, bitmap);
+        bitmap_copy(bitmap->bmap, old_bitmap->bmap, old);
+        bitmap_set(bitmap->bmap, old, new - old);
+        atomic_rcu_set(&migration_bitmap_rcu, bitmap);
         qemu_mutex_unlock(&migration_bitmap_mutex);
         migration_dirty_pages += new - old;
-        synchronize_rcu();
-        g_free(old_bitmap);
+        call_rcu(old_bitmap, migration_bitmap_free, rcu);
     }
 }

@@ -1210,8 +1219,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     reset_ram_globals();

     ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
-    migration_bitmap = bitmap_new(ram_bitmap_pages);
-    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
+    migration_bitmap_rcu = g_new(struct BitmapRcu, 1);
+    migration_bitmap_rcu->bmap = bitmap_new(ram_bitmap_pages);
+    bitmap_set(migration_bitmap_rcu->bmap, 0, ram_bitmap_pages);

     /*
      * Count the total number of pages used by ram blocks not including any
-- 
2.4.3

Re: [Qemu-devel] [PULL 1/3] Migration: Generate the completed event only when we complete

[Eric Blake]

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

On 10/15/2015 12:27 AM, Juan Quintela wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The current migration-completed event is generated a bit too early,
> which means that an eager libvirt that's ready to go as soon
> as it sees the event ends up racing with the actual end of migration.
> 
> This corresponds to RH bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1271145
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Amit Shah <amit.shah@redhat.com>
> xSigned-off-by: Juan Quintela <quintela@redhat.com>

Will the 'x' screw up tools looking at this line?

> ---
>  migration/migration.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PULL 1/3] Migration: Generate the completed event only when we complete

[Christian Borntraeger]

Am 15.10.2015 um 08:27 schrieb Juan Quintela:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The current migration-completed event is generated a bit too early,
> which means that an eager libvirt that's ready to go as soon
> as it sees the event ends up racing with the actual end of migration.
> 
> This corresponds to RH bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1271145

Is that something for stable?


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


> ---
>  migration/migration.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b7de9b7..3d40f24 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -294,12 +294,12 @@ static void process_incoming_migration_co(void *opaque)
>          migrate_decompress_threads_join();
>          exit(EXIT_FAILURE);
>      }
> -    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
>      qemu_announce_self();
> 
>      /* Make sure all file formats flush their mutable metadata */
>      bdrv_invalidate_cache_all(&local_err);
>      if (local_err) {
> +        migrate_generate_event(MIGRATION_STATUS_FAILED);
>          error_report_err(local_err);
>          migrate_decompress_threads_join();
>          exit(EXIT_FAILURE);
> @@ -320,6 +320,12 @@ static void process_incoming_migration_co(void *opaque)
>          runstate_set(global_state_get_runstate());
>      }
>      migrate_decompress_threads_join();
> +    /*
> +     * This must happen after any state changes since as soon as an external
> +     * observer sees this event they might start to prod at the VM assuming
> +     * it's ready to use.
> +     */
> +    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
>  }
> 
>  void process_incoming_migration(QEMUFile *f)
> 

Re: [Qemu-devel] [PULL 1/3] Migration: Generate the completed event only when we complete

[Dr. David Alan Gilbert]

* Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> Am 15.10.2015 um 08:27 schrieb Juan Quintela:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The current migration-completed event is generated a bit too early,
> > which means that an eager libvirt that's ready to go as soon
> > as it sees the event ends up racing with the actual end of migration.
> > 
> > This corresponds to RH bug:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1271145
> 
> Is that something for stable?

Hmm, yes, it probably should be.

cc'd in.

Dave

> 
> 
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Reviewed-by: Amit Shah <amit.shah@redhat.com>
> > xSigned-off-by: Juan Quintela <quintela@redhat.com>
> 
> 
> > ---
> >  migration/migration.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b7de9b7..3d40f24 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -294,12 +294,12 @@ static void process_incoming_migration_co(void *opaque)
> >          migrate_decompress_threads_join();
> >          exit(EXIT_FAILURE);
> >      }
> > -    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
> >      qemu_announce_self();
> > 
> >      /* Make sure all file formats flush their mutable metadata */
> >      bdrv_invalidate_cache_all(&local_err);
> >      if (local_err) {
> > +        migrate_generate_event(MIGRATION_STATUS_FAILED);
> >          error_report_err(local_err);
> >          migrate_decompress_threads_join();
> >          exit(EXIT_FAILURE);
> > @@ -320,6 +320,12 @@ static void process_incoming_migration_co(void *opaque)
> >          runstate_set(global_state_get_runstate());
> >      }
> >      migrate_decompress_threads_join();
> > +    /*
> > +     * This must happen after any state changes since as soon as an external
> > +     * observer sees this event they might start to prod at the VM assuming
> > +     * it's ready to use.
> > +     */
> > +    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
> >  }
> > 
> >  void process_incoming_migration(QEMUFile *f)
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 1/3] Migration: Generate the completed event only when we complete

[Juan Quintela]

Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Am 15.10.2015 um 08:27 schrieb Juan Quintela:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> 
>> The current migration-completed event is generated a bit too early,
>> which means that an eager libvirt that's ready to go as soon
>> as it sees the event ends up racing with the actual end of migration.
>> 
>> This corresponds to RH bug:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1271145
>
> Is that something for stable?

Yeap.


>
>
>> 
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Amit Shah <amit.shah@redhat.com>
>> xSigned-off-by: Juan Quintela <quintela@redhat.com>
>
>
>> ---
>>  migration/migration.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b7de9b7..3d40f24 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -294,12 +294,12 @@ static void process_incoming_migration_co(void *opaque)
>>          migrate_decompress_threads_join();
>>          exit(EXIT_FAILURE);
>>      }
>> -    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
>>      qemu_announce_self();
>> 
>>      /* Make sure all file formats flush their mutable metadata */
>>      bdrv_invalidate_cache_all(&local_err);
>>      if (local_err) {
>> +        migrate_generate_event(MIGRATION_STATUS_FAILED);
>>          error_report_err(local_err);
>>          migrate_decompress_threads_join();
>>          exit(EXIT_FAILURE);
>> @@ -320,6 +320,12 @@ static void process_incoming_migration_co(void *opaque)
>>          runstate_set(global_state_get_runstate());
>>      }
>>      migrate_decompress_threads_join();
>> +    /*
>> +     * This must happen after any state changes since as soon as an external
>> +     * observer sees this event they might start to prod at the VM assuming
>> +     * it's ready to use.
>> +     */
>> +    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
>>  }
>> 
>>  void process_incoming_migration(QEMUFile *f)
>> 

Re: [Qemu-devel] [PULL 0/3] Migration pull

[Peter Maydell]

On 15 October 2015 at 07:27, Juan Quintela <quintela@redhat.com> wrote:
>
> Hi Peter
>
> This is a pull request with the small fixes from the list, please apply.
>
> Later, Juan
>
> The following changes since commit c49d3411faae8ffaab8f7e5db47405a008411c10:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12' into staging (2015-10-13 10:42:06 +0100)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20151015
>
> for you to fetch changes up to 60be6340796e66b5ac8aac2d98dde5c79336a89c:
>
>   migration: fix deadlock (2015-10-15 08:14:13 +0200)
>
> ----------------------------------------------------------------
> migration/next for 20151015
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM