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

[Zachary Amsden <zamsden@redhat.com>]

On 06/08/2010 07:55 AM, 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 |  106 ++++++++++++++++++++++++++++++-------------------
>   arch/x86/kvm/i8254.h |    4 ++
>   arch/x86/kvm/irq.c   |    1 -
>   arch/x86/kvm/x86.c   |    4 ++
>   4 files changed, 73 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..dd655ef 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"
> @@ -49,6 +50,8 @@
>   #define RW_STATE_WORD0 3
>   #define RW_STATE_WORD1 4
>
> +static struct workqueue_struct *pit_wq;
> +
>   /* Compute with 96 bit intermediate result: (a*b)/c */
>   static u64 muldiv64(u64 a, u32 b, u32 c)
>   {
> @@ -281,6 +284,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.
> +        */
> +       raw_spin_lock(&ps->inject_lock);
> +       if (ps->irq_ack) {
> +               ps->irq_ack = 0;
> +               inject = 1;
> +       }
> +       raw_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(pit_wq,&pt->expired);
> +
> +       if (ktimer->t_ops->is_periodic(ktimer)) {
> +               hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> +              return HRTIMER_RESTART;
> +       }
> +       else
> +              return HRTIMER_NORESTART;
> +}
> +
>   static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>   {
>   	struct kvm_timer *pt =&ps->pit_timer;
> @@ -295,10 +350,9 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>   	pt->period = interval;
>   	ps->is_periodic = is_period;
>
> -	pt->timer.function = kvm_timer_fn;
> +	pt->timer.function = pit_timer_fn;
>
>    

I am happy to see this.  I thought kvm_timer_fn was a step backwards; it 
was too general of a function to justify the savings of 20 some odd 
lines of code.

Is there any chance that using a workqueue might help the problem of 
hrtimers firing too quickly?  I wanted to return HR_NORESTART from 
pit_timer_fn always, then restart the hrtimer on delivery, but because 
of unreliable delivery, it wasn't clear how to do that.

Perhaps the workqueue can be used to restart the timer instead, avoiding 
problems of impossibly small timeouts causing hrtimers to run amok.

Zach