On Fri, Apr 21, 2017 at 11:24:54AM +0100, Dr. David Alan Gilbert wrote: > * Alexey Perevalov (a.pereva...@samsung.com) wrote: > > Userfaultfd mechanism is able to provide process thread id, > > in case when client request it with UFDD_API ioctl. > > > > Signed-off-by: Alexey Perevalov <a.pereva...@samsung.com> > > There seem to be two parts to this: > a) Adding the mis parameter to ufd_version_check > b) Asking for the feature > > Please split it into two patches. > > Also.... > > > --- > > include/migration/postcopy-ram.h | 2 +- > > migration/migration.c | 2 +- > > migration/postcopy-ram.c | 12 ++++++------ > > migration/savevm.c | 2 +- > > 4 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/include/migration/postcopy-ram.h > > b/include/migration/postcopy-ram.h > > index 8e036b9..809f6db 100644 > > --- a/include/migration/postcopy-ram.h > > +++ b/include/migration/postcopy-ram.h > > @@ -14,7 +14,7 @@ > > #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); > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis); > > > > /* > > * Make all of RAM sensitive to accesses to areas that haven't yet been > > written > > diff --git a/migration/migration.c b/migration/migration.c > > index ad4036f..79f6425 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -802,7 +802,7 @@ void > > qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, > > * special support. > > */ > > if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) && > > - !postcopy_ram_supported_by_host()) { > > + !postcopy_ram_supported_by_host(NULL)) { > > /* postcopy_ram_supported_by_host will have emitted a more > > * detailed message > > */ > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > > index dc80dbb..70f0480 100644 > > --- a/migration/postcopy-ram.c > > +++ b/migration/postcopy-ram.c > > @@ -60,13 +60,13 @@ struct PostcopyDiscardState { > > #include <sys/eventfd.h> > > #include <linux/userfaultfd.h> > > > > -static bool ufd_version_check(int ufd) > > +static bool ufd_version_check(int ufd, MigrationIncomingState *mis) > > { > > struct uffdio_api api_struct; > > uint64_t ioctl_mask; > > > > api_struct.api = UFFD_API; > > - api_struct.features = 0; > > + api_struct.features = UFFD_FEATURE_THREAD_ID; > > if (ioctl(ufd, UFFDIO_API, &api_struct)) { > > error_report("postcopy_ram_supported_by_host: UFFDIO_API failed: > > %s", > > strerror(errno)); > > You're not actually using the 'mis' here - what I'd expected was > something that was going to check if the UFFDIO_API return said that it really > had the feature, and if so store a flag in the MIS somewhere. > > Also, I'm not sure it's right to set 'api_struct.features' on the input - what > happens if this is run on an old kernel - we don't want postcopy to fail on > an old kernel without your feature. > I'm not 100% sure of the interface, but I think the way it works is you set > features = 0 before the call, and then check the api_struct.features in the > return - in the same way that I check for UFFD_FEATURE_MISSING_HUGETLBFS. > We need to ask kernel about that feature, right, kernel returns back available features uffdio_api.features = UFFD_API_FEATURES but it also stores requested features /* only enable the requested features for this uffd context */ ctx->features = uffd_ctx_features(features);
so, at the time when process thread id is going to be sent kernel checks if it was requested + if (features & UFFD_FEATURE_THREAD_ID) + msg.arg.pagefault.ptid = task_pid_vnr(current); from patch message: Process's thread id is being provided when user requeste it by setting UFFD_FEATURE_THREAD_ID bit into uffdio_api.features. UFFD_FEATURE_MISSING_HUGETLBFS - look like default, unconditional behavior (I didn't find any usage of that define in kernel). > Dave > > > @@ -113,7 +113,7 @@ static int test_range_shared(const char *block_name, > > void *host_addr, > > * normally fine since if the postcopy succeeds it gets turned back on at > > the > > * end. > > */ > > -bool postcopy_ram_supported_by_host(void) > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis) > > { > > long pagesize = getpagesize(); > > int ufd = -1; > > @@ -136,7 +136,7 @@ bool postcopy_ram_supported_by_host(void) > > } > > > > /* Version and features check */ > > - if (!ufd_version_check(ufd)) { > > + if (!ufd_version_check(ufd, mis)) { > > goto out; > > } > > > > @@ -515,7 +515,7 @@ int postcopy_ram_enable_notify(MigrationIncomingState > > *mis) > > * Although the host check already tested the API, we need to > > * do the check again as an ABI handshake on the new fd. > > */ > > - if (!ufd_version_check(mis->userfault_fd)) { > > + if (!ufd_version_check(mis->userfault_fd, mis)) { > > return -1; > > } > > > > @@ -653,7 +653,7 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis) > > > > #else > > /* No target OS support, stubs just fail */ > > -bool postcopy_ram_supported_by_host(void) > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis) > > { > > error_report("%s: No OS support", __func__); > > return false; > > diff --git a/migration/savevm.c b/migration/savevm.c > > index 3b19a4a..f01e418 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -1360,7 +1360,7 @@ static int > > loadvm_postcopy_handle_advise(MigrationIncomingState *mis) > > return -1; > > } > > > > - if (!postcopy_ram_supported_by_host()) { > > + if (!postcopy_ram_supported_by_host(mis)) { > > postcopy_state_set(POSTCOPY_INCOMING_NONE); > > return -1; > > } > > -- > > 1.8.3.1 > > > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- BR Alexey