[Qemu-devel] [RFC 0/2] Disable hotplug during migration

[Qemu-devel] [RFC 0/2] Disable hotplug during migration

[Juan Quintela]

Hi

This series disable hotplug/unplug during migration.  Thank to Markus
for explaining where I had to put the checks.  Why?  Because during
migration we will fail if there are changes.  For instance, in
postcopy, if we add a memory region, we would failing.  Same for other
devices if they are not setup exactly the same on destination.

Iidea would be to disable it, andthen enable for the thing that we know that work.

This series are on top of my previous RAMState v2 serie.

Commets, please?

Thanks, Juan.


Juan Quintela (2):
  migration: Disable hotplug/unplug during migration
  ram: remove migration_bitmap_extend()

 exec.c                  |  1 -
 hw/core/qdev.c          |  5 +++++
 include/exec/ram_addr.h |  2 --
 migration/ram.c         | 34 ----------------------------------
 qdev-monitor.c          |  7 ++++++-
 5 files changed, 11 insertions(+), 38 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] migration: Disable hotplug/unplug during migration

[Juan Quintela]

Until we have reviewed what can/can't be hotplug during migration,
disable it.  We can enable it later for the things that we know that
work.  For instance, memory hotplug during postcopy don't work
currently.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/core/qdev.c | 5 +++++
 qdev-monitor.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1e7fb33..8c4a3f3 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -277,6 +277,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     HotplugHandler *hotplug_ctrl;
     HotplugHandlerClass *hdc;
 
+    if (!migration_is_idle()) {
+        error_setg(errp, "device_add not allowed while migrating");
+        return;
+    }
+
     if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 5f2fcdf..e0622b4 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -29,7 +29,7 @@
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
 #include "sysemu/block-backend.h"
-
+#include "migration/migration.h"
 /*
  * Aliases were a bad idea from the start.  Let's keep them
  * from spreading further.
@@ -566,6 +566,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     BusState *bus = NULL;
     Error *err = NULL;
 
+    if (!migration_is_idle()) {
+        error_setg(errp, "device_add not allowed while migrating");
+        return NULL;
+    }
+
     driver = qemu_opt_get(opts, "driver");
     if (!driver) {
         error_setg(errp, QERR_MISSING_PARAMETER, "driver");
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/2] migration: Disable hotplug/unplug during migration

[Thomas Huth]

On 23.03.2017 21:50, Juan Quintela wrote:
> Until we have reviewed what can/can't be hotplug during migration,
> disable it.  We can enable it later for the things that we know that
> work.  For instance, memory hotplug during postcopy don't work
> currently.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  qdev-monitor.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 1e7fb33..8c4a3f3 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -277,6 +277,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      HotplugHandler *hotplug_ctrl;
>      HotplugHandlerClass *hdc;
>  
> +    if (!migration_is_idle()) {
> +        error_setg(errp, "device_add not allowed while migrating");
> +        return;
> +    }
> +
>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5f2fcdf..e0622b4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -29,7 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/help_option.h"
>  #include "sysemu/block-backend.h"
> -
> +#include "migration/migration.h"
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
>   * from spreading further.

Maybe better keep an empty line between the include statement and the
comment?

 Thomas


> @@ -566,6 +566,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>      BusState *bus = NULL;
>      Error *err = NULL;
>  
> +    if (!migration_is_idle()) {
> +        error_setg(errp, "device_add not allowed while migrating");
> +        return NULL;
> +    }
> +
>      driver = qemu_opt_get(opts, "driver");
>      if (!driver) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "driver");
> 

Re: [Qemu-devel] [PATCH 1/2] migration: Disable hotplug/unplug during migration

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 23.03.2017 21:50, Juan Quintela wrote:
>> Until we have reviewed what can/can't be hotplug during migration,
>> disable it.  We can enable it later for the things that we know that
>> work.  For instance, memory hotplug during postcopy don't work
>> currently.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/core/qdev.c | 5 +++++
>>  qdev-monitor.c | 7 ++++++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 1e7fb33..8c4a3f3 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -277,6 +277,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>>      HotplugHandler *hotplug_ctrl;
>>      HotplugHandlerClass *hdc;
>>  
>> +    if (!migration_is_idle()) {
>> +        error_setg(errp, "device_add not allowed while migrating");
>> +        return;
>> +    }
>> +
>>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>>          error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>>          return;
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 5f2fcdf..e0622b4 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -29,7 +29,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/help_option.h"
>>  #include "sysemu/block-backend.h"
>> -
>> +#include "migration/migration.h"
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>>   * from spreading further.
>
> Maybe better keep an empty line between the include statement and the
> comment?

Oops, sure.

>
>  Thomas
>
>
>> @@ -566,6 +566,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>      BusState *bus = NULL;
>>      Error *err = NULL;
>>  
>> +    if (!migration_is_idle()) {
>> +        error_setg(errp, "device_add not allowed while migrating");
>> +        return NULL;
>> +    }
>> +
>>      driver = qemu_opt_get(opts, "driver");
>>      if (!driver) {
>>          error_setg(errp, QERR_MISSING_PARAMETER, "driver");
>> 

Re: [Qemu-devel] [PATCH 1/2] migration: Disable hotplug/unplug during migration

[Igor Mammedov]

On Thu, 23 Mar 2017 21:50:24 +0100
Juan Quintela <quintela@redhat.com> wrote:

> Until we have reviewed what can/can't be hotplug during migration,
> disable it.  We can enable it later for the things that we know that
> work.  For instance, memory hotplug during postcopy don't work
> currently.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  qdev-monitor.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 1e7fb33..8c4a3f3 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -277,6 +277,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      HotplugHandler *hotplug_ctrl;
>      HotplugHandlerClass *hdc;
>  
> +    if (!migration_is_idle()) {
> +        error_setg(errp, "device_add not allowed while migrating");
s/device_add/device_del/


I'd put this check after "if (!dc->hotpluggable)" block, so error
message won't hide wrong upluggable device error cases

> +        return;
> +    }
> +
>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5f2fcdf..e0622b4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -29,7 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/help_option.h"
>  #include "sysemu/block-backend.h"
> -
> +#include "migration/migration.h"
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
>   * from spreading further.
> @@ -566,6 +566,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>      BusState *bus = NULL;
>      Error *err = NULL;
>  
> +    if (!migration_is_idle()) {
> +        error_setg(errp, "device_add not allowed while migrating");
> +        return NULL;
> +    }
the same here, do it right before "/* create device */"

>      driver = qemu_opt_get(opts, "driver");
>      if (!driver) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "driver");

Re: [Qemu-devel] [PATCH 1/2] migration: Disable hotplug/unplug during migration

[Juan Quintela]

Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 23 Mar 2017 21:50:24 +0100
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Until we have reviewed what can/can't be hotplug during migration,
>> disable it.  We can enable it later for the things that we know that
>> work.  For instance, memory hotplug during postcopy don't work
>> currently.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/core/qdev.c | 5 +++++
>>  qdev-monitor.c | 7 ++++++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 1e7fb33..8c4a3f3 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -277,6 +277,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>>      HotplugHandler *hotplug_ctrl;
>>      HotplugHandlerClass *hdc;
>>  
>> +    if (!migration_is_idle()) {
>> +        error_setg(errp, "device_add not allowed while migrating");
> s/device_add/device_del/
>
>
> I'd put this check after "if (!dc->hotpluggable)" block, so error
> message won't hide wrong upluggable device error cases

Thanks.

I did both changes.  Not sure yet what *should* came 1st:
- device is not plugable/unplugable
- you can't do it while on migration

But I don't care, and as you seemed to preffer that other way, changed.

Later, Juan.

>> +        return;
>> +    }
>> +
>>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>>          error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>>          return;
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 5f2fcdf..e0622b4 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -29,7 +29,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/help_option.h"
>>  #include "sysemu/block-backend.h"
>> -
>> +#include "migration/migration.h"
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>>   * from spreading further.
>> @@ -566,6 +566,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>      BusState *bus = NULL;
>>      Error *err = NULL;
>>  
>> +    if (!migration_is_idle()) {
>> +        error_setg(errp, "device_add not allowed while migrating");
>> +        return NULL;
>> +    }
> the same here, do it right before "/* create device */"
>
>>      driver = qemu_opt_get(opts, "driver");
>>      if (!driver) {
>>          error_setg(errp, QERR_MISSING_PARAMETER, "driver");

[Qemu-devel] [PATCH 2/2] ram: remove migration_bitmap_extend()

[Juan Quintela]

We have disabled memory hotplug, so we don't need to handle
migration_bitamp there.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 exec.c                  |  1 -
 include/exec/ram_addr.h |  2 --
 migration/ram.c         | 34 ----------------------------------
 3 files changed, 37 deletions(-)

diff --git a/exec.c b/exec.c
index 2cae288..e51e1b1 100644
--- a/exec.c
+++ b/exec.c
@@ -1758,7 +1758,6 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
     new_ram_size = MAX(old_ram_size,
               (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS);
     if (new_ram_size > old_ram_size) {
-        migration_bitmap_extend(old_ram_size, new_ram_size);
         dirty_memory_extend(old_ram_size, new_ram_size);
     }
     /* Keep the list sorted from biggest to smallest block.  Unlike QTAILQ,
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index bbbfc7d..c246b55 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -413,7 +413,5 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
 
     return num_dirty;
 }
-
-void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
 #endif
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index 4d62788..045b899 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1494,40 +1494,6 @@ static void ram_state_reset(RAMState *rs)
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
 
-void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
-{
-    RAMState *rs = &ram_state;
-
-    /* called in qemu main thread, so there is
-     * no writing race against this migration_bitmap
-     */
-    if (rs->ram_bitmap) {
-        RAMBitmap *old_bitmap = rs->ram_bitmap, *bitmap;
-        bitmap = g_new(RAMBitmap, 1);
-        bitmap->bmap = bitmap_new(new);
-
-        /* prevent migration_bitmap content from being set bit
-         * by migration_bitmap_sync_range() at the same time.
-         * it is safe to migration if migration_bitmap is cleared bit
-         * at the same time.
-         */
-        qemu_mutex_lock(&rs->bitmap_mutex);
-        bitmap_copy(bitmap->bmap, old_bitmap->bmap, old);
-        bitmap_set(bitmap->bmap, old, new - old);
-
-        /* We don't have a way to safely extend the sentmap
-         * with RCU; so mark it as missing, entry to postcopy
-         * will fail.
-         */
-        bitmap->unsentmap = NULL;
-
-        atomic_rcu_set(&rs->ram_bitmap, bitmap);
-        qemu_mutex_unlock(&rs->bitmap_mutex);
-        rs->migration_dirty_pages += new - old;
-        call_rcu(old_bitmap, migration_bitmap_free, rcu);
-    }
-}
-
 /*
  * 'expected' is the value you expect the bitmap mostly to be full
  * of; it won't bother printing lines that are all this value.
-- 
2.9.3

Re: [Qemu-devel] [RFC 0/2] Disable hotplug during migration

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Hi
> 
> This series disable hotplug/unplug during migration.  Thank to Markus
> for explaining where I had to put the checks.  Why?  Because during
> migration we will fail if there are changes.  For instance, in
> postcopy, if we add a memory region, we would failing.  Same for other
> devices if they are not setup exactly the same on destination.
> 
> Iidea would be to disable it, andthen enable for the thing that we know that work.
> 
> This series are on top of my previous RAMState v2 serie.
> 
> Commets, please?

So I think this is probably a good idea, but we should ask Li Zhijian who added
migration_bitmap_extend if the reason for adding it was because they
needed the hot add to work.
cc'd.

IMHO we really need a 'configuration change mutex' that you can take
to stop any changes in the VM hardware, and that would probably reduce
a lot of the places that hold the BQL for just stopping changes.

Dave

> 
> Thanks, Juan.
> 
> 
> Juan Quintela (2):
>   migration: Disable hotplug/unplug during migration
>   ram: remove migration_bitmap_extend()
> 
>  exec.c                  |  1 -
>  hw/core/qdev.c          |  5 +++++
>  include/exec/ram_addr.h |  2 --
>  migration/ram.c         | 34 ----------------------------------
>  qdev-monitor.c          |  7 ++++++-
>  5 files changed, 11 insertions(+), 38 deletions(-)
> 
> -- 
> 2.9.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 0/2] Disable hotplug during migration

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Hi
>> 
>> This series disable hotplug/unplug during migration.  Thank to Markus
>> for explaining where I had to put the checks.  Why?  Because during
>> migration we will fail if there are changes.  For instance, in
>> postcopy, if we add a memory region, we would failing.  Same for other
>> devices if they are not setup exactly the same on destination.
>> 
>> Iidea would be to disable it, andthen enable for the thing that we know that work.
>> 
>> This series are on top of my previous RAMState v2 serie.
>> 
>> Commets, please?
>
> So I think this is probably a good idea, but we should ask Li Zhijian who added
> migration_bitmap_extend if the reason for adding it was because they
> needed the hot add to work.
> cc'd.
>
> IMHO we really need a 'configuration change mutex' that you can take
> to stop any changes in the VM hardware, and that would probably reduce
> a lot of the places that hold the BQL for just stopping changes.

That was the previous patch.  With that one, you can't add/remove
hardware during migration.

Later, Juan.

Re: [Qemu-devel] [RFC 0/2] Disable hotplug during migration

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Hi
>> 
>> This series disable hotplug/unplug during migration.  Thank to Markus
>> for explaining where I had to put the checks.  Why?  Because during
>> migration we will fail if there are changes.  For instance, in
>> postcopy, if we add a memory region, we would failing.  Same for other
>> devices if they are not setup exactly the same on destination.
>> 
>> Iidea would be to disable it, andthen enable for the thing that we know that work.
>> 
>> This series are on top of my previous RAMState v2 serie.
>> 
>> Commets, please?
>
> So I think this is probably a good idea, but we should ask Li Zhijian who added
> migration_bitmap_extend if the reason for adding it was because they
> needed the hot add to work.
> cc'd.

I guess that the problem for them is that the bitmap size was wrong
after memory resizing.  But I don't know if this has ever worked.


> IMHO we really need a 'configuration change mutex' that you can take
> to stop any changes in the VM hardware, and that would probably reduce
> a lot of the places that hold the BQL for just stopping changes.

That was the previous patch.  With that one, you can't add/remove
hardware during migration.

Later, Juan.