Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part

[Alexey <a.perevalov@samsung.com>]

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
> 

-- 

BR
Alexey