[PATCH RFC] memory: Don't allow to resize RAM while migrating

[PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

Resizing while migrating is dangerous and does not work as expected.
The whole migration code works on the usable_length of ram blocks and does
not expect this to change at random points in time.

Precopy: The ram block size must not change on the source, after
ram_save_setup(), so as long as the guest is still running on the source.

Postcopy: The ram block size must not change on the target, after
synchronizing the RAM block list (ram_load_precopy()).

AFAIKS, resizing can be trigger *after* (but not during) a reset in
ACPI code by the guest
- hw/arm/virt-acpi-build.c:acpi_ram_update()
- hw/i386/acpi-build.c:acpi_ram_update()

I see no easy way to work around this. Fail hard instead of failing
somewhere in migration code due to strange other reasons. AFAIKs, the
rebuilts will be triggered during reboot, so this should not affect
running guests, but only guests that reboot at a very bad time and
actually require size changes.

Let's further limit the impact by checking if an actual resize of the
RAM (in number of pages) is required.

Don't perform the checks in qemu_ram_resize(), as that's called during
migration when syncing the used_length. Update documentation.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Shannon Zhao <shannon.zhao@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

Any idea how to avoid killing the guest? Anything obvious I am missing?

---
 exec.c                |  6 ++++--
 include/exec/memory.h | 11 +++++++----
 memory.c              | 12 ++++++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index 67e520d18e..faa6708414 100644
--- a/exec.c
+++ b/exec.c
@@ -2116,8 +2116,10 @@ static int memory_try_enable_merging(void *addr, size_t len)
     return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
 }
 
-/* Only legal before guest might have detected the memory size: e.g. on
- * incoming migration, or right after reset.
+/*
+ * RAM must not be resized while migration is active (except from migration
+ * code). Care has to be taken if the guest might have already detected
+ * the memory.
  *
  * As memory core doesn't know how is memory accessed, it is up to
  * resize callback to update device state and/or add assertions to detect
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e85b7de99a..1e5bfbe805 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -113,7 +113,8 @@ typedef struct IOMMUNotifier IOMMUNotifier;
 #define RAM_SHARED     (1 << 1)
 
 /* Only a portion of RAM (used_length) is actually used, and migrated.
- * This used_length size can change across reboots.
+ * RAM must not be resized while migration is active (except from migration
+ * code).
  */
 #define RAM_RESIZEABLE (1 << 2)
 
@@ -843,7 +844,8 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
  *                                     RAM.  Accesses into the region will
  *                                     modify memory directly.  Only an initial
  *                                     portion of this RAM is actually used.
- *                                     The used size can change across reboots.
+ *                                     The size must not change while migration
+ *                                     is active.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
@@ -1464,8 +1466,9 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
 
 /* memory_region_ram_resize: Resize a RAM region.
  *
- * Only legal before guest might have detected the memory size: e.g. on
- * incoming migration, or right after reset.
+ * RAM must not be resized while migration is active (except from migration
+ * code). Care has to be taken if the guest might have already detected
+ * the memory.
  *
  * @mr: a memory region created with @memory_region_init_resizeable_ram.
  * @newsize: the new size the region
diff --git a/memory.c b/memory.c
index aeaa8dcc9e..7fa048aa3a 100644
--- a/memory.c
+++ b/memory.c
@@ -34,6 +34,7 @@
 #include "sysemu/accel.h"
 #include "hw/boards.h"
 #include "migration/vmstate.h"
+#include "migration/misc.h"
 
 //#define DEBUG_UNASSIGNED
 
@@ -2204,6 +2205,17 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
 {
     assert(mr->ram_block);
 
+    /*
+     * Resizing RAM while migrating is not possible, as the used_length of
+     * RAM blocks must neither change on the source (precopy), nor on the
+     * target (postcopy) as long as migration code is active.
+     */
+    if (HOST_PAGE_ALIGN(newsize) != mr->ram_block->used_length &&
+        !migration_is_idle()) {
+        error_setg(errp, "Cannot resize RAM while migrating.");
+        return;
+    }
+
     qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
-- 
2.24.1

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
> 
> Precopy: The ram block size must not change on the source, after
> ram_save_setup(), so as long as the guest is still running on the source.
> 
> Postcopy: The ram block size must not change on the target, after
> synchronizing the RAM block list (ram_load_precopy()).
> 
> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()

What can be the pre-condition of triggering this after reset?  I'm
thinking whether QEMU can be aware of this "resizing can happen"
condition, then we could simply stop the migration from happening even
before the resizing happens.  Thanks,

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
>
> Precopy: The ram block size must not change on the source, after
> ram_save_setup(), so as long as the guest is still running on the source.
>
> Postcopy: The ram block size must not change on the target, after
> synchronizing the RAM block list (ram_load_precopy()).
>
> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()
>
> I see no easy way to work around this. Fail hard instead of failing
> somewhere in migration code due to strange other reasons. AFAIKs, the
> rebuilts will be triggered during reboot, so this should not affect
> running guests, but only guests that reboot at a very bad time and
> actually require size changes.
>
> Let's further limit the impact by checking if an actual resize of the
> RAM (in number of pages) is required.
>
> Don't perform the checks in qemu_ram_resize(), as that's called during
> migration when syncing the used_length. Update documentation.
>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---


>
> Any idea how to avoid killing the guest? Anything obvious I am missing?

If you avoid the resize, it should be ok for both precopy & postcopy.

But, as you point, if acpi guest is the one changing sizes, we are in
trouble.  But really, it makes exactly zero sense to reset during
migrate.  if we _could_ catch the reset, the "intelligent" thing to do
is:

- detect reset
- launch guest on destination from zero.

I.e. not migration at all.  This would be my "better" idea, but I have
no clue how to catch that kind of things in a sane way that works in
every architecture.

You get the:

Reviewed-by: Juan Quintela <quintela@redhat.com>

because:
- your code change makes sense
- the documentation update is good.

Thanks, Juan.

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 13.02.20 20:09, Juan Quintela wrote:
> David Hildenbrand <david@redhat.com> wrote:
>> Resizing while migrating is dangerous and does not work as expected.
>> The whole migration code works on the usable_length of ram blocks and does
>> not expect this to change at random points in time.
>>
>> Precopy: The ram block size must not change on the source, after
>> ram_save_setup(), so as long as the guest is still running on the source.
>>
>> Postcopy: The ram block size must not change on the target, after
>> synchronizing the RAM block list (ram_load_precopy()).
>>
>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>> ACPI code by the guest
>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>> - hw/i386/acpi-build.c:acpi_ram_update()
>>
>> I see no easy way to work around this. Fail hard instead of failing
>> somewhere in migration code due to strange other reasons. AFAIKs, the
>> rebuilts will be triggered during reboot, so this should not affect
>> running guests, but only guests that reboot at a very bad time and
>> actually require size changes.
>>
>> Let's further limit the impact by checking if an actual resize of the
>> RAM (in number of pages) is required.
>>
>> Don't perform the checks in qemu_ram_resize(), as that's called during
>> migration when syncing the used_length. Update documentation.
>>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> 
> 
>>
>> Any idea how to avoid killing the guest? Anything obvious I am missing?
> 
> If you avoid the resize, it should be ok for both precopy & postcopy.
> 
> But, as you point, if acpi guest is the one changing sizes, we are in
> trouble.  But really, it makes exactly zero sense to reset during
> migrate.  if we _could_ catch the reset, the "intelligent" thing to do

I guess there are cases (e.g., guest admin is different to the host
admin, "intelligent tooling") where a reset could happen while
migrating. At least failing at that point won't result in losing data,
as the guest is still booting up.

> is:
> 
> - detect reset
> - launch guest on destination from zero.

And starting completely from zero might not always be the right thing to
do ...

> 
> I.e. not migration at all.  This would be my "better" idea, but I have
> no clue how to catch that kind of things in a sane way that works in
> every architecture.

E.g., on s390x, there are different kinds of resets routed through
system reset requests. IIRC, some require memory to be kept, others to
be reset to 0 (currently not done, as discarding ram blocks while
postcopy is running does not work as expected).

Resets while migrating are really tricky when it comes to memory.
Fortunately, this case should be very rare to trigger.

> 
> You get the:
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> because:
> - your code change makes sense
> - the documentation update is good.
> 
> Thanks, Juan.
> 

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 13.02.20 19:32, Peter Xu wrote:
> On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
>> Resizing while migrating is dangerous and does not work as expected.
>> The whole migration code works on the usable_length of ram blocks and does
>> not expect this to change at random points in time.
>>
>> Precopy: The ram block size must not change on the source, after
>> ram_save_setup(), so as long as the guest is still running on the source.
>>
>> Postcopy: The ram block size must not change on the target, after
>> synchronizing the RAM block list (ram_load_precopy()).
>>
>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>> ACPI code by the guest
>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>> - hw/i386/acpi-build.c:acpi_ram_update()
> 
> What can be the pre-condition of triggering this after reset?  I'm
> thinking whether QEMU can be aware of this "resizing can happen"
> condition, then we could simply stop the migration from happening even
> before the resizing happens.  Thanks,

I think the condition is not known before the guest actually tries to
read the relevant memory areas (which trigger the rebuilt+resize, and
AFAIK, the new size depends on fw config done by the guest after the
reset). So it's hard to "predict".

In the precopy case it would be easier to abort (although, not simple
AFAIKS), in the postcopy not so easy - because you're already partially
running on the migration target.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

On Thu, Feb 13, 2020 at 08:42:23PM +0100, David Hildenbrand wrote:
> On 13.02.20 19:32, Peter Xu wrote:
> > On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
> >> Resizing while migrating is dangerous and does not work as expected.
> >> The whole migration code works on the usable_length of ram blocks and does
> >> not expect this to change at random points in time.
> >>
> >> Precopy: The ram block size must not change on the source, after
> >> ram_save_setup(), so as long as the guest is still running on the source.
> >>
> >> Postcopy: The ram block size must not change on the target, after
> >> synchronizing the RAM block list (ram_load_precopy()).
> >>
> >> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> >> ACPI code by the guest
> >> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> >> - hw/i386/acpi-build.c:acpi_ram_update()
> > 
> > What can be the pre-condition of triggering this after reset?  I'm
> > thinking whether QEMU can be aware of this "resizing can happen"
> > condition, then we could simply stop the migration from happening even
> > before the resizing happens.  Thanks,
> 
> I think the condition is not known before the guest actually tries to
> read the relevant memory areas (which trigger the rebuilt+resize, and
> AFAIK, the new size depends on fw config done by the guest after the
> reset). So it's hard to "predict".

I chimmed in without much context, sorry if I'm going to ask naive
questions. :)

What I was asking is about why the resizing can happen.  A quick read
told me that it was majorly for easier extension of ROMs (firmware
updates?).  With that, I'm imaging a common case for memory
resizing...

  (1) Source QEMU runs VM on old host, with old firmware

  (2) Migrate source QEMU to destination new host, with new and bigger
      firmware

  (3) During the migration, the ROM size on the destination will still
      be the old, referring to ram_load_precopy(), as long as no
      system reset

  (4) After migration finished, when the system reboots, memory
      resizing happens with the new and bigger firmware loaded

And is this patch trying to fix/warn when there's a reboot during (3)
so the new size is discovered at a wrong time?  Is my understanding
correct?

> 
> In the precopy case it would be easier to abort (although, not simple
> AFAIKS), in the postcopy not so easy - because you're already partially
> running on the migration target.

Prior to this patch, would a precopy still survive with such an
accident (asked because I _feel_ like migrating a ramblock with
smaller used_length to the same ramblock with bigger used_length seems
to be fine?)?  Or we can stop the precopy and restart.  After this
patch, it'll crash the source VM (&error_abort specified in
memory_region_ram_resize()), which seems a bit more harsh?

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 13.02.20 21:56, Peter Xu wrote:
> On Thu, Feb 13, 2020 at 08:42:23PM +0100, David Hildenbrand wrote:
>> On 13.02.20 19:32, Peter Xu wrote:
>>> On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
>>>> Resizing while migrating is dangerous and does not work as expected.
>>>> The whole migration code works on the usable_length of ram blocks and does
>>>> not expect this to change at random points in time.
>>>>
>>>> Precopy: The ram block size must not change on the source, after
>>>> ram_save_setup(), so as long as the guest is still running on the source.
>>>>
>>>> Postcopy: The ram block size must not change on the target, after
>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>
>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>> ACPI code by the guest
>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>
>>> What can be the pre-condition of triggering this after reset?  I'm
>>> thinking whether QEMU can be aware of this "resizing can happen"
>>> condition, then we could simply stop the migration from happening even
>>> before the resizing happens.  Thanks,
>>
>> I think the condition is not known before the guest actually tries to
>> read the relevant memory areas (which trigger the rebuilt+resize, and
>> AFAIK, the new size depends on fw config done by the guest after the
>> reset). So it's hard to "predict".
> 
> I chimmed in without much context, sorry if I'm going to ask naive
> questions. :)

I think the problem is quite involved and not obvious, so there are no
naive questions :)

> 
> What I was asking is about why the resizing can happen.  A quick read
> told me that it was majorly for easier extension of ROMs (firmware
> updates?).  With that, I'm imaging a common case for memory
> resizing...
> 
>   (1) Source QEMU runs VM on old host, with old firmware
> 
>   (2) Migrate source QEMU to destination new host, with new and bigger
>       firmware
> 
>   (3) During the migration, the ROM size on the destination will still
>       be the old, referring to ram_load_precopy(), as long as no
>       system reset
> 
>   (4) After migration finished, when the system reboots, memory
>       resizing happens with the new and bigger firmware loaded

AFAIK it could trigger

a) In precopy during the second migration.
b) In postcopy during the first migration.

> 
> And is this patch trying to fix/warn when there's a reboot during (3)
> so the new size is discovered at a wrong time?  Is my understanding
> correct?

It's trying to bail out early instead of failing at other random points
(with an unclear outcome).

>>
>> In the precopy case it would be easier to abort (although, not simple
>> AFAIKS), in the postcopy not so easy - because you're already partially
>> running on the migration target.
> 
> Prior to this patch, would a precopy still survive with such an
> accident (asked because I _feel_ like migrating a ramblock with
> smaller used_length to the same ramblock with bigger used_length seems
> to be fine?)?  Or we can stop the precopy and restart.  After this

I assume growing the region is the usual case (not shrinking). FW blobs
tend to get bigger.

Migrating while growing a ram block on the source won't work. The source
would try to send a dirt page that's outside of the used_length on the
target, making e.g., ram_load_postcopy()/ram_load_precopy() fail with
"Illegal RAM offset...".

In the postcopy case, e.g., ram_dirty_bitmap_reload() will fail in case
there is a mismatch between ram block size on source/target.

Another issue is if the used_length changes while in ram_save_setup(),
just between storing ram_bytes_total_common(true) and storing
block->used_length. A mismatch will screw up the migration stream.

But these are just the immediately visible issues. I am more concerned
about used_length changing at random points in time, resulting in more
harm. (e.g., non-obvious load-store tearing when accessing the used length)

Migration code is inherently racy when it comes to ram block resizes.
And that might become more dangerous once we want to size the migration
bitmaps smaller (used_length instead of max_length) or disallow access
to ram blocks beyond the used_length. Both are things I am working on :)

> patch, it'll crash the source VM (&error_abort specified in
> memory_region_ram_resize()), which seems a bit more harsh?

There seems to be no easy way to abort migration from outside the
migration thread. As Juan said, you actually don't want to fail
migration but instead soft-abort migration and continue running the
guest on the target on a reset. But that's not easy as well.

One could think about extending ram block notifiers to notify migration
code (before) resizes, so that migration code can work around the
resize (how is TBD). Not easy as well :)

But then, I am not sure
a) If we run into this issue in real life a lot.
b) If we actually need an elaborate solutions within QEMU to handle this
case. For now, it's sufficient to restart the VM on the migration
target. No data was lost. Not nice, but very simple.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
> 
> Precopy: The ram block size must not change on the source, after
> ram_save_setup(), so as long as the guest is still running on the source.
> 
> Postcopy: The ram block size must not change on the target, after
> synchronizing the RAM block list (ram_load_precopy()).
> 
> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()
> 
> I see no easy way to work around this. Fail hard instead of failing
> somewhere in migration code due to strange other reasons. AFAIKs, the
> rebuilts will be triggered during reboot, so this should not affect
> running guests, but only guests that reboot at a very bad time and
> actually require size changes.
> 
> Let's further limit the impact by checking if an actual resize of the
> RAM (in number of pages) is required.
> 
> Don't perform the checks in qemu_ram_resize(), as that's called during
> migration when syncing the used_length. Update documentation.

Interesting; we need to do something about this - but banning resets
during migration is a bit harsh; and aborting the source VM is really
nasty - for a precopy especially we shouldn't kill the source VM,
we should just abort the migration.

The other thing that worries me is that acpi_build_update calls
   acpi_ram_update->memory_region_ram_resize
multiple times.
So, it might be that the size you end up with at the end of
acpi_build_update is actually the same size as the original - so
the net effect is the RAMBlock didn't really get resized.

Dave


> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> Any idea how to avoid killing the guest? Anything obvious I am missing?
> 
> ---
>  exec.c                |  6 ++++--
>  include/exec/memory.h | 11 +++++++----
>  memory.c              | 12 ++++++++++++
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 67e520d18e..faa6708414 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2116,8 +2116,10 @@ static int memory_try_enable_merging(void *addr, size_t len)
>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>  }
>  
> -/* Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> +/*
> + * RAM must not be resized while migration is active (except from migration
> + * code). Care has to be taken if the guest might have already detected
> + * the memory.
>   *
>   * As memory core doesn't know how is memory accessed, it is up to
>   * resize callback to update device state and/or add assertions to detect
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e85b7de99a..1e5bfbe805 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -113,7 +113,8 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>  #define RAM_SHARED     (1 << 1)
>  
>  /* Only a portion of RAM (used_length) is actually used, and migrated.
> - * This used_length size can change across reboots.
> + * RAM must not be resized while migration is active (except from migration
> + * code).
>   */
>  #define RAM_RESIZEABLE (1 << 2)
>  
> @@ -843,7 +844,8 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>   *                                     RAM.  Accesses into the region will
>   *                                     modify memory directly.  Only an initial
>   *                                     portion of this RAM is actually used.
> - *                                     The used size can change across reboots.
> + *                                     The size must not change while migration
> + *                                     is active.
>   *
>   * @mr: the #MemoryRegion to be initialized.
>   * @owner: the object that tracks the region's reference count
> @@ -1464,8 +1466,9 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>  
>  /* memory_region_ram_resize: Resize a RAM region.
>   *
> - * Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> + * RAM must not be resized while migration is active (except from migration
> + * code). Care has to be taken if the guest might have already detected
> + * the memory.
>   *
>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>   * @newsize: the new size the region
> diff --git a/memory.c b/memory.c
> index aeaa8dcc9e..7fa048aa3a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/accel.h"
>  #include "hw/boards.h"
>  #include "migration/vmstate.h"
> +#include "migration/misc.h"
>  
>  //#define DEBUG_UNASSIGNED
>  
> @@ -2204,6 +2205,17 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
>  {
>      assert(mr->ram_block);
>  
> +    /*
> +     * Resizing RAM while migrating is not possible, as the used_length of
> +     * RAM blocks must neither change on the source (precopy), nor on the
> +     * target (postcopy) as long as migration code is active.
> +     */
> +    if (HOST_PAGE_ALIGN(newsize) != mr->ram_block->used_length &&
> +        !migration_is_idle()) {
> +        error_setg(errp, "Cannot resize RAM while migrating.");
> +        return;
> +    }
> +
>      qemu_ram_resize(mr->ram_block, newsize, errp);
>  }
>  
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> Resizing while migrating is dangerous and does not work as expected.
>> The whole migration code works on the usable_length of ram blocks and does
>> not expect this to change at random points in time.
>>
>> Precopy: The ram block size must not change on the source, after
>> ram_save_setup(), so as long as the guest is still running on the source.
>>
>> Postcopy: The ram block size must not change on the target, after
>> synchronizing the RAM block list (ram_load_precopy()).
>>
>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>> ACPI code by the guest
>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>> - hw/i386/acpi-build.c:acpi_ram_update()
>>
>> I see no easy way to work around this. Fail hard instead of failing
>> somewhere in migration code due to strange other reasons. AFAIKs, the
>> rebuilts will be triggered during reboot, so this should not affect
>> running guests, but only guests that reboot at a very bad time and
>> actually require size changes.
>>
>> Let's further limit the impact by checking if an actual resize of the
>> RAM (in number of pages) is required.
>>
>> Don't perform the checks in qemu_ram_resize(), as that's called during
>> migration when syncing the used_length. Update documentation.
> 
> Interesting; we need to do something about this - but banning resets
> during migration is a bit harsh; and aborting the source VM is really
> nasty - for a precopy especially we shouldn't kill the source VM,
> we should just abort the migration.

Any alternative, easy solutions to handle this? I do wonder how often
this will actually trigger in real life.

> 
> The other thing that worries me is that acpi_build_update calls
>    acpi_ram_update->memory_region_ram_resize
> multiple times.

It's different memory regions, no? table_mr, rsdp_mr, linker_mr.

> So, it might be that the size you end up with at the end of
> acpi_build_update is actually the same size as the original - so
> the net effect is the RAMBlock didn't really get resized.

Are you sure?


-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> Resizing while migrating is dangerous and does not work as expected.
> >> The whole migration code works on the usable_length of ram blocks and does
> >> not expect this to change at random points in time.
> >>
> >> Precopy: The ram block size must not change on the source, after
> >> ram_save_setup(), so as long as the guest is still running on the source.
> >>
> >> Postcopy: The ram block size must not change on the target, after
> >> synchronizing the RAM block list (ram_load_precopy()).
> >>
> >> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> >> ACPI code by the guest
> >> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> >> - hw/i386/acpi-build.c:acpi_ram_update()
> >>
> >> I see no easy way to work around this. Fail hard instead of failing
> >> somewhere in migration code due to strange other reasons. AFAIKs, the
> >> rebuilts will be triggered during reboot, so this should not affect
> >> running guests, but only guests that reboot at a very bad time and
> >> actually require size changes.
> >>
> >> Let's further limit the impact by checking if an actual resize of the
> >> RAM (in number of pages) is required.
> >>
> >> Don't perform the checks in qemu_ram_resize(), as that's called during
> >> migration when syncing the used_length. Update documentation.
> > 
> > Interesting; we need to do something about this - but banning resets
> > during migration is a bit harsh; and aborting the source VM is really
> > nasty - for a precopy especially we shouldn't kill the source VM,
> > we should just abort the migration.
> 
> Any alternative, easy solutions to handle this? I do wonder how often
> this will actually trigger in real life.

Well it's not that hard to abort a migration (I'm not sure we've got a
convenient wrapper to do it - but it shouldn't be hard to add).

> > 
> > The other thing that worries me is that acpi_build_update calls
> >    acpi_ram_update->memory_region_ram_resize
> > multiple times.
> 
> It's different memory regions, no? table_mr, rsdp_mr, linker_mr.

Oh, so it is.

> > So, it might be that the size you end up with at the end of
> > acpi_build_update is actually the same size as the original - so
> > the net effect is the RAMBlock didn't really get resized.
> 
> Are you sure?

No!
Avocado has a migration+reset test, so it's worth trying.
Certainly in a cloud setup migrations happen often and no one knows
what the guest is doing;  aborting the source isn't acceptable.

It surprises me a bit that the region sizes would change due to guest
actions - I thought they were determined by the set of virtual hardware;
not sure if a hot-unplug/plug followed by reset would trigger it or not.

Dave

> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@redhat.com) wrote:
>>>> Resizing while migrating is dangerous and does not work as expected.
>>>> The whole migration code works on the usable_length of ram blocks and does
>>>> not expect this to change at random points in time.
>>>>
>>>> Precopy: The ram block size must not change on the source, after
>>>> ram_save_setup(), so as long as the guest is still running on the source.
>>>>
>>>> Postcopy: The ram block size must not change on the target, after
>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>
>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>> ACPI code by the guest
>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>
>>>> I see no easy way to work around this. Fail hard instead of failing
>>>> somewhere in migration code due to strange other reasons. AFAIKs, the
>>>> rebuilts will be triggered during reboot, so this should not affect
>>>> running guests, but only guests that reboot at a very bad time and
>>>> actually require size changes.
>>>>
>>>> Let's further limit the impact by checking if an actual resize of the
>>>> RAM (in number of pages) is required.
>>>>
>>>> Don't perform the checks in qemu_ram_resize(), as that's called during
>>>> migration when syncing the used_length. Update documentation.
>>>
>>> Interesting; we need to do something about this - but banning resets
>>> during migration is a bit harsh; and aborting the source VM is really
>>> nasty - for a precopy especially we shouldn't kill the source VM,
>>> we should just abort the migration.
>>
>> Any alternative, easy solutions to handle this? I do wonder how often
>> this will actually trigger in real life.
> 
> Well it's not that hard to abort a migration (I'm not sure we've got a
> convenient wrapper to do it - but it shouldn't be hard to add).
> 

We do have qmp_migrate_cancel(). I hope that can be called under BQL.

Can that be called in both, precopy and postcopy case? I assume in the
precopy, it's easy.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
> >>> * David Hildenbrand (david@redhat.com) wrote:
> >>>> Resizing while migrating is dangerous and does not work as expected.
> >>>> The whole migration code works on the usable_length of ram blocks and does
> >>>> not expect this to change at random points in time.
> >>>>
> >>>> Precopy: The ram block size must not change on the source, after
> >>>> ram_save_setup(), so as long as the guest is still running on the source.
> >>>>
> >>>> Postcopy: The ram block size must not change on the target, after
> >>>> synchronizing the RAM block list (ram_load_precopy()).
> >>>>
> >>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> >>>> ACPI code by the guest
> >>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> >>>> - hw/i386/acpi-build.c:acpi_ram_update()
> >>>>
> >>>> I see no easy way to work around this. Fail hard instead of failing
> >>>> somewhere in migration code due to strange other reasons. AFAIKs, the
> >>>> rebuilts will be triggered during reboot, so this should not affect
> >>>> running guests, but only guests that reboot at a very bad time and
> >>>> actually require size changes.
> >>>>
> >>>> Let's further limit the impact by checking if an actual resize of the
> >>>> RAM (in number of pages) is required.
> >>>>
> >>>> Don't perform the checks in qemu_ram_resize(), as that's called during
> >>>> migration when syncing the used_length. Update documentation.
> >>>
> >>> Interesting; we need to do something about this - but banning resets
> >>> during migration is a bit harsh; and aborting the source VM is really
> >>> nasty - for a precopy especially we shouldn't kill the source VM,
> >>> we should just abort the migration.
> >>
> >> Any alternative, easy solutions to handle this? I do wonder how often
> >> this will actually trigger in real life.
> > 
> > Well it's not that hard to abort a migration (I'm not sure we've got a
> > convenient wrapper to do it - but it shouldn't be hard to add).
> > 
> 
> We do have qmp_migrate_cancel(). I hope that can be called under BQL.

Well it's a monitor command so I think so; although it's not really
designed for an error - it's a user action.  Doing a
migrate_set_error(..) followed by a qemu_file_shutdown is probably a
good bet.

> Can that be called in both, precopy and postcopy case? I assume in the
> precopy, it's easy.

The problem is during postcopy you're toast when that happens because
you can't restart; however, can this happen once we're actually in
postcopy?  It's a little different - if it happens before the transition
to postcopy then it's the same as precopy; if it happens afterwards..
well it's going to happen ont he destination side and that's quite
different.

Dave

> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 14.02.20 12:02, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@redhat.com) wrote:
>>>> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
>>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>>> Resizing while migrating is dangerous and does not work as expected.
>>>>>> The whole migration code works on the usable_length of ram blocks and does
>>>>>> not expect this to change at random points in time.
>>>>>>
>>>>>> Precopy: The ram block size must not change on the source, after
>>>>>> ram_save_setup(), so as long as the guest is still running on the source.
>>>>>>
>>>>>> Postcopy: The ram block size must not change on the target, after
>>>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>>>
>>>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>>>> ACPI code by the guest
>>>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>>>
>>>>>> I see no easy way to work around this. Fail hard instead of failing
>>>>>> somewhere in migration code due to strange other reasons. AFAIKs, the
>>>>>> rebuilts will be triggered during reboot, so this should not affect
>>>>>> running guests, but only guests that reboot at a very bad time and
>>>>>> actually require size changes.
>>>>>>
>>>>>> Let's further limit the impact by checking if an actual resize of the
>>>>>> RAM (in number of pages) is required.
>>>>>>
>>>>>> Don't perform the checks in qemu_ram_resize(), as that's called during
>>>>>> migration when syncing the used_length. Update documentation.
>>>>>
>>>>> Interesting; we need to do something about this - but banning resets
>>>>> during migration is a bit harsh; and aborting the source VM is really
>>>>> nasty - for a precopy especially we shouldn't kill the source VM,
>>>>> we should just abort the migration.
>>>>
>>>> Any alternative, easy solutions to handle this? I do wonder how often
>>>> this will actually trigger in real life.
>>>
>>> Well it's not that hard to abort a migration (I'm not sure we've got a
>>> convenient wrapper to do it - but it shouldn't be hard to add).
>>>
>>
>> We do have qmp_migrate_cancel(). I hope that can be called under BQL.
> 
> Well it's a monitor command so I think so; although it's not really
> designed for an error - it's a user action.  Doing a
> migrate_set_error(..) followed by a qemu_file_shutdown is probably a
> good bet.

I'll base on "[PATCH v2 fixed 00/16] Ram blocks with resizable anonymous
allocations under POSIX", where I extend the ram block notifier with a
resize notification.

migrate/ram.c can register the notifier and react accordingly. E.g., for
precopy, abort migration. Not sure about postcopy (below).

> 
>> Can that be called in both, precopy and postcopy case? I assume in the
>> precopy, it's easy.
> 
> The problem is during postcopy you're toast when that happens because
> you can't restart; however, can this happen once we're actually in
> postcopy?  It's a little different - if it happens before the transition
> to postcopy then it's the same as precopy; if it happens afterwards..
> well it's going to happen ont he destination side and that's quite
> different.

If it happens after, we are in trouble at least with received bitmaps.
Not sure about other issues (it's a lot of code :) ). Especially
shrinking while trying to place pages will be bad and fail. It's code
that assumes used_length won't change.

ramblock_recv_bitmap_send() on the target and ram_dirty_bitmap_reload()
on the source. ram_dirty_bitmap_reload() will bail out if the sizes
don't match.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 14.02.20 12:08, David Hildenbrand wrote:
> On 14.02.20 12:02, Dr. David Alan Gilbert wrote:
>> * David Hildenbrand (david@redhat.com) wrote:
>>> On 14.02.20 11:42, Dr. David Alan Gilbert wrote:
>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>> On 14.02.20 11:25, Dr. David Alan Gilbert wrote:
>>>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>>>> Resizing while migrating is dangerous and does not work as expected.
>>>>>>> The whole migration code works on the usable_length of ram blocks and does
>>>>>>> not expect this to change at random points in time.
>>>>>>>
>>>>>>> Precopy: The ram block size must not change on the source, after
>>>>>>> ram_save_setup(), so as long as the guest is still running on the source.
>>>>>>>
>>>>>>> Postcopy: The ram block size must not change on the target, after
>>>>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>>>>
>>>>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>>>>> ACPI code by the guest
>>>>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>>>>
>>>>>>> I see no easy way to work around this. Fail hard instead of failing
>>>>>>> somewhere in migration code due to strange other reasons. AFAIKs, the
>>>>>>> rebuilts will be triggered during reboot, so this should not affect
>>>>>>> running guests, but only guests that reboot at a very bad time and
>>>>>>> actually require size changes.
>>>>>>>
>>>>>>> Let's further limit the impact by checking if an actual resize of the
>>>>>>> RAM (in number of pages) is required.
>>>>>>>
>>>>>>> Don't perform the checks in qemu_ram_resize(), as that's called during
>>>>>>> migration when syncing the used_length. Update documentation.
>>>>>>
>>>>>> Interesting; we need to do something about this - but banning resets
>>>>>> during migration is a bit harsh; and aborting the source VM is really
>>>>>> nasty - for a precopy especially we shouldn't kill the source VM,
>>>>>> we should just abort the migration.
>>>>>
>>>>> Any alternative, easy solutions to handle this? I do wonder how often
>>>>> this will actually trigger in real life.
>>>>
>>>> Well it's not that hard to abort a migration (I'm not sure we've got a
>>>> convenient wrapper to do it - but it shouldn't be hard to add).
>>>>
>>>
>>> We do have qmp_migrate_cancel(). I hope that can be called under BQL.
>>
>> Well it's a monitor command so I think so; although it's not really
>> designed for an error - it's a user action.  Doing a
>> migrate_set_error(..) followed by a qemu_file_shutdown is probably a
>> good bet.
> 
> I'll base on "[PATCH v2 fixed 00/16] Ram blocks with resizable anonymous
> allocations under POSIX", where I extend the ram block notifier with a
> resize notification.
> 
> migrate/ram.c can register the notifier and react accordingly. E.g., for
> precopy, abort migration. Not sure about postcopy (below).
> 
>>
>>> Can that be called in both, precopy and postcopy case? I assume in the
>>> precopy, it's easy.
>>
>> The problem is during postcopy you're toast when that happens because
>> you can't restart; however, can this happen once we're actually in
>> postcopy?  It's a little different - if it happens before the transition
>> to postcopy then it's the same as precopy; if it happens afterwards..
>> well it's going to happen ont he destination side and that's quite
>> different.
> 
> If it happens after, we are in trouble at least with received bitmaps.
> Not sure about other issues (it's a lot of code :) ). Especially
> shrinking while trying to place pages will be bad and fail. It's code
> that assumes used_length won't change.
> 
> ramblock_recv_bitmap_send() on the target and ram_dirty_bitmap_reload()
> on the source. ram_dirty_bitmap_reload() will bail out if the sizes
> don't match.
> 

So, with (modified) ram block notifiers it could look something like this:

From c0049ac2e95d6756037db918852c507fb88297d9 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Fri, 14 Feb 2020 13:01:03 +0100
Subject: [PATCH v1] tmp

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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;
+    }
+
+    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);
+    }
+}
+
+static RAMBlockNotifier ram_mig_ram_notifier = {
+    .ram_block_resized = ram_mig_ram_block_resized,
+};
+
 void ram_mig_init(void)
 {
     qemu_mutex_init(&XBZRLE.lock);
     register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
+    ram_block_notifier_add(&ram_mig_ram_notifier);
 }
-- 
2.24.1


Thoughts?

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Juan Quintela]

David Hildenbrand <david@redhat.com> wrote:

I agree with the removed bits.


> 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;
> +    }
> +
> +    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();

If we can't do anything else, this is reasonable.

But as discussed before, it is still not fully clear to me _why_ are
ramblocks changing if we have disabled add/remove memory during migration.

> +    } 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);

Idea is good, but we also need to exit destination, not only source, no?

> +    }
> +}



> +static RAMBlockNotifier ram_mig_ram_notifier = {
> +    .ram_block_resized = ram_mig_ram_block_resized,
> +};
> +
>  void ram_mig_init(void)
>  {
>      qemu_mutex_init(&XBZRLE.lock);
>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> +    ram_block_notifier_add(&ram_mig_ram_notifier);
>  }

Shouldn't we remove the notifier when we finish the migration.

Later, Juan.

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

>> 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;
>> +    }
>> +
>> +    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();
> 
> If we can't do anything else, this is reasonable.
> 
> But as discussed before, it is still not fully clear to me _why_ are
> ramblocks changing if we have disabled add/remove memory during migration.


Ramblock add/remove is ties to device add/remove, which we block.

Resize, however, it not. Here, the resize happens while the guest is
booting up. The content/size of the ram block depends also on previous
guest action AFAIK. There is no way from stopping the guest from doing
that. It's required for the guest to continue booting (with ACPI).

I'm currently working on a project which reuses resizable ram blocks in
different context. There, I can simply defer/avoid resizing when
migration is active. In the ACPI case, however, we cannot avoid it.

Hope that answers your question

> 
>> +    } 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);
> 
> Idea is good, but we also need to exit destination, not only source, no?

@Dave, any idea what could be the right thing to do here?

> 
>> +    }
>> +}
> 
> 
> 
>> +static RAMBlockNotifier ram_mig_ram_notifier = {
>> +    .ram_block_resized = ram_mig_ram_block_resized,
>> +};
>> +
>>  void ram_mig_init(void)
>>  {
>>      qemu_mutex_init(&XBZRLE.lock);
>>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
>> +    ram_block_notifier_add(&ram_mig_ram_notifier);
>>  }
> 
> Shouldn't we remove the notifier when we finish the migration.

It's called from main() unconditionally (so not when migration starts),
so the notifier remains active the whole QEMU lifetime (which should be
fine AFAIKT).

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> 
> >> 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;
> >> +    }
> >> +
> >> +    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();
> > 
> > If we can't do anything else, this is reasonable.
> > 
> > But as discussed before, it is still not fully clear to me _why_ are
> > ramblocks changing if we have disabled add/remove memory during migration.
> 
> 
> Ramblock add/remove is ties to device add/remove, which we block.
> 
> Resize, however, it not. Here, the resize happens while the guest is
> booting up. The content/size of the ram block depends also on previous
> guest action AFAIK. There is no way from stopping the guest from doing
> that. It's required for the guest to continue booting (with ACPI).
> 
> I'm currently working on a project which reuses resizable ram blocks in
> different context. There, I can simply defer/avoid resizing when
> migration is active. In the ACPI case, however, we cannot avoid it.
> 
> Hope that answers your question
> 
> > 
> >> +    } 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);
> > 
> > Idea is good, but we also need to exit destination, not only source, no?
> 
> @Dave, any idea what could be the right thing to do here?

I think that's OK; postcopy_is_running() will return true on the
destination (e.g. see it's use in ram_load()) and should work.

I'd really appreciate if you could print hte RAMBlock or something at
this point - when we hit this error we're going to want to try and
figure out why.

Dave

> > 
> >> +    }
> >> +}
> > 
> > 
> > 
> >> +static RAMBlockNotifier ram_mig_ram_notifier = {
> >> +    .ram_block_resized = ram_mig_ram_block_resized,
> >> +};
> >> +
> >>  void ram_mig_init(void)
> >>  {
> >>      qemu_mutex_init(&XBZRLE.lock);
> >>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> >> +    ram_block_notifier_add(&ram_mig_ram_notifier);
> >>  }
> > 
> > Shouldn't we remove the notifier when we finish the migration.
> 
> It's called from main() unconditionally (so not when migration starts),
> so the notifier remains active the whole QEMU lifetime (which should be
> fine AFAIKT).
> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

On Fri, Feb 14, 2020 at 10:17:07AM +0100, David Hildenbrand wrote:
> On 13.02.20 21:56, Peter Xu wrote:
> > On Thu, Feb 13, 2020 at 08:42:23PM +0100, David Hildenbrand wrote:
> >> On 13.02.20 19:32, Peter Xu wrote:
> >>> On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
> >>>> Resizing while migrating is dangerous and does not work as expected.
> >>>> The whole migration code works on the usable_length of ram blocks and does
> >>>> not expect this to change at random points in time.
> >>>>
> >>>> Precopy: The ram block size must not change on the source, after
> >>>> ram_save_setup(), so as long as the guest is still running on the source.
> >>>>
> >>>> Postcopy: The ram block size must not change on the target, after
> >>>> synchronizing the RAM block list (ram_load_precopy()).
> >>>>
> >>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
> >>>> ACPI code by the guest
> >>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> >>>> - hw/i386/acpi-build.c:acpi_ram_update()
> >>>
> >>> What can be the pre-condition of triggering this after reset?  I'm
> >>> thinking whether QEMU can be aware of this "resizing can happen"
> >>> condition, then we could simply stop the migration from happening even
> >>> before the resizing happens.  Thanks,
> >>
> >> I think the condition is not known before the guest actually tries to
> >> read the relevant memory areas (which trigger the rebuilt+resize, and
> >> AFAIK, the new size depends on fw config done by the guest after the
> >> reset). So it's hard to "predict".
> > 
> > I chimmed in without much context, sorry if I'm going to ask naive
> > questions. :)
> 
> I think the problem is quite involved and not obvious, so there are no
> naive questions :)
> 
> > 
> > What I was asking is about why the resizing can happen.  A quick read
> > told me that it was majorly for easier extension of ROMs (firmware
> > updates?).  With that, I'm imaging a common case for memory
> > resizing...
> > 
> >   (1) Source QEMU runs VM on old host, with old firmware
> > 
> >   (2) Migrate source QEMU to destination new host, with new and bigger
> >       firmware
> > 
> >   (3) During the migration, the ROM size on the destination will still
> >       be the old, referring to ram_load_precopy(), as long as no
> >       system reset
> > 
> >   (4) After migration finished, when the system reboots, memory
> >       resizing happens with the new and bigger firmware loaded
> 
> AFAIK it could trigger
> 
> a) In precopy during the second migration.
> b) In postcopy during the first migration.

After reading your reply - even the 1st migration of precopy?  Say,
when source QEMU resets and found changed FW during the precopy?

I'll comment postcopy below.

> 
> > 
> > And is this patch trying to fix/warn when there's a reboot during (3)
> > so the new size is discovered at a wrong time?  Is my understanding
> > correct?
> 
> It's trying to bail out early instead of failing at other random points
> (with an unclear outcome).

Yeah, I am just uncertain on whether in some cases it could be a
silent success (when used_length changed, however migration still
completed without error reported) and now we're changing it to a VM
crash... Could that happen?

  - before the patch, when precopy triggers this,

    - when it didn't encounter issue with the changed used_length, it
      could get silently ignored.  Lucky enough & good case.

    - when it triggered an error, precopy failed.  _However_, we can
      simply restart... so still not so bad.

  - after the patch, when precopy detects this, we abort
    immediately...  Which is really not good...

If you see, that's the major thing I was worrying about...

And since used_length is growing in most cases as you said (at least
before virtio-mem comes? :), I'm suspecting that could be the major
case that there will be a silent success.

> 
> >>
> >> In the precopy case it would be easier to abort (although, not simple
> >> AFAIKS), in the postcopy not so easy - because you're already partially
> >> running on the migration target.
> > 
> > Prior to this patch, would a precopy still survive with such an
> > accident (asked because I _feel_ like migrating a ramblock with
> > smaller used_length to the same ramblock with bigger used_length seems
> > to be fine?)?  Or we can stop the precopy and restart.  After this
> 
> I assume growing the region is the usual case (not shrinking). FW blobs
> tend to get bigger.
> 
> Migrating while growing a ram block on the source won't work. The source
> would try to send a dirt page that's outside of the used_length on the
> target, making e.g., ram_load_postcopy()/ram_load_precopy() fail with
> "Illegal RAM offset...".

Right.

> 
> In the postcopy case, e.g., ram_dirty_bitmap_reload() will fail in case
> there is a mismatch between ram block size on source/target.

IMHO that's an extreme rare case when (one example I can think of):

  - we start a postcopy after a precopy
  - system reset, noticed a firmware update
  - we got a network failure, postcopy interrupted
  - we try to recover a postcopy

So are you using postcopy recovery?  I will be surprised if so because
then you'll be the first user I know that really used that besides QE. :)

> 
> Another issue is if the used_length changes while in ram_save_setup(),
> just between storing ram_bytes_total_common(true) and storing
> block->used_length. A mismatch will screw up the migration stream.

Yes this seems to be another issue then.  IIUC the ramblocks are
protected by RCU, the migration code has always been with the read
lock there so logically it should see a consistent view of system
ramblocks in ram_save_setup().  IMHO the thing that was inconsistent
is that RCU is not safe enough for changing used_length for a ramblock.

> 
> But these are just the immediately visible issues. I am more concerned
> about used_length changing at random points in time, resulting in more
> harm. (e.g., non-obvious load-store tearing when accessing the used length)
> 
> Migration code is inherently racy when it comes to ram block resizes.
> And that might become more dangerous once we want to size the migration
> bitmaps smaller (used_length instead of max_length) or disallow access
> to ram blocks beyond the used_length. Both are things I am working on :)

Right. Now I start to wonder whether migration is the only special guy
here.  I noticed at least we've got:

struct RAMBlockNotifier {
    void (*ram_block_added)(RAMBlockNotifier *n, void *host, size_t size);
    void (*ram_block_removed)(RAMBlockNotifier *n, void *host, size_t size);
    QLIST_ENTRY(RAMBlockNotifier) next;
};

I suspect at least all these users could also break in some way if
resize happens.

> 
> > patch, it'll crash the source VM (&error_abort specified in
> > memory_region_ram_resize()), which seems a bit more harsh?
> 
> There seems to be no easy way to abort migration from outside the
> migration thread. As Juan said, you actually don't want to fail
> migration but instead soft-abort migration and continue running the
> guest on the target on a reset. But that's not easy as well.
> 
> One could think about extending ram block notifiers to notify migration
> code (before) resizes, so that migration code can work around the
> resize (how is TBD). Not easy as well :)

True.  But if you see my worry still stands, on whether such a patch
would make things worse by crashing it when it could still have a
chance to survive.  Shall we loose the penalty of that even if we want
to warn the user earlier?

> 
> But then, I am not sure
> a) If we run into this issue in real life a lot.

/me curious about this too.  I'd bet is it's not happening a lot, even
hardly noticed/triggered.  Since I noticed resizing is there since
2014.  :)

> b) If we actually need an elaborate solutions within QEMU to handle this
> case. For now, it's sufficient to restart the VM on the migration
> target. No data was lost. Not nice, but very simple.

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

>> a) In precopy during the second migration.
>> b) In postcopy during the first migration.
> 
> After reading your reply - even the 1st migration of precopy?  Say,
> when source QEMU resets and found changed FW during the precopy?

I think the FW will only change during migration (depends on the other
QEMU version) - but yeah, might be possible - no expert.

> 
>>
>>>
>>> And is this patch trying to fix/warn when there's a reboot during (3)
>>> so the new size is discovered at a wrong time?  Is my understanding
>>> correct?
>>
>> It's trying to bail out early instead of failing at other random points
>> (with an unclear outcome).
> 
> Yeah, I am just uncertain on whether in some cases it could be a
> silent success (when used_length changed, however migration still
> completed without error reported) and now we're changing it to a VM
> crash... Could that happen?
> 
>   - before the patch, when precopy triggers this,
> 
>     - when it didn't encounter issue with the changed used_length, it
>       could get silently ignored.  Lucky enough & good case.
> 
>     - when it triggered an error, precopy failed.  _However_, we can
>       simply restart... so still not so bad.
> 
>   - after the patch, when precopy detects this, we abort
>     immediately...  Which is really not good...

Se the other sub-thread (see below), we're thinking about canceling
pre-copy, which could work just fine.

> 
> If you see, that's the major thing I was worrying about...
> 
> And since used_length is growing in most cases as you said (at least
> before virtio-mem comes? :), I'm suspecting that could be the major

hah! :) The think about virtio-mem is that it can actually decide to not
resize during migration (and I have that implemented right now) - acpi
code can't.

> case that there will be a silent success.

The thing is, it might not be a silent success but a very strange
error/crash. We have a data race here. But yeah, I agree that we should
at least precopy not crashing.

>>>> In the precopy case it would be easier to abort (although, not simple
>>>> AFAIKS), in the postcopy not so easy - because you're already partially
>>>> running on the migration target.
>>>
>>> Prior to this patch, would a precopy still survive with such an
>>> accident (asked because I _feel_ like migrating a ramblock with
>>> smaller used_length to the same ramblock with bigger used_length seems
>>> to be fine?)?  Or we can stop the precopy and restart.  After this
>>
>> I assume growing the region is the usual case (not shrinking). FW blobs
>> tend to get bigger.
>>
>> Migrating while growing a ram block on the source won't work. The source
>> would try to send a dirt page that's outside of the used_length on the
>> target, making e.g., ram_load_postcopy()/ram_load_precopy() fail with
>> "Illegal RAM offset...".
> 
> Right.
> 
>>
>> In the postcopy case, e.g., ram_dirty_bitmap_reload() will fail in case
>> there is a mismatch between ram block size on source/target.
> 
> IMHO that's an extreme rare case when (one example I can think of):
> 
>   - we start a postcopy after a precopy
>   - system reset, noticed a firmware update
>   - we got a network failure, postcopy interrupted
>   - we try to recover a postcopy
> 
> So are you using postcopy recovery?  I will be surprised if so because
> then you'll be the first user I know that really used that besides QE. :)

One of my strengths is to read code and find flaws :P
Good to know that that should be "barely" affected for now :)

>> Another issue is if the used_length changes while in ram_save_setup(),
>> just between storing ram_bytes_total_common(true) and storing
>> block->used_length. A mismatch will screw up the migration stream.
> 
> Yes this seems to be another issue then.  IIUC the ramblocks are
> protected by RCU, the migration code has always been with the read
> lock there so logically it should see a consistent view of system
> ramblocks in ram_save_setup().  IMHO the thing that was inconsistent
> is that RCU is not safe enough for changing used_length for a ramblock.

Yes.

> 
>>
>> But these are just the immediately visible issues. I am more concerned
>> about used_length changing at random points in time, resulting in more
>> harm. (e.g., non-obvious load-store tearing when accessing the used length)
>>
>> Migration code is inherently racy when it comes to ram block resizes.
>> And that might become more dangerous once we want to size the migration
>> bitmaps smaller (used_length instead of max_length) or disallow access
>> to ram blocks beyond the used_length. Both are things I am working on :)
> 
> Right. Now I start to wonder whether migration is the only special guy
> here.  I noticed at least we've got:
> 
> struct RAMBlockNotifier {
>     void (*ram_block_added)(RAMBlockNotifier *n, void *host, size_t size);
>     void (*ram_block_removed)(RAMBlockNotifier *n, void *host, size_t size);
>     QLIST_ENTRY(RAMBlockNotifier) next;
> };
> 
> I suspect at least all these users could also break in some way if
> resize happens.

Hah! You should read

https://lore.kernel.org/qemu-devel/20200212134254.11073-1-david@redhat.com/

:)

VFIO is indeed broken on resizes - and fixed in that series (I assume
nobody migrates ...). HAX and SEV simply pin all memory and don't care
about any used_length changes. The callbacks were for now called with
max_length, which works but is not extensible.

See my suggestion in

https://lore.kernel.org/qemu-devel/bb33b209-2b15-4bbd-7fe9-3aa813e4c194@redhat.com/

which builds up on a ram resize notifier.

> 
>>
>>> patch, it'll crash the source VM (&error_abort specified in
>>> memory_region_ram_resize()), which seems a bit more harsh?
>>
>> There seems to be no easy way to abort migration from outside the
>> migration thread. As Juan said, you actually don't want to fail
>> migration but instead soft-abort migration and continue running the
>> guest on the target on a reset. But that's not easy as well.
>>
>> One could think about extending ram block notifiers to notify migration
>> code (before) resizes, so that migration code can work around the
>> resize (how is TBD). Not easy as well :)
> 
> True.  But if you see my worry still stands, on whether such a patch
> would make things worse by crashing it when it could still have a
> chance to survive.  Shall we loose the penalty of that even if we want
> to warn the user earlier?

Canceling migration in precopy case should be fine. Postcopy needs more
thought.

I certainly don't want to live with strange data races in migration code
because "it could work sometimes eventually".

Thanks for all the comments and thoughts!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

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 <david@redhat.com>
> Date: Fri, 14 Feb 2020 13:01:03 +0100
> Subject: [PATCH v1] tmp
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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? :)

> +
> +    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...

Thanks,

> +    }
> +}
> +
> +static RAMBlockNotifier ram_mig_ram_notifier = {
> +    .ram_block_resized = ram_mig_ram_block_resized,
> +};
> +
>  void ram_mig_init(void)
>  {
>      qemu_mutex_init(&XBZRLE.lock);
>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> +    ram_block_notifier_add(&ram_mig_ram_notifier);
>  }
> -- 
> 2.24.1
> 
> 
> Thoughts?
> 
> -- 
> Thanks,
> 
> David / dhildenb
> 
> 

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

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 <david@redhat.com>
>> Date: Fri, 14 Feb 2020 13:01:03 +0100
>> Subject: [PATCH v1] tmp
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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.

> 
>> +
>> +    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.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

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 <david@redhat.com>
> >> Date: Fri, 14 Feb 2020 13:01:03 +0100
> >> Subject: [PATCH v1] tmp
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  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()
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().

> 
> > 
> >> +
> >> +    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? :).  Or could you let me know where else did I overlook?

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

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 <david@redhat.com>
>>>> Date: Fri, 14 Feb 2020 13:01:03 +0100
>>>> Subject: [PATCH v1] tmp
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  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

[Peter Xu]

On Fri, Feb 14, 2020 at 07:26:59PM +0100, David Hildenbrand wrote:
> >>>> +    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 ...).

Yeah fencing is good, but crashing a VM while it wasn't going to crash
is another thing, imho.  You can dump an error message if you really
like, but instead of exit() I really prefer we either still let the
old way to at least work or really fix it.

When I say "really fix it", I mean we can even start to think about
the shrinking case and how to support that for postcopy.  For example,
in the above case host_from_ram_block_offset() will return NULL then,
and the fix could be that we drop that extra page because we don't
need that any more, instead of bailing out.

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

> Am 14.02.2020 um 20:45 schrieb Peter Xu <peterx@redhat.com>:
> 
> On Fri, Feb 14, 2020 at 07:26:59PM +0100, David Hildenbrand wrote:
>>>>>> +    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 ...).
> 
> Yeah fencing is good, but crashing a VM while it wasn't going to crash
> is another thing, imho.  You can dump an error message if you really
> like, but instead of exit() I really prefer we either still let the
> old way to at least work or really fix it.

I‘ll do whatever Juan/Dave think is best. I am not convinced that there is no way to corrupt data or crash later when the guest is already running again post-reboot and doing real work.

> 
> When I say "really fix it", I mean we can even start to think about
> the shrinking case and how to support that for postcopy.  For example,
> in the above case host_from_ram_block_offset() will return NULL then,
> and the fix could be that we drop that extra page because we don't
> need that any more, instead of bailing out.
> 

I have patches on the list that will make everything exceed used_length inaccessible. If there is still an access, we will crash. Printing a warning might help figure out what went wrong.

I have a patch lying around that allocates the bitmaps only for the used_length. Access outside of that (esp. receiced bitmap) will, well, depends, crash or mess up data. Printing an error might help to figure out what went wrong. Maybe.

Just FYI how I found this issue and why I want to sanitize the code. And we are trying to keep something alive here that never could have worked 100% reliably as it is inherently racy.

Cheers!

Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

On Fri, Feb 14, 2020 at 03:04:23PM -0500, David Hildenbrand wrote:
> 
> 
> > Am 14.02.2020 um 20:45 schrieb Peter Xu <peterx@redhat.com>:
> > 
> > On Fri, Feb 14, 2020 at 07:26:59PM +0100, David Hildenbrand wrote:
> >>>>>> +    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 ...).
> > 
> > Yeah fencing is good, but crashing a VM while it wasn't going to crash
> > is another thing, imho.  You can dump an error message if you really
> > like, but instead of exit() I really prefer we either still let the
> > old way to at least work or really fix it.
> 
> I‘ll do whatever Juan/Dave think is best. I am not convinced that there is no way to corrupt data or crash later when the guest is already running again post-reboot and doing real work.

Yeah I never said it will always work. :)

However it does not mean it'll break every time.  My guess is that for
the happened cases it might still survive quite a few, confessing that
is without much clue.  I just prefer to avoid having an explicit patch
to bail out like that, because it doesn't really help that much by
crashing earlier.

That's something I learnt when I started to work on migration, that
is, we don't call exit() on source VM when we really, really needed
to.  For postcopy, it's the destination VM that matters here.

Yeh not a big deal since this is really corner case even if it
happened.  Let's follow the maintainers' judgement.

Thanks,

-- 
Peter Xu