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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.06.19 at 15:14, <julien.grall@xxxxxxx> wrote:
> On 13/06/2019 13:58, Jan Beulich wrote:
>>>>> On 13.06.19 at 14:48, <julien.grall@xxxxxxx> wrote:
>>> On 13/06/2019 13:41, Jan Beulich wrote:
>>>>>>> On 13.06.19 at 14:32, <andrii.anisov@xxxxxxxxx> wrote:
>>>>> 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.

For one I initially (wrongly) understood you want the trylock in the
hypercall handler. And then, for context switch, wasting the target
(i.e. being switched in) vCPU's time slice is not an issue here. Of
course if there's a chance that acquiring the lock could require more
than a full time slice, then yes, some try-lock-ery may be needed.

However, ...

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

... I thought I had already outlined a model: Allow cross-vCPU updates
only while the target vCPU is still offline. Once online, a vCPU can only
itself update its runstate area address. I think you can get away
without any locks in this case; there may be a corner case with a vCPU
being onlined right at that point in time, so there may need to be a more
strict condition (like "only one online vCPU" instead of "the target vCPU
is offline").

Jan



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