subject:"\[Qemu\-devel\] \[PATCH V5 4\/9\] migration\: split ufd_version_check onto receive\/request features part"

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

2017-05-19 Thread Dr. David Alan Gilbert

* Alexey (a.pereva...@samsung.com) wrote:
> On Tue, May 16, 2017 at 11:32:51AM +0100, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (a.pereva...@samsung.com) 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 
> > > ---
> > >  migration/postcopy-ram.c | 82 
> > > ++--
> > >  1 file changed, 73 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > index 0f75700..c96d5f5 100644
> > > --- a/migration/postcopy-ram.c
> > > +++ b/migration/postcopy-ram.c
> > > @@ -60,32 +60,96 @@ struct PostcopyDiscardState {
> > >  #include 
> > >  #include 
> > >  
> > > -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
> > 
> > That's not quite right;
> >  * Returns: True on success, sets *features to supported features
> > False on failure or if kernel doesn't support ufd
> > 
> yes, obtained features is out parameter,
> but I want to keep false uncommented and just add error_report into
> syscall check, because the possible reason of failure is:
> 1. No syscall userfaultfd, but function expects that syscall, it reflects in
> comment
> 2  Within syscall:  exhausted fd or out of memory (file in kernel
> is allocating)
> 3. Problem in ioctl due to internal state of UFFD, as example
> UFFDIO_API after UFFDIO_REGISTER

I don't think we're allowed to depend on error pointers, but either
way we should comment it to make sure it's clear, so if you have a
boolean return at least say it's true for success and explain features
etc.

> Also I would prefer follow migration/ram.c comment style.

Yes, that's fine - it's the content of the comment I was more
worried about (and the one below).

Dave

> > > + */
> > > +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) {
> > > +return false;
> > > +}
> > >  
> > > +/* ask features */
> > >  api_struct.api = UFFD_API;
> > >  api_struct.features = 0;
> > >  if (ioctl(ufd, UFFDIO_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;
> > > +}
> > 
> > Needs a comment; perhaps something like:
> >   * Called once on a newly opened ufd, can request specific features.
> >   * Returns: True on success
> > 
> > > +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, _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;
> > > +uint64_t supported_features;
> > > +
> > > +/*
> > > + * it's not possible to
> > > + * request UFFD_API twice per one fd
> > > + */
> > > +if

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

2017-05-18 Thread Alexey

On Tue, May 16, 2017 at 11:32:51AM +0100, Dr. David Alan Gilbert wrote:
> * Alexey Perevalov (a.pereva...@samsung.com) 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 
> > ---
> >  migration/postcopy-ram.c | 82 
> > ++--
> >  1 file changed, 73 insertions(+), 9 deletions(-)
> > 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 0f75700..c96d5f5 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -60,32 +60,96 @@ struct PostcopyDiscardState {
> >  #include 
> >  #include 
> >  
> > -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
> 
> That's not quite right;
>  * Returns: True on success, sets *features to supported features
> False on failure or if kernel doesn't support ufd
> 
yes, obtained features is out parameter,
but I want to keep false uncommented and just add error_report into
syscall check, because the possible reason of failure is:
1. No syscall userfaultfd, but function expects that syscall, it reflects in
comment
2  Within syscall:  exhausted fd or out of memory (file in kernel
is allocating)
3. Problem in ioctl due to internal state of UFFD, as example
UFFDIO_API after UFFDIO_REGISTER

Also I would prefer follow migration/ram.c comment style.
> > + */
> > +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) {
> > +return false;
> > +}
> >  
> > +/* ask features */
> >  api_struct.api = UFFD_API;
> >  api_struct.features = 0;
> >  if (ioctl(ufd, UFFDIO_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;
> > +}
> 
> Needs a comment; perhaps something like:
>   * Called once on a newly opened ufd, can request specific features.
>   * Returns: True on success
> 
> > +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, _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;
> > +uint64_t supported_features;
> > +
> > +/*
> > + * it's not possible to
> > + * request UFFD_API twice per one fd
> > + */
> > +if (!receive_ufd_features(_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

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

2017-05-16 Thread Dr. David Alan Gilbert

* Alexey Perevalov (a.pereva...@samsung.com) 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 
> ---
>  migration/postcopy-ram.c | 82 
> ++--
>  1 file changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 0f75700..c96d5f5 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -60,32 +60,96 @@ struct PostcopyDiscardState {
>  #include 
>  #include 
>  
> -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

That's not quite right;
 * Returns: True on success, sets *features to supported features
False on failure or if kernel doesn't support ufd

> + */
> +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) {
> +return false;
> +}
>  
> +/* ask features */
>  api_struct.api = UFFD_API;
>  api_struct.features = 0;
>  if (ioctl(ufd, UFFDIO_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;
> +}

Needs a comment; perhaps something like:
  * Called once on a newly opened ufd, can request specific features.
  * Returns: True on success

> +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, _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;
> +uint64_t supported_features;
> +
> +/*
> + * it's not possible to
> + * request UFFD_API twice per one fd
> + */
> +if (!receive_ufd_features(_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");
> @@ -136,7 +200,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;
>  }
>  
> @@ -513,7 +577,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

[Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

2017-05-12 Thread Alexey Perevalov

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 
---
 migration/postcopy-ram.c | 82 ++--
 1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0f75700..c96d5f5 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -60,32 +60,96 @@ struct PostcopyDiscardState {
 #include 
 #include 
 
-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(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) {
+return false;
+}
 
+/* ask features */
 api_struct.api = UFFD_API;
 api_struct.features = 0;
 if (ioctl(ufd, UFFDIO_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;
+}
+
+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, _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;
+uint64_t supported_features;
+
+/*
+ * it's not possible to
+ * request UFFD_API twice per one fd
+ */
+if (!receive_ufd_features(_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");
@@ -136,7 +200,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;
 }
 
@@ -513,7 +577,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;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

Re: [Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

[Qemu-devel] [PATCH V5 4/9] migration: split ufd_version_check onto receive/request features part

4 matches

Site Navigation

Mail list logo

Footer information