[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating
From: |
David Hildenbrand |
Subject: |
Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating |
Date: |
Fri, 14 Feb 2020 19:26:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 14.02.20 19:11, Peter Xu wrote:
> On Fri, Feb 14, 2020 at 06:32:35PM +0100, David Hildenbrand wrote:
>> On 14.02.20 18:29, Peter Xu wrote:
>>> On Fri, Feb 14, 2020 at 01:02:46PM +0100, David Hildenbrand wrote:
>>>> From c0049ac2e95d6756037db918852c507fb88297d9 Mon Sep 17 00:00:00 2001
>>>> From: David Hildenbrand <address@hidden>
>>>> Date: Fri, 14 Feb 2020 13:01:03 +0100
>>>> Subject: [PATCH v1] tmp
>>>>
>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>> ---
>>>> migration/migration.c | 9 +++++++--
>>>> migration/migration.h | 1 +
>>>> migration/ram.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 50 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 3a21a4686c..0e7efe2920 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>>> }
>>>> }
>>>>
>>>> +void migration_cancel(void)
>>>> +{
>>>> + migrate_fd_cancel(current_migration);
>>>> +}
>>>> +
>>>> void migration_shutdown(void)
>>>> {
>>>> /*
>>>> * Cancel the current migration - that will (eventually)
>>>> * stop the migration using this structure
>>>> */
>>>> - migrate_fd_cancel(current_migration);
>>>> + migration_cancel();
>>>> object_unref(OBJECT(current_migration));
>>>> }
>>>>
>>>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool
>>>> blk,
>>>>
>>>> void qmp_migrate_cancel(Error **errp)
>>>> {
>>>> - migrate_fd_cancel(migrate_get_current());
>>>> + migration_cancel();
>>>> }
>>>>
>>>> void qmp_migrate_continue(MigrationStatus state, Error **errp)
>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>> index 8473ddfc88..79fd74afa5 100644
>>>> --- a/migration/migration.h
>>>> +++ b/migration/migration.h
>>>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func,
>>>> void *opaque);
>>>> void migration_make_urgent_request(void);
>>>> void migration_consume_urgent_request(void);
>>>> bool migration_rate_limit(void);
>>>> +void migration_cancel(void);
>>>>
>>>> #endif
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index ed23ed1c7c..f86f32b453 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -52,6 +52,7 @@
>>>> #include "migration/colo.h"
>>>> #include "block.h"
>>>> #include "sysemu/sysemu.h"
>>>> +#include "sysemu/runstate.h"
>>>> #include "savevm.h"
>>>> #include "qemu/iov.h"
>>>> #include "multifd.h"
>>>> @@ -3710,8 +3711,49 @@ static SaveVMHandlers savevm_ram_handlers = {
>>>> .resume_prepare = ram_resume_prepare,
>>>> };
>>>>
>>>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>> + size_t old_size, size_t new_size)
>>>> +{
>>>> + /*
>>>> + * We don't care about resizes triggered on incoming migration (when
>>>> + * syncing ram blocks), or of course, when no migration is going on.
>>>> + */
>>>> + if (migration_is_idle() || !runstate_is_running()) {
>>>> + return;
>>>> + }
>>>
>>> I feel like migration_is_idle() check is enough. Firstly, I feel like
>>> we allow migration even with VM stopped. At the meantime, if VM is
>>> not running, I see no reason that this resizing will happen after all? :)
>>
>> Migration code resizes ram blocks when synchronizing the ram state. See
>> qemu_ram_resize() in ram_load_precopy()
>>
>> That happens on incoming migration while the vm is stopped.
>
> Ah so your comment is for that which I misread. I'm surprised even
> the incoming migration will set MigrationState and migration_is_idle()
I was already surprised by that :) And states are not well document
(IOW, not documented)
> will return false for it, since we've got MigrationIncomingState after
> all. But yeh that seems to be correct.
>
> Then it seems fine. It's just a bit unclear even with the comment.
> Another alternative is we only register this resize() hook when
> migration starts, and unregister it when stopped. Then we can even
> drop the migration_is_idle().
Yeah, I prefer the current code because its simpler (and I don't have to
worry about races when registering/unregistering notifiers, because
locking in combination with migration is still a big puzzle to me)
I'll try to make the comment clearer, thanks!
>
>>
>>>
>>>> +
>>>> + if (!postcopy_is_running()) {
>>>> + Error *err = NULL;
>>>> +
>>>> + /*
>>>> + * Precopy code cannot deal with the size of ram blocks changing
>>>> at
>>>> + * random points in time. We're still running on the source, abort
>>>> + * the migration and continue running here. Make sure to wait
>>>> until
>>>> + * migration was canceled.
>>>> + */
>>>> + error_setg(&err, "RAM resized during precopy.");
>>>> + migrate_set_error(migrate_get_current(), err);
>>>> + error_free(err);
>>>> + migration_cancel();
>>>> + } else {
>>>> + /*
>>>> + * Postcopy code cannot deal with the size of ram blocks changing
>>>> at
>>>> + * random points in time. We're running on the target. Fail hard.
>>>> + *
>>>> + * TODO: How to handle this in a better way?
>>>> + */
>>>> + error_report("RAM resized during postcopy.");
>>>> + exit(-1);
>>>
>>> Now I'm rethinking the postcopy case....
>>>
>>> ram_dirty_bitmap_reload() should only happen during the postcopy
>>> recovery, and when that happens the VM should be stopped on both
>>> sides. Which means, ram resizing should not trigger there...
>>
>> But that guest got the chance to run for a bit and eventually reboot
>> AFAIK. Also, there are other data races possible when used_length
>> suddenly changes, this is just the most obvious one where things will;
>> get screwed up.
>
> Right, the major one could be in ram_load_postcopy() where we call
> host_from_ram_block_offset(). However if FW extension is the major
> use case then it seems to still work (still better than crashing,
> isn't it? :).
"Let's close our eyes and hope it will never happen" ? :) No, I don't
like that. This screams for a better solution long term, and until then
a proper fencing IMHO. We're making here wild guesses about data races
and why they might not be that bad in certain cases (did I mention
load/store tearing? used_length is not an atomic value ...).
--
Thanks,
David / dhildenb
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, (continued)
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Dr. David Alan Gilbert, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Juan Quintela, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Dr. David Alan Gilbert, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Peter Xu, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Peter Xu, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating,
David Hildenbrand <=
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Peter Xu, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, David Hildenbrand, 2020/02/14
- Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating, Peter Xu, 2020/02/14