Re: Remove APIC lock

Dong, Eddie
2007-08-27T01:54:49Z

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Avi Kivity wrote:
>>
>> For this situation, even without preemption, the problem is still
>> there. But maybe you are refering the old code, the latest code is
>> already preemption free since the apic_timer_fn didn't change any
>> APIC state. It only increase apic->timer.pending.
>>
>>
>
> I see the following:
>
>> static int __apic_timer_fn(struct kvm_lapic *apic)
>> {
>> int result = 0;
>> wait_queue_head_t *q = &apic->vcpu->wq;
>>
>> atomic_inc(&apic->timer.pending);
>> if (waitqueue_active(q))
>> wake_up_interruptible(q);
>> if (apic_lvtt_period(apic)) {
>> result = 1;
>> apic->timer.dev.expires = ktime_add_ns(
>> apic->timer.dev.expires,
>> apic->timer.period);
>> } return result;
>> }
>
> So, timer.dev.expires is protected by hrtimer internal locking?

Not sure, but apic->timer.dev.expires is only used in the call back
function.
So we are ok here.

>
> Tricky, but it should work.
>>
>> When CPU 1 do hrtimer migration, hrtimer_cancel will wait for
>> an in-flying timer be completed and then remove it. see
>> hrtimer_try_to_cancel.
>>
>
> Okay.
>
> I'm satisfied that it's safe now. I'll add some comments
> later and commit.
>

Thanks, I will try to fix the potential leak hole, i.e. vmx_intr_assist
need to take the mutex of both PIC & APIC.

For the issue Greg raised, i.e. for in kernel pv driver IRQ injection,
probably we can leave to Greg and Dor who are working on pv drivers :-)
kvm->lock and vcou->mutex both are too big, but current apic->lock
(before
this patch) is too small and didn't solve the whole problem.

Thx, Eddie