[v2,13/13] migration/ram: Tolerate partially changed mappings in postcopy code
diff mbox series

Message ID 20200221164204.105570-14-david@redhat.com
State New
Headers show
Series
  • migrate/ram: Fix resizing RAM blocks while migrating
Related show

Commit Message

David Hildenbrand Feb. 21, 2020, 4:42 p.m. UTC
When we partially change mappings (esp., mmap over parts of an existing
mmap like qemu_ram_remap() does) where we have a userfaultfd handler
registered, the handler will implicitly be unregistered from the parts that
changed.

Trying to place pages onto mappings where there is no longer a handler
registered will fail. Let's make sure that any waiter is woken up - we
have to do that manually.

Let's also document how UFFDIO_UNREGISTER will handle this scenario.

This is mainly a preparation for RAM blocks with resizable allcoations,
where the mapping of the invalid RAM range will change. The source will
keep sending pages that are outside of the new (shrunk) RAM size. We have
to treat these pages like they would have been migrated, but can
essentially simply drop the content (ignore the placement error).

Keep printing a warning on EINVAL, to avoid hiding other (programming)
issues. ENOENT is unique.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/postcopy-ram.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Peter Xu Feb. 24, 2020, 10:49 p.m. UTC | #1
On Fri, Feb 21, 2020 at 05:42:04PM +0100, David Hildenbrand wrote:
> When we partially change mappings (esp., mmap over parts of an existing
> mmap like qemu_ram_remap() does) where we have a userfaultfd handler
> registered, the handler will implicitly be unregistered from the parts that
> changed.
> 
> Trying to place pages onto mappings where there is no longer a handler
> registered will fail. Let's make sure that any waiter is woken up - we
> have to do that manually.
> 
> Let's also document how UFFDIO_UNREGISTER will handle this scenario.
> 
> This is mainly a preparation for RAM blocks with resizable allcoations,
> where the mapping of the invalid RAM range will change. The source will
> keep sending pages that are outside of the new (shrunk) RAM size. We have
> to treat these pages like they would have been migrated, but can
> essentially simply drop the content (ignore the placement error).
> 
> Keep printing a warning on EINVAL, to avoid hiding other (programming)
> issues. ENOENT is unique.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  migration/postcopy-ram.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index c68caf4e42..f023830b9a 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -506,6 +506,12 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>      range_struct.start = (uintptr_t)host_addr;
>      range_struct.len = length;
>  
> +    /*
> +     * In case the mapping was partially changed since we enabled userfault
> +     * (e.g., via qemu_ram_remap()), the userfaultfd handler was already removed
> +     * for the mappings that changed. Unregistering will, however, still work
> +     * and ignore mappings without a registered handler.
> +     */

Ideally we should still only unregister what we have registered.
After all we do have this information because we know what we
registered, we know what has unmapped (in your new resize() hook, when
postcopy_state==RUNNING).

An extreme example is when we register with pages in range [A, B),
then shrink it to [A, C), then we mapped something else within [C, B)
(note, with virtio-mem logically B can be very big and C can be very
small, it means [B, C) can cover quite some address space). Then if:

  - [C, B) memory type is not compatible with uffd, or

  - [C, B) could be registered with uffd again due to some other
    reason (so far QEMU should not have such a reason)

Then the unregister could fail or misbehave, IMHO.  Another benefit is
that...

>      if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
>          error_report("%s: userfault unregister %s", __func__, strerror(errno));
>  
> @@ -1180,6 +1186,17 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
>      return 0;
>  }
>  
> +static int qemu_ufd_wake_ioctl(int userfault_fd, void *host_addr,
> +                               uint64_t pagesize)
> +{
> +    struct uffdio_range range = {
> +        .start = (uint64_t)(uintptr_t)host_addr,
> +        .len = pagesize,
> +    };
> +
> +    return ioctl(userfault_fd, UFFDIO_WAKE, &range);
> +}
> +
>  static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
>                                 void *from_addr, uint64_t pagesize, RAMBlock *rb)
>  {
> @@ -1198,6 +1215,26 @@ static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
>          zero_struct.mode = 0;
>          ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
>      }
> +
> +    /*
> +     * When the mapping gets partially changed (e.g., qemu_ram_remap()) before
> +     * we try to place a page, the userfaultfd handler will be removed for the
> +     * changed mappings and placing pages will fail. We can safely ignore this,
> +     * because mappings that changed on the destination don't need data from the
> +     * source (e.g., qemu_ram_remap()). Wake up any waiter waiting for that page
> +     * (unlikely but possible). Waking up waiters is always possible, even
> +     * without a registered userfaultfd handler.
> +     *
> +     * Old kernels report EINVAL, new kernels report ENOENT in case there is
> +     * no longer a userfaultfd handler for a mapping.
> +     */
> +    if (ret && (errno == ENOENT || errno == EINVAL)) {
> +        if (errno == EINVAL) {
> +            warn_report("%s: Failed to place page %p. Waking up any waiters.",
> +                         __func__, host_addr);
> +        }
> +        ret = qemu_ufd_wake_ioctl(userfault_fd, host_addr, pagesize);

... if with above information (takes notes on where we registered
uffd), I think we don't need to capture error, but we can simply skip
those outliers.

Thanks,

> +    }
>      if (!ret) {
>          ramblock_recv_bitmap_set_range(rb, host_addr,
>                                         pagesize / qemu_target_page_size());
> -- 
> 2.24.1
>
David Hildenbrand Feb. 25, 2020, 7:44 a.m. UTC | #2
On 24.02.20 23:49, Peter Xu wrote:
> On Fri, Feb 21, 2020 at 05:42:04PM +0100, David Hildenbrand wrote:
>> When we partially change mappings (esp., mmap over parts of an existing
>> mmap like qemu_ram_remap() does) where we have a userfaultfd handler
>> registered, the handler will implicitly be unregistered from the parts that
>> changed.
>>
>> Trying to place pages onto mappings where there is no longer a handler
>> registered will fail. Let's make sure that any waiter is woken up - we
>> have to do that manually.
>>
>> Let's also document how UFFDIO_UNREGISTER will handle this scenario.
>>
>> This is mainly a preparation for RAM blocks with resizable allcoations,
>> where the mapping of the invalid RAM range will change. The source will
>> keep sending pages that are outside of the new (shrunk) RAM size. We have
>> to treat these pages like they would have been migrated, but can
>> essentially simply drop the content (ignore the placement error).
>>
>> Keep printing a warning on EINVAL, to avoid hiding other (programming)
>> issues. ENOENT is unique.
>>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  migration/postcopy-ram.c | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index c68caf4e42..f023830b9a 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -506,6 +506,12 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>>      range_struct.start = (uintptr_t)host_addr;
>>      range_struct.len = length;
>>  
>> +    /*
>> +     * In case the mapping was partially changed since we enabled userfault
>> +     * (e.g., via qemu_ram_remap()), the userfaultfd handler was already removed
>> +     * for the mappings that changed. Unregistering will, however, still work
>> +     * and ignore mappings without a registered handler.
>> +     */
> 
> Ideally we should still only unregister what we have registered.
> After all we do have this information because we know what we
> registered, we know what has unmapped (in your new resize() hook, when
> postcopy_state==RUNNING).

Not in the case of qemu_ram_remap(). And whatever you propose will
require synchronization (see my other mail) and more complicated
handling than this. uffd allows you to handle races with mmap changes in
a very elegant way (e.g., -ENOENT, or unregisterignoring changed mappings).

> 
> An extreme example is when we register with pages in range [A, B),
> then shrink it to [A, C), then we mapped something else within [C, B)
> (note, with virtio-mem logically B can be very big and C can be very
> small, it means [B, C) can cover quite some address space). Then if:
> 
>   - [C, B) memory type is not compatible with uffd, or

That will never happen in the near future. Without resizable allocations:
- All memory is either anonymous or from a single fd

In addition, right now, only anonymous memory can be used for resizable
RAM. However, with resizable allocations we could have:
- All used_length memory is either anonymous or from a single fd
- All remaining memory is either anonymous or from a single fd

Everything else does not make any sense IMHO and I don't think this is
relevant long term. You cannot arbitrarily map things into the
used_length part of a RAMBlock. That would contradict to its page_size
and its fd. E.g., you would break qemu_ram_remap().

> 
>   - [C, B) could be registered with uffd again due to some other
>     reason (so far QEMU should not have such a reason)

Any code that wants to make use of uffd properly has to synchronize
against postcopy code either way IMHO. It just doesn't work otherwise.

E.g., once I would use it to protect unplugged memory in virtio-mem
(something I am looking into right now and teaching QEMU not to touch
all RAMBlock memory is complicated :) ), virtio-mem would unregister any
uffd handler when notified that postcopy will start, and re-register
after postcopy finished.

[...]


>>  
>> +static int qemu_ufd_wake_ioctl(int userfault_fd, void *host_addr,
>> +                               uint64_t pagesize)
>> +{
>> +    struct uffdio_range range = {
>> +        .start = (uint64_t)(uintptr_t)host_addr,
>> +        .len = pagesize,
>> +    };
>> +
>> +    return ioctl(userfault_fd, UFFDIO_WAKE, &range);
>> +}
>> +
>>  static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
>>                                 void *from_addr, uint64_t pagesize, RAMBlock *rb)
>>  {
>> @@ -1198,6 +1215,26 @@ static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
>>          zero_struct.mode = 0;
>>          ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
>>      }
>> +
>> +    /*
>> +     * When the mapping gets partially changed (e.g., qemu_ram_remap()) before
>> +     * we try to place a page, the userfaultfd handler will be removed for the
>> +     * changed mappings and placing pages will fail. We can safely ignore this,
>> +     * because mappings that changed on the destination don't need data from the
>> +     * source (e.g., qemu_ram_remap()). Wake up any waiter waiting for that page
>> +     * (unlikely but possible). Waking up waiters is always possible, even
>> +     * without a registered userfaultfd handler.
>> +     *
>> +     * Old kernels report EINVAL, new kernels report ENOENT in case there is
>> +     * no longer a userfaultfd handler for a mapping.
>> +     */
>> +    if (ret && (errno == ENOENT || errno == EINVAL)) {
>> +        if (errno == EINVAL) {
>> +            warn_report("%s: Failed to place page %p. Waking up any waiters.",
>> +                         __func__, host_addr);
>> +        }
>> +        ret = qemu_ufd_wake_ioctl(userfault_fd, host_addr, pagesize);
> 
> ... if with above information (takes notes on where we registered
> uffd), I think we don't need to capture error, but we can simply skip
> those outliers.

I think we could skip them in general. Nobody should be touching that
memory. But debugging e.g., a SEGFAULT is easier than debugging some
sleeping thread IMHO.
Peter Xu Feb. 25, 2020, 2:27 p.m. UTC | #3
On Tue, Feb 25, 2020 at 08:44:56AM +0100, David Hildenbrand wrote:
> On 24.02.20 23:49, Peter Xu wrote:
> > On Fri, Feb 21, 2020 at 05:42:04PM +0100, David Hildenbrand wrote:
> >> When we partially change mappings (esp., mmap over parts of an existing
> >> mmap like qemu_ram_remap() does) where we have a userfaultfd handler
> >> registered, the handler will implicitly be unregistered from the parts that
> >> changed.
> >>
> >> Trying to place pages onto mappings where there is no longer a handler
> >> registered will fail. Let's make sure that any waiter is woken up - we
> >> have to do that manually.
> >>
> >> Let's also document how UFFDIO_UNREGISTER will handle this scenario.
> >>
> >> This is mainly a preparation for RAM blocks with resizable allcoations,
> >> where the mapping of the invalid RAM range will change. The source will
> >> keep sending pages that are outside of the new (shrunk) RAM size. We have
> >> to treat these pages like they would have been migrated, but can
> >> essentially simply drop the content (ignore the placement error).
> >>
> >> Keep printing a warning on EINVAL, to avoid hiding other (programming)
> >> issues. ENOENT is unique.
> >>
> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Andrea Arcangeli <aarcange@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  migration/postcopy-ram.c | 37 +++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 37 insertions(+)
> >>
> >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> >> index c68caf4e42..f023830b9a 100644
> >> --- a/migration/postcopy-ram.c
> >> +++ b/migration/postcopy-ram.c
> >> @@ -506,6 +506,12 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> >>      range_struct.start = (uintptr_t)host_addr;
> >>      range_struct.len = length;
> >>  
> >> +    /*
> >> +     * In case the mapping was partially changed since we enabled userfault
> >> +     * (e.g., via qemu_ram_remap()), the userfaultfd handler was already removed
> >> +     * for the mappings that changed. Unregistering will, however, still work
> >> +     * and ignore mappings without a registered handler.
> >> +     */
> > 
> > Ideally we should still only unregister what we have registered.
> > After all we do have this information because we know what we
> > registered, we know what has unmapped (in your new resize() hook, when
> > postcopy_state==RUNNING).
> 
> Not in the case of qemu_ram_remap(). And whatever you propose will
> require synchronization (see my other mail) and more complicated
> handling than this. uffd allows you to handle races with mmap changes in
> a very elegant way (e.g., -ENOENT, or unregisterignoring changed mappings).

All writers to the new postcopy_min_length should have BQL already.
The only left is the last cleanup_range() where we can take the BQL
for a while.  However...

> 
> > 
> > An extreme example is when we register with pages in range [A, B),
> > then shrink it to [A, C), then we mapped something else within [C, B)
> > (note, with virtio-mem logically B can be very big and C can be very
> > small, it means [B, C) can cover quite some address space). Then if:
> > 
> >   - [C, B) memory type is not compatible with uffd, or
> 
> That will never happen in the near future. Without resizable allocations:
> - All memory is either anonymous or from a single fd
> 
> In addition, right now, only anonymous memory can be used for resizable
> RAM. However, with resizable allocations we could have:
> - All used_length memory is either anonymous or from a single fd
> - All remaining memory is either anonymous or from a single fd
> 
> Everything else does not make any sense IMHO and I don't think this is
> relevant long term. You cannot arbitrarily map things into the
> used_length part of a RAMBlock. That would contradict to its page_size
> and its fd. E.g., you would break qemu_ram_remap().

... I think this persuaded me. :) You are right they can still be
protected until max_length with PROT_NONE.  Would you mind add some of
the above into the comment above unregister of uffd?

Thanks,
Peter Xu Feb. 25, 2020, 3:37 p.m. UTC | #4
On Tue, Feb 25, 2020 at 09:27:40AM -0500, Peter Xu wrote:
> On Tue, Feb 25, 2020 at 08:44:56AM +0100, David Hildenbrand wrote:
> > On 24.02.20 23:49, Peter Xu wrote:
> > > On Fri, Feb 21, 2020 at 05:42:04PM +0100, David Hildenbrand wrote:
> > >> When we partially change mappings (esp., mmap over parts of an existing
> > >> mmap like qemu_ram_remap() does) where we have a userfaultfd handler
> > >> registered, the handler will implicitly be unregistered from the parts that
> > >> changed.
> > >>
> > >> Trying to place pages onto mappings where there is no longer a handler
> > >> registered will fail. Let's make sure that any waiter is woken up - we
> > >> have to do that manually.
> > >>
> > >> Let's also document how UFFDIO_UNREGISTER will handle this scenario.
> > >>
> > >> This is mainly a preparation for RAM blocks with resizable allcoations,
> > >> where the mapping of the invalid RAM range will change. The source will
> > >> keep sending pages that are outside of the new (shrunk) RAM size. We have
> > >> to treat these pages like they would have been migrated, but can
> > >> essentially simply drop the content (ignore the placement error).
> > >>
> > >> Keep printing a warning on EINVAL, to avoid hiding other (programming)
> > >> issues. ENOENT is unique.
> > >>
> > >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >> Cc: Juan Quintela <quintela@redhat.com>
> > >> Cc: Peter Xu <peterx@redhat.com>
> > >> Cc: Andrea Arcangeli <aarcange@redhat.com>
> > >> Signed-off-by: David Hildenbrand <david@redhat.com>
> > >> ---
> > >>  migration/postcopy-ram.c | 37 +++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 37 insertions(+)
> > >>
> > >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > >> index c68caf4e42..f023830b9a 100644
> > >> --- a/migration/postcopy-ram.c
> > >> +++ b/migration/postcopy-ram.c
> > >> @@ -506,6 +506,12 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> > >>      range_struct.start = (uintptr_t)host_addr;
> > >>      range_struct.len = length;
> > >>  
> > >> +    /*
> > >> +     * In case the mapping was partially changed since we enabled userfault
> > >> +     * (e.g., via qemu_ram_remap()), the userfaultfd handler was already removed
> > >> +     * for the mappings that changed. Unregistering will, however, still work
> > >> +     * and ignore mappings without a registered handler.
> > >> +     */
> > > 
> > > Ideally we should still only unregister what we have registered.
> > > After all we do have this information because we know what we
> > > registered, we know what has unmapped (in your new resize() hook, when
> > > postcopy_state==RUNNING).
> > 
> > Not in the case of qemu_ram_remap(). And whatever you propose will
> > require synchronization (see my other mail) and more complicated
> > handling than this. uffd allows you to handle races with mmap changes in
> > a very elegant way (e.g., -ENOENT, or unregisterignoring changed mappings).
> 
> All writers to the new postcopy_min_length should have BQL already.
> The only left is the last cleanup_range() where we can take the BQL
> for a while.  However...
> 
> > 
> > > 
> > > An extreme example is when we register with pages in range [A, B),
> > > then shrink it to [A, C), then we mapped something else within [C, B)
> > > (note, with virtio-mem logically B can be very big and C can be very
> > > small, it means [B, C) can cover quite some address space). Then if:
> > > 
> > >   - [C, B) memory type is not compatible with uffd, or
> > 
> > That will never happen in the near future. Without resizable allocations:
> > - All memory is either anonymous or from a single fd
> > 
> > In addition, right now, only anonymous memory can be used for resizable
> > RAM. However, with resizable allocations we could have:
> > - All used_length memory is either anonymous or from a single fd
> > - All remaining memory is either anonymous or from a single fd
> > 
> > Everything else does not make any sense IMHO and I don't think this is
> > relevant long term. You cannot arbitrarily map things into the
> > used_length part of a RAMBlock. That would contradict to its page_size
> > and its fd. E.g., you would break qemu_ram_remap().
> 
> ... I think this persuaded me. :) You are right they can still be
> protected until max_length with PROT_NONE.  Would you mind add some of
> the above into the comment above unregister of uffd?

Sorry please ignore the PROT_NONE part.  Maybe just mention something
like "the used_length will either be or larger than the userfaultfd
registered range (with the same memory type), so it will be safe to
unregister for even bigger than what we have registered".  Thanks,

Patch
diff mbox series

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index c68caf4e42..f023830b9a 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -506,6 +506,12 @@  static int cleanup_range(RAMBlock *rb, void *opaque)
     range_struct.start = (uintptr_t)host_addr;
     range_struct.len = length;
 
+    /*
+     * In case the mapping was partially changed since we enabled userfault
+     * (e.g., via qemu_ram_remap()), the userfaultfd handler was already removed
+     * for the mappings that changed. Unregistering will, however, still work
+     * and ignore mappings without a registered handler.
+     */
     if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
         error_report("%s: userfault unregister %s", __func__, strerror(errno));
 
@@ -1180,6 +1186,17 @@  int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
     return 0;
 }
 
+static int qemu_ufd_wake_ioctl(int userfault_fd, void *host_addr,
+                               uint64_t pagesize)
+{
+    struct uffdio_range range = {
+        .start = (uint64_t)(uintptr_t)host_addr,
+        .len = pagesize,
+    };
+
+    return ioctl(userfault_fd, UFFDIO_WAKE, &range);
+}
+
 static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
                                void *from_addr, uint64_t pagesize, RAMBlock *rb)
 {
@@ -1198,6 +1215,26 @@  static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
         zero_struct.mode = 0;
         ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
     }
+
+    /*
+     * When the mapping gets partially changed (e.g., qemu_ram_remap()) before
+     * we try to place a page, the userfaultfd handler will be removed for the
+     * changed mappings and placing pages will fail. We can safely ignore this,
+     * because mappings that changed on the destination don't need data from the
+     * source (e.g., qemu_ram_remap()). Wake up any waiter waiting for that page
+     * (unlikely but possible). Waking up waiters is always possible, even
+     * without a registered userfaultfd handler.
+     *
+     * Old kernels report EINVAL, new kernels report ENOENT in case there is
+     * no longer a userfaultfd handler for a mapping.
+     */
+    if (ret && (errno == ENOENT || errno == EINVAL)) {
+        if (errno == EINVAL) {
+            warn_report("%s: Failed to place page %p. Waking up any waiters.",
+                         __func__, host_addr);
+        }
+        ret = qemu_ufd_wake_ioctl(userfault_fd, host_addr, pagesize);
+    }
     if (!ret) {
         ramblock_recv_bitmap_set_range(rb, host_addr,
                                        pagesize / qemu_target_page_size());