Re: Remove APIC lock

Dong, Eddie
2007-08-25T15:25:29Z

Avi Kivity wrote:
> Dong, Eddie wrote:
>>> What about preemption:
>>>
>>> - vcpu executes lapic code in qemu process context
>>>
>>
>> Don't understand. LAPIC is in kernel, how can qemu access?
>> If you mean qemu is calling APIC KVM syscall, then it already
>> disabled preemption & take kvm->lock.
>>
>>
>>
>
> I meant qemu executes the KVM_VCPU_RUN ioctl. kvm->lock does not
> disable preemption (it is a mutex).

Just noticed it is changed to mutex, but seems same here :-)
If the process is switched to other task, it is OK since it won't access
local
APIC. Current VP access to APIC will take the mutex first (see below).
Or you are talking other corner case?

>
> Do we really take kvm->lock for local accesses? That's a significant
> problem, much more than the timer.

Today all APIC/IOAPIC access comes from shadow page fault which already
take kvm->lock. KVM_IRQ_LINE will take too. (just noticed the
save/restore part
missed this one, will add later if we agree here). PIC access comes from
kernel_pio which takes the mutex too.

Another missing place is vmx_intr_assist which needs to take the mutex
too.
Will add later.

>
>>> - process is preempted
>>> - timer fires, touches lapic code
>>>
>>
>> See above.
>>
>>
>>> Furthermore, I question the necessity of this. Taking a spinlock
>>> is a couple dozen cycles on modern processors. Entering the guest
>>> is a couple thousand. So what are we saving?
>>>
>>
>> I am stuck, can u explain more? We didn't enter guest more than
>> before.
>>
>>
>
> What I'm saying is that the performance improvement is negligible,
> because the VT switch dominates the time. Taking the lock is fast in
> comparison.
>
> However I do like the simplification that removing the lock brings.

Yes, I do this for simplification only, I didn't expect any performance
difference.
I met recursive lock taking case in debug, so just want to revisit it
now.

>
>>> (migrating the timer is a good thing though).
>>>
>>> A different approach might be to wake up the vcpu like we do for
>>> irr, with kvm_vcpu_kick(), and let the timer code be handled in
>>> process context, so no locks need be taken.
>>>
>>>
>>
>> This can be too, but will that be much efficient than timer
>> migration? Usually the guest APIC timer is around 1ms, which means
>> we have
>> to VM Exit guest 1000HZ with cost of thousands of cycles each time.
>> While the process migration can only happen at scheduler quantum
>> (10ms?) and highly depend on the scheduler policy. I guess the cost
>> of hrtimer migration won;t exceed 1000 cycles in modern processor.
>> And today the migration cost is already exceeds hundreds of cycles,
>> add this additional one won;t change many. So I think the cost using
>> hrtimer migration is much cheap than kvm_vcpu_kick(), but I may miss
>> something.
>>
>>
>
> I meant in addition to timer migration (I really like the timer
> migration part -- it's much more important than lock removal for
> performance). kvm_vcpu_kick() is needed to wake up from halt, or if we
> have races between the timer and task migration.

:-) Actually we have solved this issue in previous patch and this one
naturally.
In adding back APIC timer IRQ patch, we will wakeup the halt vCPU.

In this patch since hrtimer always run in same pCPU with guest VP
(when VP is active), each time when hrtime fires (comes from a hardware
IRQ),
it already VM Exit to kernel (similar function with kvm_vcpu_kick but
no need to explicitly call it) and then we do IRQ injection at
vmx_intr_assist time.

>
> I'm thinking about something like the remote tlb flush logic:
>
> - hrtimer callback sets a bit in vcpu->requests and calls
> kvm_vcpu_kick().
> - if the hrtimer and the vcpu are co-located, kvm_vcpu_kick() does
> nothing
> - if the guest is running on another cpu, it sends an IPI
> - if the vcpu is halted, it wakes it up
> - when we reenter the guest again, we examine vcpu->requests; if
> KVM_REQ_LAPIC is set we know the timer expired and we run the
> lapic code
> (where we can examine the pending count).
> - we also need to update hlt sleep to look at vcpu->requests
>
> So, to summarize:
> - timer migration always helps, regardless of what we do
> - lock removal helps, IMO, not performance, but stability and
> maintainablity
> - kvm is preemptible except in vcpu_load() and guest entry, so
> if we do
> lock removal we need to do everything in task context (nothing in
> interrupts except raising bits like irr)
>
>
> Opinions?