Re: [Qemu-devel] [PATCH] hostmem: Disable add/del memory during migration

Re: [Qemu-devel] [PATCH] hostmem: Disable add/del memory during migration

[Daniel P. Berrangé]

On Mon, Mar 25, 2019 at 12:52:06PM +0100, Juan Quintela wrote:
> Yury Kotov <yury-kotov@yandex-team.ru> wrote:
> > I found a bug in QEMU 2.12 with adding memory-backend while live migration
> > thread is running.
> >
> > But it seems that this bug was implicitly fixed in this commit (QEMU 3.0):
> >   b895de50: migration: discard non-migratable RAMBlocks
> >
> > I think it's better to disallow add/del memory backends during migration to
> > to prevent other possible problems. Anyway, user can't use this memory because
> > of disabled hotplug/hotunplug devs.
> 
> Hi
> 
> My understanding is that we already disable memory hotplug/unplug during
> migration. At least, the idea of those patches was to disable all
> hotplug/unplug during migration.  The only reason that I can think for
> using this patch is if anyone is planning about support some
> hotplug/upplug during migration (to my knowledge, nobody is working on
> that).
> 
> So, I think it is better to just disallow on high level all hoplug
> operations, instead of in all backends.
> 
> But will wait to see what everybody else think about it.

FWIW, libvirt already rejects any API call that would change guest
ABI while migration is taking place, which covers any hotplug or
hotunplug operation for devices or memory or anything else.

In fact libvirt whitelist which QMP commands it is willing to allow
during migration to only those which it knows are going to be safe
to use.  Libvirt's whitelist is quite narrow since there are many
QMP command it'll never run at any time.

> BTW,   if we plan to ship this for an old qemu, I will try to disable
> hotplug for all devices, not only memory, no?

Perhaps QMP could have some concept of a dynamic whitelist of commands,
so at start of migration, migration code would register a whitelist with
QMP, so it would reject everything else upfront. This would avoid needing
to put migration checks into every QMP command handler that can cause
problems ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] hostmem: Disable add/del memory during migration

[Yury Kotov]

25.03.2019, 14:58, "Juan Quintela" <quintela@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>  I found a bug in QEMU 2.12 with adding memory-backend while live migration
>>  thread is running.
>>
>>  But it seems that this bug was implicitly fixed in this commit (QEMU 3.0):
>>    b895de50: migration: discard non-migratable RAMBlocks
>>
>>  I think it's better to disallow add/del memory backends during migration to
>>  to prevent other possible problems. Anyway, user can't use this memory because
>>  of disabled hotplug/hotunplug devs.
>
> Hi
>
> My understanding is that we already disable memory hotplug/unplug during
> migration. At least, the idea of those patches was to disable all
> hotplug/unplug during migration. The only reason that I can think for
> using this patch is if anyone is planning about support some
> hotplug/upplug during migration (to my knowledge, nobody is working on
> that).
>

Sorry, what patches do you mean? It seems that I missed them...

> So, I think it is better to just disallow on high level all hoplug
> operations, instead of in all backends.
>

Agreed. I also had such idea and I did a quick look on this, but din't found how
to do it without some refactoring.

> But will wait to see what everybody else think about it.
>
> BTW, if we plan to ship this for an old qemu, I will try to disable
> hotplug for all devices, not only memory, no?
>
> Later, Juan.
>
>>  The idea of this commit is the same as that:
>>    b06424de: migration: Disable hotplug/unplug memory during migration
>>
>>  Backtrace of this bug in QEMU 2.12:
>>  0 find_next_bit (addr=addr@entry=0x0, size=size@entry=262144, offset=offset@entry=0) at util/bitops.c:46
>>  1 migration_bitmap_find_dirty (rs=0x7f58f80008c0, start=0, rb=0x5557e66e3200) at migration/ram.c:816
>>  2 find_dirty_block (again=<synthetic pointer>, pss=<synthetic pointer>, rs=0x7f58f80008c0) at migration/ram.c:1243
>>  3 ram_find_and_save_block (rs=rs@entry=0x7f58f80008c0, last_stage=last_stage@entry=false) at migration/ram.c:1592
>>  4 ram_find_and_save_block (last_stage=false, rs=0x7f58f80008c0) at migration/ram.c:2335
>>  5 ram_save_iterate (f=0x5557e69f1000, opaque=<optimized out>) at migration/ram.c:2338
>>  6 qemu_savevm_state_iterate (f=0x5557e69f1000, postcopy=false) at migration/savevm.c:1191
>>  7 migration_iteration_run (s=0x5557e666b030) at migration/migration.c:2301
>>  8 migration_thread (opaque=0x5557e666b030) at migration/migration.c:2409
>>  9 start_thread (arg=0x7f59055d5700) at pthread_create.c:333
>>  10 clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>>
>>  Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>>  ---
>>   backends/hostmem.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>>  diff --git a/backends/hostmem.c b/backends/hostmem.c
>>  index f61093654e..5c71bd3f6b 100644
>>  --- a/backends/hostmem.c
>>  +++ b/backends/hostmem.c
>>  @@ -18,6 +18,7 @@
>>   #include "qapi/visitor.h"
>>   #include "qemu/config-file.h"
>>   #include "qom/object_interfaces.h"
>>  +#include "migration/misc.h"
>>
>>   #ifdef CONFIG_NUMA
>>   #include <numaif.h>
>>  @@ -271,6 +272,11 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>>       void *ptr;
>>       uint64_t sz;
>>
>>  + if (!migration_is_idle()) {
>>  + error_setg(errp, "Adding memory-backend isn't allowed while migrating");
>>  + goto out;
>>  + }
>>  +
>>       if (bc->alloc) {
>>           bc->alloc(backend, &local_err);
>>           if (local_err) {
>>  @@ -344,7 +350,8 @@ out:
>>   static bool
>>   host_memory_backend_can_be_deleted(UserCreatable *uc)
>>   {
>>  - if (host_memory_backend_is_mapped(MEMORY_BACKEND(uc))) {
>>  + if (host_memory_backend_is_mapped(MEMORY_BACKEND(uc)) ||
>>  + !migration_is_idle()) {
>>           return false;
>>       } else {
>>           return true;

Regards,
Yury