[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check ont
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part |
Date: |
Wed, 24 May 2017 10:36:29 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
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...
> + * @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));
> 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.
> 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.
> + 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
- [Qemu-devel] [PATCH V6 09/10] migration: add postcopy total blocktime into query-migrate, (continued)
- Message not available
- Message not available
- Message not available
- Message not available
- [Qemu-devel] [PATCH V6 08/10] migration: calculate vCPU blocktime on dst side, Alexey Perevalov, 2017/05/23
- Re: [Qemu-devel] [PATCH V6 08/10] migration: calculate vCPU blocktime on dst side, Peter Xu, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 08/10] migration: calculate vCPU blocktime on dst side, Alexey, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 08/10] migration: calculate vCPU blocktime on dst side, Peter Xu, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 08/10] migration: calculate vCPU blocktime on dst side, Alexey Perevalov, 2017/05/24
Message not available
- [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part, Alexey Perevalov, 2017/05/23
- Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part,
Peter Xu <=
- Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part, Alexey, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part, Peter Xu, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part, Alexey Perevalov, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 04/10] migration: split ufd_version_check onto receive/request features part, Dr. David Alan Gilbert, 2017/05/31
Message not available
- [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Alexey Perevalov, 2017/05/23
- Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Peter Xu, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Alexey, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Peter Xu, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Alexey Perevalov, 2017/05/24
- Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page, Peter Xu, 2017/05/24