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 10:46:13 -0500 Message-ID: <20150123154613.GA15927@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YEgRI-00048h-AQ for xen-devel@lists.xenproject.org; Fri, 23 Jan 2015 15:46:28 +0000 Content-Disposition: inline In-Reply-To: <54C2782B0200007800058F01@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 03:34:51PM +0000, Jan Beulich wrote: > >>> On 23.01.15 at 15:46, wrote: > > On 23/01/15 14:32, Konrad Rzeszutek Wilk wrote: > >> On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote: > >>>>>> On 22.01.15 at 17:38, wrote: > >>>> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote: > >>>>>>>> On 21.01.15 at 22:27, wrote: > >>>>>> As I was looking at some of the XSA I realized that the > >>>>>> call-chain of: > >>>>>> > >>>>>> domain_relinquish_resources > >>>>>> ->vcpu_destroy_pagetables > >>>>>> -> put_page_and_type_preemptible > >>>>>> -> __put_page_type > >>>>>> returns -EINTR > >>>>>> [...] > >>>>>> b). Or should the hypervisor convert the -EINTR to -ERESTART > >>>>>> (or -EAGAIN) - which most of the code (see users of > >>>>>> get_page_type_preemptible) do right now? > >>>>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked > >>>>> this when adding the preemption capability here. > >>>> OK. Would this RFC patch be OK? (I can send it off as normal - just > >>>> want to make sure you are OK with this being put there) > >>> Conceptually yes, but there are issues: > >>> > >>>> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001 > >>>> From: Konrad Rzeszutek Wilk > >>>> Date: Thu, 22 Jan 2015 11:34:21 -0500 > >>>> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR > >>>> instead of -EAGAIN > >>> You should stop sending such conversions to EAGAIN. We switched > >>> to ERESTART, and you (just guessing) taking the patch from an > >>> older Xen version shouldn't result in this recurring mistake. > >> Nah, Andrew said in his email EAGAIN so that is what I picked. > > > > EAGAIN was correct for the domain_destroy hypercall, at this hypercall > > explicitly has continuation built into its API via EAGAIN. > > Ouch - I based the comment on code resulting from a patch I > didn't send out yet (largely because Konrad indicated issues with > XEN_DOMCTL_destroydomain that I'd need to look into in more > detail before doing so), doing away with the tool stack based > continuations. Yet based on what domain_kill() does with > domain_relinquish_resources()'s return value, it should > nevertheless be -ERESTART here to ease future changes. OK :-) >>From ac642805a96261f518fbba7d47f3ca38c950b3c8 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 22 Jan 2015 11:34:21 -0500 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 preemption mechanism we have to domain destruction is to return -EAGAIN 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. Signed-off-by: Konrad Rzeszutek Wilk --- v2: Add comment and s/ERESTART/EAGAIN/ --- xen/arch/x86/mm.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 6e9c2c0..5452c01 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2677,6 +2677,16 @@ int vcpu_destroy_pagetables(struct vcpu *v) v->arch.cr3 = 0; + /* + * 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). + */ + if ( rc == -EINTR ) + rc = -ERESTART; + return rc; } -- 2.1.0