On 06/04/2017 22:14, Christoffer Dall wrote:
>> So it seems like there are no races after all in KVM/ARM code
>
> No races after Drew's fix has been applied to set vcpu->mode =
> IN_GUEST_MODE, before checking the pause flag, correct? (I think that's
> what the spin model below is modeling).
Yes. All of them include the vcpu->mode = IN_GUEST_MODE assignment and
(implicitly because spin is sequentially consistent) the memory barrier.
>> After this experiment, I think I like Drew's KVM_REQ_PAUSE more than I did
>> yesterday. However, yet another alternative is to leave pause/power_off as
>> they are, while taking some inspiration from his patch to do some cleanups:
>>
>> 1) change the "if"
>>
>> if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> vcpu->arch.power_off || vcpu->arch.pause) {
>>
>> to test kvm_requests_pending instead of pause/power_off
>>
>> 2) clear KVM_REQ_VCPU_EXIT before the other "if":
>>
>> if (vcpu->arch.power_off || vcpu->arch.pause)
>> vcpu_sleep(vcpu);
>
> I like using requests as only requests from one thread to the VCPU
> thread, and not to maintain specific state about a VCPU.
>
> The benefit of Drew's approach is that since these pieces of state are
> boolean, you can have just a single check in the critical path in the
> run loop instead of having to access multiple fields.
I think you'd still need two checks for KVM_REQ_PAUSE/KVM_REQ_POWEROFF:
one to check whether to sleep, and one to check whether to abort the
vmentry.
Pause and power_off could be merged into a single bitmask if necessary, too.
Paolo
