| Message ID | 20200226155304.60219-14-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migrate/ram: Fix resizing RAM blocks while migrating | expand |
On Wed, Feb 26, 2020 at 04:53: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 when we hit 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> Reviewed-by: Peter Xu <peterx@redhat.com>
On 26.02.20 17:06, Peter Xu wrote: > On Wed, Feb 26, 2020 at 04:53: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 when we hit 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> > > Reviewed-by: Peter Xu <peterx@redhat.com> > Thanks a lot! BTW, while I am playing with userfaultfd, I already have patches to factor out all uffd handling from postcopy code into utils/uffd.c My list of patches does not seem to get any smaller :(
On Wed, Feb 26, 2020 at 05:08:08PM +0100, David Hildenbrand wrote: > On 26.02.20 17:06, Peter Xu wrote: > > On Wed, Feb 26, 2020 at 04:53: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 when we hit 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> > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > > Thanks a lot! > > BTW, while I am playing with userfaultfd, I already have patches to > factor out all uffd handling from postcopy code into utils/uffd.c > > My list of patches does not seem to get any smaller :( Simply because you're working on more things? :) Thanks for working on this (and this is far better than the exit() version, IMHO)!
On 26.02.20 17:26, Peter Xu wrote: > On Wed, Feb 26, 2020 at 05:08:08PM +0100, David Hildenbrand wrote: >> On 26.02.20 17:06, Peter Xu wrote: >>> On Wed, Feb 26, 2020 at 04:53: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 when we hit 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> >>> >>> Reviewed-by: Peter Xu <peterx@redhat.com> >>> >> >> Thanks a lot! >> >> BTW, while I am playing with userfaultfd, I already have patches to >> factor out all uffd handling from postcopy code into utils/uffd.c >> >> My list of patches does not seem to get any smaller :( > > Simply because you're working on more things? :) virtio-mem has been a steady source of huge refactorings (both in QEMU and the kernel). At least on the kernel side, an end might be in sight :) > > Thanks for working on this (and this is far better than the exit() > version, IMHO)! Thanks for insisting to fix it instead of working around it!
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index c68caf4e42..f39c6304de 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -506,6 +506,14 @@ 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. There could only be + * an issue if we would suddenly encounter a mapping that's incompatible + * with UFFD - which cannot happen within a single RAM block. + */ if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) { error_report("%s: userfault unregister %s", __func__, strerror(errno)); @@ -1180,6 +1188,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 +1217,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());
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 when we hit 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 | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)