Re: [Qemu-devel] [PATCH 3/6] migration: add UFFD_FEATURE_THREAD_ID feature support

[Dr. David Alan Gilbert]

* Alexey (address@hidden) wrote:
> + Andrea Arcangeli
> 
> On Mon, Apr 24, 2017 at 06:10:02PM +0100, Dr. David Alan Gilbert wrote:
> > * Alexey (address@hidden) wrote:
> > > On Mon, Apr 24, 2017 at 04:12:29PM +0800, Peter Xu wrote:
> > > > On Fri, Apr 21, 2017 at 06:22:12PM +0300, Alexey wrote:
> > > > > On Fri, Apr 21, 2017 at 11:24:54AM +0100, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > * Alexey Perevalov (address@hidden) 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 <address@hidden>
> > > > > > 
> > > > > > 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
> > > > 
> > > > I feel like this does not against Dave's comment, maybe we just need
> > > > to send the UFFDIO_API twice? Like:
> > > yes, ioctl with UFFDIO_API will fail on old kernel if we will request
> > > e.g. UFFD_FEATURE_THREAD_ID or other new feature.
> > > 
> > > So in general way need a per feature request, for better error handling.
> > 
> > No, we don't need to - I think the way the kernel works is that you pass
> > features = 0 in, and it sets api_struct.features on the way out;
> > so if you always pass 0 in, you can then just check the features that
> > it returns.
> >
> Without explicitly set UFFD_FEATURE_THREAD_ID, ptid will not sent back
> to user space.
> 
> Also it's impossible to call ioctl UFFD_API more than one time, due to
> internal state of userfault_ctx inside kernel is changing
> UFFD_STATE_WAIT_API -> UFFD_STATE_RUNNING, 
> but ioctl UFFD_API expects UFFD_STATE_WAIT_API
> ^^^
> 
> So looks like no way to provide backward compatibility for old kernels.
> I even don't know how to be with new kernels, because point of extension
> should be for new kernels (e.g. I want to add new feature in future,
> UFFD_FEATURE_ALLOW_PADDING which will allow UFFD_COPY for lesser page
> size than was registered).
> So how to be in this case, add new UFFD feature, like
> UFFD_FEATURE_ALLOW_CALL_API_AGAIN (allow set not always/persistent feature,
> like UFFD_FEATURE_THREAD_ID)
> 
> or just remove condition in kernel while sending ptid.
> 
> Or it's even not a problem, just close ufd/reopen and resend
> UFFD_FEATURE_THREAD_ID.

Yes, I think you'll have to do that;  so I guess you need to:
   a) Change ufd_version_check to open the ufd in the first place
      and remove the other syscalls that open it
   b) Make it pass features = 0 in to start with so that you can
      see if the kernel supports it
   c) Close the ufd and then do the API call again with the
      features you need.

Does that work?

Note we must never break QEMU working on old kernels.

Dave

> > Dave
> > 
> > > 
> > > > 
> > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > > index 85fd8d7..fd0905f 100644
> > > > --- a/migration/postcopy-ram.c
> > > > +++ b/migration/postcopy-ram.c
> > > > @@ -64,6 +64,7 @@ static bool ufd_version_check(int ufd)
> > > >  {
> > > >      struct uffdio_api api_struct;
> > > >      uint64_t ioctl_mask;
> > > > +    uint64_t features = 0;
> > > > 
> > > >      api_struct.api = UFFD_API;
> > > >      api_struct.features = 0;
> > > > @@ -92,6 +93,27 @@ static bool ufd_version_check(int ufd)
> > > >              return false;
> > > >          }
> > > >      }
> > > > +
> > > > +#ifdef UFFD_FEATURE_THREAD_ID
> > > > +    if (api_struct.features & UFFD_FEATURE_THREAD_ID) {
> > > > +        features |= UFFD_FEATURE_THREAD_ID;
> > > > +    }
> > > > +#endif
> > > > +
> > > > +    if (features) {
> > > > +        /*
> > > > +         * If there are new features to be enabled from userspace,
> > > > +         * trigger another UFFDIO_API ioctl.
> > > > +         */
> > > > +        api_struct.api = UFFD_API;
> > > > +        api_struct.features = features;
> > > > +        if (ioctl(ufd, UFFDIO_API, &api_struct)) {
> > > > +            error_report("UFFDIO_API failed to setup features: 
> > > > 0x%"PRIx64,
> > > > +                         features);
> > > > +            return false;
> > > > +        }
> > > > +    }
> > > > +
> > > >      return true;
> > > >  }
> > > > 
> > > > > /* 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);
> > > > 
> > > > (I am slightly curious about why we need this if block, after all
> > > >  userspace should know whether the ptid field would be valid from the
> > > >  first UFFDIO_API ioctl, right?)
> > > If I correctly understand you question ) that condition was suggested,
> > > due to page faulting is performance critical part (in general, not only 
> > > postcopy
> > > case ), that's why it should be enabled from userspace, 
> > > only for statistics/debug purpose.
> > > Also looks like David want to see that feature on QEMU as not always
> > > feature too.
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > 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).
> > > > 
> > > > -- 
> > > > Peter Xu
> > > > 
> > > 
> > > -- 
> > > 
> > > BR
> > > Alexey
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > 
> 
> -- 
> 
> BR
> Alexey
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK