Re: [Xen-devel] -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
> >>> On 22.01.15 at 17:38, <konrad.wilk@xxxxxxxxxx> wrote:
> > On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
> >> >>> On 21.01.15 at 22:27, <konrad.wilk@xxxxxxxxxx> 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 <konrad.wilk@xxxxxxxxxx>
> > 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.
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2677,6 +2677,9 @@ int vcpu_destroy_pagetables(struct vcpu *v)
> >  
> >      v->arch.cr3 = 0;
> >  
> > +    if ( rc == -EINTR )
> > +        rc = -EAGAIN;
> > +
> 
> Either (my preference) you attach this directly to the two
> put_page_and_type_preemptible() invocations, or you add a
> comment here explaining that this catches those two specific
> cases in one central place. This is because while right now only

I will add explanation. The other users of put_page_.. all have
catch the -EINTR and do the conversion.

> those two invocations can lead to rc being non-zero, there's
> nothing preventing other error generating code to be added to
> this function later on, and we shouldn't blindly convert -EINTR
> to some other error code, as that may lead to overlooking
> necessary conversions elsewhere (as happened while I made
> this function preemptible).
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel