Hi Jan,
On 13/06/2019 13:58, Jan Beulich wrote:
On 13.06.19 at 14:48, <julien.gr...@arm.com> wrote:
Hi Jan,
On 13/06/2019 13:41, Jan Beulich wrote:
On 13.06.19 at 14:32, <andrii.ani...@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
