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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.16 at 13:49, <JGross@xxxxxxxx> wrote:
> On 26/02/16 13:39, Jan Beulich wrote:
>>>>> On 26.02.16 at 12:20, <dario.faggioli@xxxxxxxxxx> 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)?

That would make me worry less, yes.

Jan


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