From patchwork Mon Apr 24 08:12:29 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Xu X-Patchwork-Id: 754141 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wBJxc41sPz9sDG for ; Mon, 24 Apr 2017 18:13:08 +1000 (AEST) Received: from localhost ([::1]:42622 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d2Z7K-0002LE-2O for incoming@patchwork.ozlabs.org; Mon, 24 Apr 2017 04:13:06 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d2Z6w-0002KI-Qk for qemu-devel@nongnu.org; Mon, 24 Apr 2017 04:12:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d2Z6t-00021Y-LV for qemu-devel@nongnu.org; Mon, 24 Apr 2017 04:12:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45358) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d2Z6t-00021P-BN for qemu-devel@nongnu.org; Mon, 24 Apr 2017 04:12:39 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 32F86C059758; Mon, 24 Apr 2017 08:12:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 32F86C059758 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=peterx@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 32F86C059758 Received: from pxdev.xzpeter.org (ovpn-8-111.pek2.redhat.com [10.72.8.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DD39F18153; Mon, 24 Apr 2017 08:12:33 +0000 (UTC) Date: Mon, 24 Apr 2017 16:12:29 +0800 From: Peter Xu To: Alexey Message-ID: <20170424081229.GB31384@pxdev.xzpeter.org> References: <1492175840-5021-1-git-send-email-a.perevalov@samsung.com> <1492175840-5021-4-git-send-email-a.perevalov@samsung.com> <20170421102453.GD2908@work-vm> <20170421152212.GB5101@aperevalov-ubuntu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170421152212.GB5101@aperevalov-ubuntu> User-Agent: Mutt/1.5.24 (2015-08-30) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 24 Apr 2017 08:12:38 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH 3/6] migration: add UFFD_FEATURE_THREAD_ID feature support X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: i.maximets@samsung.com, aarcange@redhat.com, "Dr. David Alan Gilbert" , qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" 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 (a.perevalov@samsung.com) 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 > > > > 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 > > > #include > > > > > > -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: > /* 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?) 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). 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; }