Re: [PATCH 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Zachary Amsden <zamsden@redhat.com>]

On 06/11/2010 06:15 PM, Yang, Sheng wrote:
> On Saturday 12 June 2010 01:35:23 Marcelo Tosatti wrote:
>    
>> On Thu, Jun 10, 2010 at 04:44:05PM -0400, Chris Lalancette wrote:
>>      
>>> We really want to "kvm_set_irq" during the hrtimer callback,
>>> but that is risky because that is during interrupt context.
>>> Instead, offload the work to a workqueue, which is a bit safer
>>> and should provide most of the same functionality.
>>>
>>> Signed-off-by: Chris Lalancette<clalance@redhat.com>
>>> ---
>>>
>>>   arch/x86/kvm/i8254.c |  117
>>>   ++++++++++++++++++++++++++++---------------------- arch/x86/kvm/i8254.h
>>>   |    4 +-
>>>   arch/x86/kvm/irq.c   |    1 -
>>>   3 files changed, 69 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>>> index 188d827..99c7472 100644
>>> --- a/arch/x86/kvm/i8254.c
>>> +++ b/arch/x86/kvm/i8254.c
>>> @@ -34,6 +34,7 @@
>>>
>>>   #include<linux/kvm_host.h>
>>>   #include<linux/slab.h>
>>>
>>> +#include<linux/workqueue.h>
>>>
>>>   #include "irq.h"
>>>   #include "i8254.h"
>>>
>>> @@ -244,11 +245,11 @@ static void kvm_pit_ack_irq(struct
>>> kvm_irq_ack_notifier *kian)
>>>
>>>   {
>>>
>>>   	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>>>   	
>>>   						 irq_ack_notifier);
>>>
>>> -	raw_spin_lock(&ps->inject_lock);
>>> +	spin_lock(&ps->inject_lock);
>>>
>>>   	if (atomic_dec_return(&ps->pit_timer.pending)<  0)
>>>   	
>>>   		atomic_inc(&ps->pit_timer.pending);
>>>   	
>>>   	ps->irq_ack = 1;
>>>
>>> -	raw_spin_unlock(&ps->inject_lock);
>>> +	spin_unlock(&ps->inject_lock);
>>>
>>>   }
>>>
>>>   void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
>>>
>>> @@ -281,6 +282,58 @@ static struct kvm_timer_ops kpit_ops = {
>>>
>>>   	.is_periodic = kpit_is_periodic,
>>>
>>>   };
>>>
>>> +static void pit_do_work(struct work_struct *work)
>>> +{
>>> +       struct kvm_pit *pit = container_of(work, struct kvm_pit,
>>> expired); +       struct kvm *kvm = pit->kvm;
>>> +       struct kvm_vcpu *vcpu;
>>> +       int i;
>>> +       struct kvm_kpit_state *ps =&pit->pit_state;
>>> +       int inject = 0;
>>> +
>>> +       /* Try to inject pending interrupts when
>>> +        * last one has been acked.
>>> +        */
>>> +       spin_lock(&ps->inject_lock);
>>> +       if (ps->irq_ack) {
>>> +               ps->irq_ack = 0;
>>> +               inject = 1;
>>> +       }
>>> +       spin_unlock(&ps->inject_lock);
>>> +       if (inject) {
>>> +               kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
>>> +               kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
>>> +
>>> +               /*
>>> +                * Provides NMI watchdog support via Virtual Wire mode.
>>> +                * The route is: PIT ->  PIC ->  LVT0 in NMI mode.
>>> +                *
>>> +                * Note: Our Virtual Wire implementation is simplified,
>>> only +                * propagating PIT interrupts to all VCPUs when
>>> they have set +                * LVT0 to NMI delivery. Other PIC
>>> interrupts are just sent to +                * VCPU0, and only if its
>>> LVT0 is in EXTINT mode. +                */
>>> +               if (kvm->arch.vapics_in_nmi_mode>  0)
>>> +                       kvm_for_each_vcpu(i, vcpu, kvm)
>>> +                               kvm_apic_nmi_wd_deliver(vcpu);
>>> +       }
>>> +}
>>> +
>>> +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
>>> +{
>>> +	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
>>> +	struct kvm_pit *pt = ktimer->kvm->arch.vpit;
>>> +
>>> +	queue_work(pt->wq,&pt->expired);
>>>        
>> So this disables interrupt reinjection. Older RHEL3 guests do not
>> compensate for lost ticks, and as such are likely to drift without
>> it (but RHEL3 is EOL, should one care?).
>>
>> Are there other guests which rely on PIT reinjection, or is it OK
>> to remove it completly?
>>      
> IIRC, the old kernel *does* compensate ticks, so we need disable reinjection. And
> the latest kernel doesn't do this, so we have to do reinjection.
>    

It looks like 2.4.21 has lost tick compensation for x86_64, but not for 
i386, so the reinjection is still needed.