Re: [Qemu-devel] [PATCH RESEND V3 3/6] migration: split ufd_version_check onto receive/request features part

[Alexey Perevalov]

On 04/28/2017 12:01 PM, Peter Xu wrote:
> On Fri, Apr 28, 2017 at 09:57:35AM +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 need 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 <address@hidden>
> > ---
> >   migration/postcopy-ram.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 63 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 4c859b4..21e7150 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -60,15 +60,51 @@ struct PostcopyDiscardState {
> >   #include <sys/eventfd.h>
> >   #include <linux/userfaultfd.h>
> >   
> > -static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
> > +
> > +/*
> > + * Check userfault fd features, to request only supported features in
> > + * future.
> > + * __NR_userfaultfd - should be checked before
> > + * Return obtained features
> > + */
> > +static bool receive_ufd_features(__u64 *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) {
> This check should be <0 rather than -1?
right, kernel could return any type of error,
if (error < 0)
    return error;

> > +        return false;
> > +    }
> > +
> > +    /* ask features */
> >       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",
> > +        error_report("receive_ufd_features: UFFDIO_API failed: %s",
> > +                strerror(errno));
> > +        ret = false;
> > +        goto release_ufd;
> > +    }
> > +
> > +    *features = api_struct.features;
> > +
> > +release_ufd:
> > +    close(ufd);
> > +    return ret;
> > +}
> > +
> > +static bool request_ufd_features(int ufd, __u64 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("request_ufd_features: UFFDIO_API failed: %s",
> >                        strerror(errno));
> >           return false;
> >       }
> > @@ -81,11 +117,33 @@ static bool ufd_version_check(int ufd, 
> > MigrationIncomingState *mis)
> >           return false;
> >       }
> >   
> > +    return true;
> > +}
> > +
> > +static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
> This is not only a check not... It enables something in the kernel. So
> I'll suggest change the function name correspondingly.
yes, after that small changes, the meaning of the function has changed
maybe it's ufd_assign_and_check_features
> > +{
> > +    __u64 new_features = 0;
> > +
> > +    /* ask features */
> > +    __u64 supported_features;
> > +
> > +    if (!receive_ufd_features(&supported_features)) {
> > +        error_report("ufd_version_check failed");
> > +        return false;
> > +    }
> > +
> > +    /* request features */
> > +    if (new_features && !request_ufd_features(ufd, new_features)) {
> Firstly, looks like new_features == 0 here always, no?
I will use it in next patch.
> Second, I would suggest we enable feature explicitly. For this series,
> it's only for the THREAD_ID thing. I would mask the rest. The problem
> is, what if new features introduced in the future that we don't really
> want to enable for postcopy?
right now I think to rename new_features to enabled_features
or features_to_request,
if we don't want to enable feature - don't set according bit in 
enabled_features

> Thanks,
>
> > +        error_report("ufd_version_check failed: features %" PRIu64,
> > +                (uint64_t)new_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");
> > --
> > 1.9.1


--
Best regards,
Alexey Perevalov