From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: -EINTR return in domain_relinquish_resources Date: Fri, 23 Jan 2015 12:21:35 -0500 Message-ID: <20150123172135.GA4888@l.oracle.com> References: <20150121212720.GA24555@l.oracle.com> <54C0D8530200007800057FEB@mail.emea.novell.com> <20150122163840.GB32691@l.oracle.com> <54C21E4D0200007800058992@mail.emea.novell.com> <20150123143247.GE7365@l.oracle.com> <54C25EB2.6090403@citrix.com> <54C2782B0200007800058F01@mail.emea.novell.com> <20150123154613.GA15927@l.oracle.com> <54C27EFB0200007800058F83@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YEhvT-0004dk-7J for xen-devel@lists.xenproject.org; Fri, 23 Jan 2015 17:21:43 +0000 Content-Disposition: inline In-Reply-To: <54C27EFB0200007800058F83@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Fri, Jan 23, 2015 at 04:03:55PM +0000, Jan Beulich wrote: > >>> On 23.01.15 at 16:46, wrote: > > Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART > > instead of -EINTR > > > > which has the side effect that domain_relinquish_resources will stop > > and return to user-space -EINTR - which it is not equipped to deal with. > > The title read wrong, especially on its own, as it appears to > state the inverse thing of what you do in the patch. Perhaps > > x86: vcpu_destroy_pagetables() must not return -EINTR > > with the initial part of the description adjusted accordingly? > > > + /* > > + * The put_page_and_type_preemptible is liable to return -EINTR. Other > > + * callers of it filter the -EINTR to whatever they deem applicable - in > > + * this case we MUST do it as the caller of this function will return the > > + * error code to userspace. And userspace for domain destruction expects > > + * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN). > > + */ > > This is still misleading, as it kind of implies that the function has only > that one caller. Don't talk about domain_relinquish_resources() and > EAGAIN at all. Right. I somehow managed to miss the other caller of vcpu_destroy_pagetables. Please see following patch: >>From 3ace309c61805bae546378e37553913231e43cec Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 22 Jan 2015 11:34:21 -0500 Subject: [PATCH] domain: vcpu_destroy_pagetables() must not return -EINTR .. otherwise it has the side effect that: domain_relinquish_resources will stop and will return to user-space with -EINTR which it is not equipped to deal with that error code; or vcpu_reset - which will ignore it and convert the error to -ENOMEM.. The preemption mechanism we have for domain destruction is to return -EAGAIN (and then user-space calls the hypercall again) and as such we need to catch the case of: domain_relinquish_resources ->vcpu_destroy_pagetables -> put_page_and_type_preemptible -> __put_page_type returns -EINTR and convert it to the proper type. For: XEN_DOMCTL_setvcpucontext -> vcpu_reset -> vcpu_destroy_pagetables we need to return -ERESTART otherwise we end up returning -ENOMEM. There are also other callers of vcpu_destroy_pagetables: arch_vcpu_reset (vcpu_reset) are: - hvm_s3_suspend (asserts on any return code), - vlapic_init_sipi_one (asserts on any return code), Signed-off-by: Konrad Rzeszutek Wilk --- v2: Add comment and s/ERESTART/EAGAIN/ v3: Also include the hypercall. --- xen/arch/x86/mm.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 6e9c2c0..badbeab 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2677,6 +2677,13 @@ int vcpu_destroy_pagetables(struct vcpu *v) v->arch.cr3 = 0; + /* + * The put_page_and_type_preemptible is liable to return -EINTR. The + * callers of us expect -ERESTART so convert it over. + */ + if ( rc == -EINTR ) + rc = -ERESTART; + return rc; } -- 2.1.0 > > Jan >