Message ID | 1434450415-11339-22-git-send-email-dgilbert@redhat.com |
---|---|
State | New |
Headers | show |
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Provide a check to see if the OS we're running on has all the bits > needed for postcopy. > > Creates postcopy-ram.c which will get most of the other helpers we need. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> I am guessing that test is ok, but we are doing the test each time that we change the function. We always end calling that kind of functions in several places. Shouldn't be good to rename the function to __postcopy_ram_supported_by_host() and do a toplevel function that is: bool postcopy_ram_supported_by_host(void) { static bool first_time = true; static supported = false; if (firt_time) { first_time = false; supported = __postcopy_ram_supported_by_host() } return supported; } Notice that I don't know how slow the mmap + usefault thing is, but I guess that the values would not change while running, no? It has a review-by because I don't see anything wrong with it.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Provide a check to see if the OS we're running on has all the bits > > needed for postcopy. > > > > Creates postcopy-ram.c which will get most of the other helpers we need. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > I am guessing that test is ok, but we are doing the test each time that > we change the function. We always end calling that kind of functions in > several places. Shouldn't be good to rename the function to > __postcopy_ram_supported_by_host() > > and do a toplevel function that is: > > bool postcopy_ram_supported_by_host(void) > { > static bool first_time = true; > static supported = false; > > if (firt_time) { > first_time = false; > supported = __postcopy_ram_supported_by_host() > } > return supported; > } > > Notice that I don't know how slow the mmap + usefault thing is, but I > guess that the values would not change while running, no? Since we only call this once, at the start of an incoming migration, it seems overkill to do that. Dave > > It has a review-by because I don't see anything wrong with it. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On (Tue) 16 Jun 2015 [11:26:34], Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Provide a check to see if the OS we're running on has all the bits > needed for postcopy. > > Creates postcopy-ram.c which will get most of the other helpers we need. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > @@ -1165,6 +1166,10 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis, > return -1; > } > > + if (!postcopy_ram_supported_by_host()) { > + return -1; > + } > + So this is just advise: if we receive this, and we can't handle postcopy, we're going to abort migration? Shouldn't we continue and let src know that we can't accept postcopy? ie the problem is currently punted to higher levels, should we try to handle it ourselves? Amit
* Amit Shah (amit.shah@redhat.com) wrote: > On (Tue) 16 Jun 2015 [11:26:34], Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Provide a check to see if the OS we're running on has all the bits > > needed for postcopy. > > > > Creates postcopy-ram.c which will get most of the other helpers we need. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > @@ -1165,6 +1166,10 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis, > > return -1; > > } > > > > + if (!postcopy_ram_supported_by_host()) { > > + return -1; > > + } > > + > > So this is just advise: if we receive this, and we can't handle > postcopy, we're going to abort migration? Shouldn't we continue and > let src know that we can't accept postcopy? ie the problem is > currently punted to higher levels, should we try to handle it > ourselves? We could, although it happens right at the start of migration, and it only happens if you explicitly enabled the postcopy capability, so I'm not sure there's any advantage in trying to fall back when you've been told to use it. Dave > > > > Amit -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On (Tue) 16 Jun 2015 [11:26:34], Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Provide a check to see if the OS we're running on has all the bits > needed for postcopy. > > Creates postcopy-ram.c which will get most of the other helpers we need. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Amit Shah <amit.shah@redhat.com> Amit
diff --git a/include/migration/postcopy-ram.h b/include/migration/postcopy-ram.h new file mode 100644 index 0000000..d81934f --- /dev/null +++ b/include/migration/postcopy-ram.h @@ -0,0 +1,19 @@ +/* + * Postcopy migration for RAM + * + * Copyright 2013 Red Hat, Inc. and/or its affiliates + * + * Authors: + * Dave Gilbert <dgilbert@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ +#ifndef QEMU_POSTCOPY_RAM_H +#define QEMU_POSTCOPY_RAM_H + +/* Return true if the host supports everything we need to do postcopy-ram */ +bool postcopy_ram_supported_by_host(void); + +#endif diff --git a/migration/Makefile.objs b/migration/Makefile.objs index d929e96..0cac6d7 100644 --- a/migration/Makefile.objs +++ b/migration/Makefile.objs @@ -1,7 +1,7 @@ common-obj-y += migration.o tcp.o common-obj-y += vmstate.o common-obj-y += qemu-file.o qemu-file-buf.o qemu-file-unix.o qemu-file-stdio.o -common-obj-y += xbzrle.o +common-obj-y += xbzrle.o postcopy-ram.o common-obj-$(CONFIG_RDMA) += rdma.o common-obj-$(CONFIG_POSIX) += exec.o unix.o fd.o diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c new file mode 100644 index 0000000..baf83f2 --- /dev/null +++ b/migration/postcopy-ram.c @@ -0,0 +1,158 @@ +/* + * Postcopy migration for RAM + * + * Copyright 2013-2015 Red Hat, Inc. and/or its affiliates + * + * Authors: + * Dave Gilbert <dgilbert@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +/* + * Postcopy is a migration technique where the execution flips from the + * source to the destination before all the data has been copied. + */ + +#include <glib.h> +#include <stdio.h> +#include <unistd.h> + +#include "qemu-common.h" +#include "migration/migration.h" +#include "migration/postcopy-ram.h" +#include "sysemu/sysemu.h" +#include "qemu/error-report.h" +#include "trace.h" + +/* Postcopy needs to detect accesses to pages that haven't yet been copied + * across, and efficiently map new pages in, the techniques for doing this + * are target OS specific. + */ +#if defined(__linux__) + +#include <sys/mman.h> +#include <sys/ioctl.h> +#include <sys/syscall.h> +#include <sys/types.h> +#include <asm/types.h> /* for __u64 */ +#endif + +#if defined(__linux__) && defined(__NR_userfaultfd) +#include <linux/userfaultfd.h> + +static bool ufd_version_check(int ufd) +{ + struct uffdio_api api_struct; + uint64_t ioctl_mask; + + api_struct.api = UFFD_API; + api_struct.features = 0; + if (ioctl(ufd, UFFDIO_API, &api_struct)) { + error_report("postcopy_ram_supported_by_host: UFFDIO_API failed: %s", + strerror(errno)); + return false; + } + + ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | + (__u64)1 << _UFFDIO_UNREGISTER; + if ((api_struct.ioctls & ioctl_mask) != ioctl_mask) { + error_report("Missing userfault features: %" PRIx64, + (uint64_t)(~api_struct.ioctls & ioctl_mask)); + return false; + } + + return true; +} + +bool postcopy_ram_supported_by_host(void) +{ + long pagesize = getpagesize(); + int ufd = -1; + bool ret = false; /* Error unless we change it */ + void *testarea = NULL; + struct uffdio_register reg_struct; + struct uffdio_range range_struct; + uint64_t feature_mask; + + if ((1ul << qemu_target_page_bits()) > pagesize) { + error_report("Target page size bigger than host page size"); + goto out; + } + + ufd = syscall(__NR_userfaultfd, O_CLOEXEC); + if (ufd == -1) { + error_report("%s: userfaultfd not available: %s", __func__, + strerror(errno)); + goto out; + } + + /* Version and features check */ + if (!ufd_version_check(ufd)) { + goto out; + } + + /* + * We need to check that the ops we need are supported on anon memory + * To do that we need to register a chunk and see the flags that + * are returned. + */ + testarea = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE | + MAP_ANONYMOUS, -1, 0); + if (testarea == MAP_FAILED) { + error_report("%s: Failed to map test area: %s", __func__, + strerror(errno)); + goto out; + } + g_assert(((size_t)testarea & (pagesize-1)) == 0); + + reg_struct.range.start = (uintptr_t)testarea; + reg_struct.range.len = pagesize; + reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING; + + if (ioctl(ufd, UFFDIO_REGISTER, ®_struct)) { + error_report("%s userfault register: %s", __func__, strerror(errno)); + goto out; + } + + range_struct.start = (uintptr_t)testarea; + range_struct.len = pagesize; + if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) { + error_report("%s userfault unregister: %s", __func__, strerror(errno)); + goto out; + } + + feature_mask = (__u64)1 << _UFFDIO_WAKE | + (__u64)1 << _UFFDIO_COPY | + (__u64)1 << _UFFDIO_ZEROPAGE; + if ((reg_struct.ioctls & feature_mask) != feature_mask) { + error_report("Missing userfault map features: %" PRIx64, + (uint64_t)(~reg_struct.ioctls & feature_mask)); + goto out; + } + + /* Success! */ + ret = true; +out: + if (testarea) { + munmap(testarea, pagesize); + } + if (ufd != -1) { + close(ufd); + } + return ret; +} + +#else +/* No target OS support, stubs just fail */ + +bool postcopy_ram_supported_by_host(void) +{ + error_report("%s: No OS support", __func__); + return false; +} + +#endif + diff --git a/migration/savevm.c b/migration/savevm.c index ebd3d31..f324c6e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -37,6 +37,7 @@ #include "qemu/timer.h" #include "audio/audio.h" #include "migration/migration.h" +#include "migration/postcopy-ram.h" #include "qemu/sockets.h" #include "qemu/queue.h" #include "sysemu/cpus.h" @@ -1165,6 +1166,10 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis, return -1; } + if (!postcopy_ram_supported_by_host()) { + return -1; + } + if (remote_hps != getpagesize()) { /* * Some combinations of mismatch are probably possible but it gets