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

["Yang, Sheng" <sheng.yang@intel.com>]

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.

So we can't disable reinjection anyway.

BTW: The patch has some coding style issues, suggest using scripts/checkpatch.pl 
to check it.

--
regards
Yang, Sheng

> 
> > +
> > +	if (ktimer->t_ops->is_periodic(ktimer)) {
> > +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> > +		return HRTIMER_RESTART;
> > +	}
> > +	else
> > +		return HRTIMER_NORESTART;
> > +}
> 
> Also need to cancel the pending work whenever the current code cancels
> the hrtimer (destroy_pit_timer, etc).