Re: [Xen-devel] [PATCH v3] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall

[Julien Grall <julien.grall@arm.com>]

Hi Jan,

On 13/06/2019 13:58, Jan Beulich wrote:
>>>> On 13.06.19 at 14:48, <julien.grall@arm.com> wrote:
>> Hi Jan,
>>
>> On 13/06/2019 13:41, Jan Beulich wrote:
>>>>>> On 13.06.19 at 14:32, <andrii.anisov@gmail.com> wrote:
>>>> Jan, Julien,
>>>>
>>>> On 11.06.19 12:10, Jan Beulich wrote:
>>>>>>> At the very least such loops want a cpu_relax() in their bodies.
>>>>>>> But this being on a hypercall path - are there theoretical guarantees
>>>>>>> that a guest can't abuse this to lock up a CPU?
>>>>>> Hmmm, I suggested this but it looks like a guest may call the hypercall
>>>> multiple
>>>>>> time from different vCPU. So this could be a way to delay work on the CPU.
>>>>>>
>>>>>> I wanted to make the context switch mostly lockless and therefore avoiding
>>>> to
>>>>>> introduce a spinlock.
>>>>>
>>>>> Well, constructs like the above are trying to mimic a spinlock
>>>>> without actually using a spinlock. There are extremely rare
>>>>> situation in which this may indeed be warranted, but here it
>>>>> falls in the common "makes things worse overall" bucket, I
>>>>> think. To not unduly penalize the actual update paths, I think
>>>>> using a r/w lock would be appropriate here.
>>>>
>>>> So what is the conclusion here? Should we go with trylock and
>>>> hypercall_create_continuation() in order to avoid locking but still not fail
>>>> to the guest?
>>>
>>> I'm not convinced a "trylock" approach is needed - that's
>>> something Julien suggested.
>>
>> I think the trylock in the context switch is a must. Otherwise you would delay
>> context switch if the information get updated.
> 
> Delay in what way? I.e. how would this be an issue other than for
> the guest itself (which shouldn't be constantly updating the
> address for the region)?

Why would it only be an issue with the guest itself? Any wait on lock in Xen 
implies that you can't schedule another vCPU as we are not preemptible.

As the lock is taken in the context switch, I am worry that a guest continuously 
trying to call the hypercall and therefore use the lock may actually delay the 
end of the context switch. And therefore delay the rest of the work.

I suggested the trylock here, so the context switch could avoid updating the 
runstate if we are in the hypercall.

> 
>>> I'm pretty sure we're acquiring other
>>> locks in hypercall context without going the trylock route. I am
>>> convinced though that the pseudo-lock you've used needs to be
>>> replaced by a real (and perhaps r/w) one, _if_ there is any need
>>> for locking in the first place.
>>
>> You were the one asking for theoretical guarantees that a guest can't abuse this
>> to lock up a CPU. There are no way to guarantee that as multiple vCPUs could
>> call the hypercall and take the same lock potentially delaying significantly the
>> work.
> 
> Well, I may have gone a little too far with my original response. It
> just was so odd to see this pseudo lock used.
> 
>> Regarding the need of the lock, I still can't see how you can make it safe
>> without it as you may have concurrent call.
>>
>> Feel free to suggest a way.
> 
> Well, if none can be found, then fine. I don't have the time or interest
> here to try and think about a lockless approach; it just doesn't _feel_
> like this ought to strictly require use of a lock. This gut feeling of mine
> may well be wrong.

I am not asking you to spend a lot of time on it. But if you have a gut feeling 
this can be done, then a little help would be extremely useful...

Otherwise, I will consider that the lock is the best way to go.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel