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

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

On Fri, Jan 23, 2015 at 04:03:55PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 16:46, <konrad.wilk@xxxxxxxxxx> 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 <konrad.wilk@xxxxxxxxxx>
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 <konrad.wilk@xxxxxxxxxx>
---
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
> 

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