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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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
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