Re: [PATCH] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross <jgross@suse.com>]

On 26/02/16 13:39, Jan Beulich wrote:
>>>> On 26.02.16 at 12:20, <dario.faggioli@citrix.com> wrote:
>> On Fri, 2016-02-26 at 12:14 +0100, Juergen Gross wrote:
>>> On 26/02/16 11:39, Jan Beulich wrote:
>>>>
>>>>> @@ -670,7 +676,13 @@ int cpu_disable_scheduler(unsigned int cpu)
>>>>>              if ( cpumask_empty(&online_affinity) &&
>>>>>                   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>>>>>              {
>>>>> -                printk(XENLOG_DEBUG "Breaking affinity for
>>>>> %pv\n", v);
>>>>> +                if ( v->affinity_broken )
>>>>> +                {
>>>>> +                    /* The vcpu is temporarily pinned, can't
>>>>> move it. */
>>>>> +                    vcpu_schedule_unlock_irqrestore(lock, flags,
>>>>> v);
>>>>> +                    ret = -EBUSY;
>>>>> +                    continue;
>>>>> +                }
>>>> So far the function can only return 0 or -EAGAIN. By using
>>>> "continue"
>>>> here you will make it impossible for the caller to reliably
>>>> determine
>>>> whether possibly both things failed. Despite -EBUSY being a logical
>>>> choice here, I think you'd better use -EAGAIN here too. And it
>>>> needs
>>>> to be determined whether continuing the loop in this as well as the
>>>> pre-existing cases is actually the right thing to do.
>>> EBUSY vs. EAGAIN: by returning EAGAIN I would signal to Xen tools
>>> that
>>> the hypervisor is currently not able to do the desired operation
>>> (especially removing a cpu from a cpupool), but the situation will
>>> change automatically via scheduling. EBUSY will stop retries in Xen
>>> tools and this is want I want here: I can't be sure the situation
>>> will change soon.
>>>
>> I agree with this.
> 
> I'm of two minds here: I can see your viewpoint, but considering
> this is called "temporarily pin a vcpu" the condition is supposed to
> be going away again soon.

It is supposed to do so, yes. But the hypervisor can't make sure it
will, as it requires an appropriate hypercall by the hardware domain.
In the cpupool case no domain is capable to make the situation
persist.

Would you be fine with adding a tools patch doing a limited number
of retries in the EBUSY case (maybe with sleeping 1 second in that
case)?

> 
>>> Regarding continuation of the loop: I think you are right in the
>>> EBUSY case: I should break out of the loop. I should not do so in the
>>> EAGAIN case as I want to remove as many vcpus from the physical cpu
>>> as
>>> possible without returning to the Xen tools in between.
>>>
>> And with this too.
>>
>> And I think that, if we indeed break out of the loop on EBUSY, that
>> will also make it possible to figure out properly what actually went
>> wrong, so it should be fine from that point of view as well.
> 
> Yes indeed.
> 
>>>>> @@ -679,6 +691,8 @@ int cpu_disable_scheduler(unsigned int cpu)
>>>>>                      v->affinity_broken = 1;
>>>>>                  }
>>>>>  
>>>>> +                printk(XENLOG_DEBUG "Breaking affinity for
>>>>> %pv\n", v);
>>>> Wouldn't it be even better to make this the "else" to the
>>>> preceding if(), since in the suspend case this is otherwise going
>>>> to be printed for every vCPU not currently running on pCPU0?
>>> Yes, I'll change it.
>>>
>> On this, can (either of) you elaborate a bit more? I don't think I'm
>> following...
> 
> In addition to Jürgen's reply: My main concern here is that on
> a bug system this message would get printed for almost every
> vCPU in the system, which could end up being a lot of noise.
> 
> And there's a similar message on the resume side I think -
> perhaps that one should be silenced too.

Okay. I'll do the silencing (both cases) in an extra patch.


Juergen

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