| Message ID | 1495539071-12995-5-git-send-email-a.perevalov@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 23, 2017 at 02:31:05PM +0300, Alexey Perevalov wrote: > This modification is necessary for userfault fd features which are > required to be requested from userspace. > UFFD_FEATURE_THREAD_ID is a one of such "on demand" feature, which will > be introduced in the next patch. > > QEMU have to use separate userfault file descriptor, due to > userfault context has internal state, and after first call of > ioctl UFFD_API it changes its state to UFFD_STATE_RUNNING (in case of > success), but kernel while handling ioctl UFFD_API expects UFFD_STATE_WAIT_API. > So only one ioctl with UFFD_API is possible per ufd. > > Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> Hi, Alexey, Mostly good to me, some nitpicks below. > --- > migration/postcopy-ram.c | 100 ++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 91 insertions(+), 9 deletions(-) > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index 3ed78bf..4f3f495 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -59,32 +59,114 @@ struct PostcopyDiscardState { > #include <sys/eventfd.h> > #include <linux/userfaultfd.h> > > -static bool ufd_version_check(int ufd, MigrationIncomingState *mis) > + > +/** > + * receive_ufd_features: check userfault fd features, to request only supported > + * features in the future. > + * > + * Returns: true on success > + * > + * __NR_userfaultfd - should be checked before I don't see this line necessary. After all we will detect the error no matter what... > + * @features: out parameter will contain uffdio_api.features provided by kernel > + * in case of success > + */ > +static bool receive_ufd_features(uint64_t *features) > { > - struct uffdio_api api_struct; > - uint64_t ioctl_mask; > + struct uffdio_api api_struct = {0}; > + int ufd; > + bool ret = true; > + > + /* if we are here __NR_userfaultfd should exists */ > + ufd = syscall(__NR_userfaultfd, O_CLOEXEC); > + if (ufd == -1) { > + error_report("%s: syscall __NR_userfaultfd failed: %s", __func__, > + strerror(errno)); > + return false; > + } > > + /* ask features */ > api_struct.api = UFFD_API; > api_struct.features = 0; > if (ioctl(ufd, UFFDIO_API, &api_struct)) { > - error_report("%s: UFFDIO_API failed: %s", __func__ > + error_report("%s: UFFDIO_API failed: %s", __func__, > strerror(errno)); > + ret = false; > + goto release_ufd; > + } > + > + *features = api_struct.features; > + > +release_ufd: > + close(ufd); > + return ret; > +} > + > +/** > + * request_ufd_features: this function should be called only once on a newly > + * opened ufd, subsequent calls will lead to error. > + * > + * Returns: true on succes > + * > + * @ufd: fd obtained from userfaultfd syscall > + * @features: bit mask see UFFD_API_FEATURES > + */ > +static bool request_ufd_features(int ufd, uint64_t features) > +{ > + struct uffdio_api api_struct = {0}; > + uint64_t ioctl_mask; > + > + api_struct.api = UFFD_API; > + api_struct.features = features; > + if (ioctl(ufd, UFFDIO_API, &api_struct)) { > + error_report("%s failed: UFFDIO_API failed: %s", __func__, > + strerror(errno)); Maybe we can indent this line to follow this file's rule? error_report("%s failed: UFFDIO_API failed: %s", __func__, strerror(errno)); > return false; > } > > - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | > - (__u64)1 << _UFFDIO_UNREGISTER; > + ioctl_mask = 1 << _UFFDIO_REGISTER | > + 1 << _UFFDIO_UNREGISTER; Could I ask why we explicitly removed (__u64) here? Since I see the old one better. > 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; > +} > + > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis) > +{ > + uint64_t asked_features = 0; > + static uint64_t supported_features; > + > + /* > + * it's not possible to > + * request UFFD_API twice per one fd > + * userfault fd features is persistent > + */ > + if (!supported_features) { I would prefer not having this static variable. After all, this function call is rare, and the receive_ufd_features() is not that slow as well. > + if (!receive_ufd_features(&supported_features)) { > + error_report("%s failed", __func__); > + return false; > + } > + } > + > + /* > + * request features, even if asked_features is 0, due to > + * kernel expects UFFD_API before UFFDIO_REGISTER, per > + * userfault file descriptor > + */ > + if (!request_ufd_features(ufd, asked_features)) { > + error_report("%s failed: features %" PRIu64, __func__, > + asked_features); Better indent? Thanks,
Hi, Peter, On Wed, May 24, 2017 at 10:36:29AM +0800, Peter Xu wrote: > On Tue, May 23, 2017 at 02:31:05PM +0300, Alexey Perevalov wrote: > > This modification is necessary for userfault fd features which are > > required to be requested from userspace. > > UFFD_FEATURE_THREAD_ID is a one of such "on demand" feature, which will > > be introduced in the next patch. > > > > QEMU have to use separate userfault file descriptor, due to > > userfault context has internal state, and after first call of > > ioctl UFFD_API it changes its state to UFFD_STATE_RUNNING (in case of > > success), but kernel while handling ioctl UFFD_API expects UFFD_STATE_WAIT_API. > > So only one ioctl with UFFD_API is possible per ufd. > > > > Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> > > Hi, Alexey, > > Mostly good to me, some nitpicks below. > > > --- > > migration/postcopy-ram.c | 100 ++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 91 insertions(+), 9 deletions(-) > > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > > index 3ed78bf..4f3f495 100644 > > --- a/migration/postcopy-ram.c > > +++ b/migration/postcopy-ram.c > > @@ -59,32 +59,114 @@ struct PostcopyDiscardState { > > #include <sys/eventfd.h> > > #include <linux/userfaultfd.h> > > > > -static bool ufd_version_check(int ufd, MigrationIncomingState *mis) > > + > > +/** > > + * receive_ufd_features: check userfault fd features, to request only supported > > + * features in the future. > > + * > > + * Returns: true on success > > + * > > + * __NR_userfaultfd - should be checked before > > I don't see this line necessary. After all we will detect the error no > matter what... Yes, because in this function it has a check already, but that check isn't odd. So comment will be removed. > > > + * @features: out parameter will contain uffdio_api.features provided by kernel > > + * in case of success > > + */ > > +static bool receive_ufd_features(uint64_t *features) > > { > > - struct uffdio_api api_struct; > > - uint64_t ioctl_mask; > > + struct uffdio_api api_struct = {0}; > > + int ufd; > > + bool ret = true; > > + > > + /* if we are here __NR_userfaultfd should exists */ > > + ufd = syscall(__NR_userfaultfd, O_CLOEXEC); > > + if (ufd == -1) { > > + error_report("%s: syscall __NR_userfaultfd failed: %s", __func__, > > + strerror(errno)); > > + return false; > > + } > > > > + /* ask features */ > > api_struct.api = UFFD_API; > > api_struct.features = 0; > > if (ioctl(ufd, UFFDIO_API, &api_struct)) { > > - error_report("%s: UFFDIO_API failed: %s", __func__ > > + error_report("%s: UFFDIO_API failed: %s", __func__, > > strerror(errno)); > > + ret = false; > > + goto release_ufd; > > + } > > + > > + *features = api_struct.features; > > + > > +release_ufd: > > + close(ufd); > > + return ret; > > +} > > + > > +/** > > + * request_ufd_features: this function should be called only once on a newly > > + * opened ufd, subsequent calls will lead to error. > > + * > > + * Returns: true on succes > > + * > > + * @ufd: fd obtained from userfaultfd syscall > > + * @features: bit mask see UFFD_API_FEATURES > > + */ > > +static bool request_ufd_features(int ufd, uint64_t features) > > +{ > > + struct uffdio_api api_struct = {0}; > > + uint64_t ioctl_mask; > > + > > + api_struct.api = UFFD_API; > > + api_struct.features = features; > > + if (ioctl(ufd, UFFDIO_API, &api_struct)) { > > + error_report("%s failed: UFFDIO_API failed: %s", __func__, > > + strerror(errno)); > > Maybe we can indent this line to follow this file's rule? > > error_report("%s failed: UFFDIO_API failed: %s", __func__, > strerror(errno)); looks like I missed that rule. > > > return false; > > } > > > > - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | > > - (__u64)1 << _UFFDIO_UNREGISTER; > > + ioctl_mask = 1 << _UFFDIO_REGISTER | > > + 1 << _UFFDIO_UNREGISTER; > > Could I ask why we explicitly removed (__u64) here? Since I see the > old one better. maybe my change not robust, in any case thank to point me, but now I think, here should be a constant instead of ioctl_mask, like UFFD_API_IOCTLS, the total meaning of that check it's make sure kernel returns to us no error and accepted features. ok, from the beginning: if we request unsupported feature (we check it before) or internal state of userfault ctx inside kernel isn't UFFD_STATE_WAIT_API (for example we are in the middle of the coping process) ioctl should end with EINVAL error and ioctls field in uffdio_api will be empty Right now I think ioctls check for UFFD_API is not necessary. We just say here, we will use _UFFDIO_REGISTER, _UFFDIO_UNREGISTER, but kernel supports it unconditionally, by contrast with UFFDIO_REGISTER ioctl - it also returns ioctl field in uffdio_register structure, here can be a variations. > > > 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; > > +} > > + > > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis) > > +{ > > + uint64_t asked_features = 0; > > + static uint64_t supported_features; > > + > > + /* > > + * it's not possible to > > + * request UFFD_API twice per one fd > > + * userfault fd features is persistent > > + */ > > + if (!supported_features) { > > I would prefer not having this static variable. After all, this > function call is rare, and the receive_ufd_features() is not that slow > as well. ok ) for the sake of low code complexity > > > + if (!receive_ufd_features(&supported_features)) { > > + error_report("%s failed", __func__); > > + return false; > > + } > > + } > > + > > + /* > > + * request features, even if asked_features is 0, due to > > + * kernel expects UFFD_API before UFFDIO_REGISTER, per > > + * userfault file descriptor > > + */ > > + if (!request_ufd_features(ufd, asked_features)) { > > + error_report("%s failed: features %" PRIu64, __func__, > > + asked_features); > > Better indent? > > Thanks, > > -- > Peter Xu >
On Wed, May 24, 2017 at 09:45:48AM +0300, Alexey wrote: [...] > > > > > return false; > > > } > > > > > > - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | > > > - (__u64)1 << _UFFDIO_UNREGISTER; > > > + ioctl_mask = 1 << _UFFDIO_REGISTER | > > > + 1 << _UFFDIO_UNREGISTER; > > > > Could I ask why we explicitly removed (__u64) here? Since I see the > > old one better. > maybe my change not robust, in any case thank to point me, but now I > think, here should be a constant instead of ioctl_mask, like > UFFD_API_IOCTLS, the total meaning of that check it's make sure kernel > returns to us no error and accepted features. > ok, from the beginning: > > if we request unsupported feature (we check it before) or internal > state of userfault ctx inside kernel isn't UFFD_STATE_WAIT_API (for > example we are in the middle of the coping process) > ioctl should end with EINVAL error and ioctls field in > uffdio_api will be empty > > Right now I think ioctls check for UFFD_API is not necessary. > We just say here, we will use _UFFDIO_REGISTER, _UFFDIO_UNREGISTER, > but kernel supports it unconditionally, by contrast with > UFFDIO_REGISTER ioctl - it also returns ioctl field in uffdio_register > structure, here can be a variations. Sorry I didn't get the point... AFAIU here (__u64) makes the constant "1" a 64bit variable. Just like when we do bit shift we normally have "1ULL<<40". I liked it since even if _UFFDIO_REGISTER is defined as >32 it will not overflow since by default a constant "1" is a "int" typed (and it's 32bit width). Thanks,
On 05/24/2017 02:33 PM, Peter Xu wrote: > On Wed, May 24, 2017 at 09:45:48AM +0300, Alexey wrote: > > [...] > >>>> return false; >>>> } >>>> >>>> - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | >>>> - (__u64)1 << _UFFDIO_UNREGISTER; >>>> + ioctl_mask = 1 << _UFFDIO_REGISTER | >>>> + 1 << _UFFDIO_UNREGISTER; >>> Could I ask why we explicitly removed (__u64) here? Since I see the >>> old one better. >> maybe my change not robust, in any case thank to point me, but now I >> think, here should be a constant instead of ioctl_mask, like >> UFFD_API_IOCTLS, the total meaning of that check it's make sure kernel >> returns to us no error and accepted features. >> ok, from the beginning: >> >> if we request unsupported feature (we check it before) or internal >> state of userfault ctx inside kernel isn't UFFD_STATE_WAIT_API (for >> example we are in the middle of the coping process) >> ioctl should end with EINVAL error and ioctls field in >> uffdio_api will be empty >> >> Right now I think ioctls check for UFFD_API is not necessary. >> We just say here, we will use _UFFDIO_REGISTER, _UFFDIO_UNREGISTER, >> but kernel supports it unconditionally, by contrast with >> UFFDIO_REGISTER ioctl - it also returns ioctl field in uffdio_register >> structure, here can be a variations. > Sorry I didn't get the point... I misprinted >We just say here, we will use _UFFDIO_REGISTER >s/_UFFDIO_REGISTER/_UFFDIO_API/g but the point, ioctl_mask is not necessary here, kernel always returns it. But for _UFFDIO_UNREGISTER, later, not in this function, yes that check is required. > > AFAIU here (__u64) makes the constant "1" a 64bit variable. Just like > when we do bit shift we normally have "1ULL<<40". I liked it since > even if _UFFDIO_REGISTER is defined as >32 it will not overflow since > by default a constant "1" is a "int" typed (and it's 32bit width). > Thanks, >
* Alexey Perevalov (a.perevalov@samsung.com) wrote: > On 05/24/2017 02:33 PM, Peter Xu wrote: > > On Wed, May 24, 2017 at 09:45:48AM +0300, Alexey wrote: > > > > [...] > > > > > > > return false; > > > > > } > > > > > - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | > > > > > - (__u64)1 << _UFFDIO_UNREGISTER; > > > > > + ioctl_mask = 1 << _UFFDIO_REGISTER | > > > > > + 1 << _UFFDIO_UNREGISTER; > > > > Could I ask why we explicitly removed (__u64) here? Since I see the > > > > old one better. > > > maybe my change not robust, in any case thank to point me, but now I > > > think, here should be a constant instead of ioctl_mask, like > > > UFFD_API_IOCTLS, the total meaning of that check it's make sure kernel > > > returns to us no error and accepted features. > > > ok, from the beginning: > > > > > > if we request unsupported feature (we check it before) or internal > > > state of userfault ctx inside kernel isn't UFFD_STATE_WAIT_API (for > > > example we are in the middle of the coping process) > > > ioctl should end with EINVAL error and ioctls field in > > > uffdio_api will be empty > > > > > > Right now I think ioctls check for UFFD_API is not necessary. > > > We just say here, we will use _UFFDIO_REGISTER, _UFFDIO_UNREGISTER, > > > but kernel supports it unconditionally, by contrast with > > > UFFDIO_REGISTER ioctl - it also returns ioctl field in uffdio_register > > > structure, here can be a variations. > > Sorry I didn't get the point... > I misprinted > >We just say here, we will use _UFFDIO_REGISTER > > > s/_UFFDIO_REGISTER/_UFFDIO_API/g > but the point, ioctl_mask is not necessary here, kernel always returns it. > But for _UFFDIO_UNREGISTER, later, not in this function, yes that check is required. But Peter's only point was that to build the mask it's better to keep the (__u64) cast for safety. Dave > > > > AFAIU here (__u64) makes the constant "1" a 64bit variable. Just like > > when we do bit shift we normally have "1ULL<<40". I liked it since > > even if _UFFDIO_REGISTER is defined as >32 it will not overflow since > > by default a constant "1" is a "int" typed (and it's 32bit width). > > > Thanks, > > > > -- > Best regards, > Alexey Perevalov -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 3ed78bf..4f3f495 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -59,32 +59,114 @@ struct PostcopyDiscardState { #include <sys/eventfd.h> #include <linux/userfaultfd.h> -static bool ufd_version_check(int ufd, MigrationIncomingState *mis) + +/** + * receive_ufd_features: check userfault fd features, to request only supported + * features in the future. + * + * Returns: true on success + * + * __NR_userfaultfd - should be checked before + * @features: out parameter will contain uffdio_api.features provided by kernel + * in case of success + */ +static bool receive_ufd_features(uint64_t *features) { - struct uffdio_api api_struct; - uint64_t ioctl_mask; + struct uffdio_api api_struct = {0}; + int ufd; + bool ret = true; + + /* if we are here __NR_userfaultfd should exists */ + ufd = syscall(__NR_userfaultfd, O_CLOEXEC); + if (ufd == -1) { + error_report("%s: syscall __NR_userfaultfd failed: %s", __func__, + strerror(errno)); + return false; + } + /* ask features */ api_struct.api = UFFD_API; api_struct.features = 0; if (ioctl(ufd, UFFDIO_API, &api_struct)) { - error_report("%s: UFFDIO_API failed: %s", __func__ + error_report("%s: UFFDIO_API failed: %s", __func__, strerror(errno)); + ret = false; + goto release_ufd; + } + + *features = api_struct.features; + +release_ufd: + close(ufd); + return ret; +} + +/** + * request_ufd_features: this function should be called only once on a newly + * opened ufd, subsequent calls will lead to error. + * + * Returns: true on succes + * + * @ufd: fd obtained from userfaultfd syscall + * @features: bit mask see UFFD_API_FEATURES + */ +static bool request_ufd_features(int ufd, uint64_t features) +{ + struct uffdio_api api_struct = {0}; + uint64_t ioctl_mask; + + api_struct.api = UFFD_API; + api_struct.features = features; + if (ioctl(ufd, UFFDIO_API, &api_struct)) { + error_report("%s failed: UFFDIO_API failed: %s", __func__, + strerror(errno)); return false; } - ioctl_mask = (__u64)1 << _UFFDIO_REGISTER | - (__u64)1 << _UFFDIO_UNREGISTER; + ioctl_mask = 1 << _UFFDIO_REGISTER | + 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; +} + +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis) +{ + uint64_t asked_features = 0; + static uint64_t supported_features; + + /* + * it's not possible to + * request UFFD_API twice per one fd + * userfault fd features is persistent + */ + if (!supported_features) { + if (!receive_ufd_features(&supported_features)) { + error_report("%s failed", __func__); + return false; + } + } + + /* + * request features, even if asked_features is 0, due to + * kernel expects UFFD_API before UFFDIO_REGISTER, per + * userfault file descriptor + */ + if (!request_ufd_features(ufd, asked_features)) { + error_report("%s failed: features %" PRIu64, __func__, + asked_features); + return false; + } + if (getpagesize() != ram_pagesize_summary()) { bool have_hp = false; /* We've got a huge page */ #ifdef UFFD_FEATURE_MISSING_HUGETLBFS - have_hp = api_struct.features & UFFD_FEATURE_MISSING_HUGETLBFS; + have_hp = supported_features & UFFD_FEATURE_MISSING_HUGETLBFS; #endif if (!have_hp) { error_report("Userfault on this host does not support huge pages"); @@ -135,7 +217,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis) } /* Version and features check */ - if (!ufd_version_check(ufd, mis)) { + if (!ufd_check_and_apply(ufd, mis)) { goto out; } @@ -512,7 +594,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, mis)) { + if (!ufd_check_and_apply(mis->userfault_fd, mis)) { return -1; }
This modification is necessary for userfault fd features which are required to be requested from userspace. UFFD_FEATURE_THREAD_ID is a one of such "on demand" feature, which will be introduced in the next patch. QEMU have to use separate userfault file descriptor, due to userfault context has internal state, and after first call of ioctl UFFD_API it changes its state to UFFD_STATE_RUNNING (in case of success), but kernel while handling ioctl UFFD_API expects UFFD_STATE_WAIT_API. So only one ioctl with UFFD_API is possible per ufd. Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> --- migration/postcopy-ram.c | 100 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 9 deletions(-)