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.pereva...@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
>
--
BR
Alexey
