| Message ID | 20190325101556.30227-1-yury-kotov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | hostmem: Disable add/del memory during migration | expand |
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. 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;
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
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;
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. 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(-)