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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jun 14, 2010 at 01:11:20PM -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 |  125 ++++++++++++++++++++++++++++---------------------
>  arch/x86/kvm/i8254.h |    4 +-
>  arch/x86/kvm/irq.c   |    1 -
>  3 files changed, 74 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..3bed8ac 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)
> @@ -267,7 +268,8 @@ void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
>  static void destroy_pit_timer(struct kvm_timer *pt)
>  {
>  	pr_debug("execute del timer!\n");
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
>  }
>  
>  static bool kpit_is_periodic(struct kvm_timer *ktimer)
> @@ -281,6 +283,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;
> +
> +	if (ktimer->reinject)
> +		queue_work(pt->wq, &pt->expired);
If ktimer->reinject is set to false by userspace pit irq will never be
delivered or do I missing something here?

May be we should consider using return value from kvm_set_irq() for
coalescing detection to simplify things. I once had patch for that.

> +
> +	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;
> @@ -291,14 +345,14 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  	pr_debug("create pit timer, interval is %llu nsec\n", interval);
>  
>  	/* TODO The new value only affected after the retriggered */
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
>  	pt->period = interval;
>  	ps->is_periodic = is_period;
>  
> -	pt->timer.function = kvm_timer_fn;
> +	pt->timer.function = pit_timer_fn;
>  	pt->t_ops = &kpit_ops;
>  	pt->kvm = ps->pit->kvm;
> -	pt->vcpu = pt->kvm->bsp_vcpu;
>  
>  	atomic_set(&pt->pending, 0);
>  	ps->irq_ack = 1;
> @@ -626,7 +680,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  
>  	mutex_init(&pit->pit_state.lock);
>  	mutex_lock(&pit->pit_state.lock);
> -	raw_spin_lock_init(&pit->pit_state.inject_lock);
> +	spin_lock_init(&pit->pit_state.inject_lock);
> +
> +	pit->wq = create_singlethread_workqueue("kvm-pit-wq");
> +	if (!pit->wq) {
> +		kfree(pit);
> +		return NULL;
> +	}
> +	INIT_WORK(&pit->expired, pit_do_work);
>  
>  	kvm->arch.vpit = pit;
>  	pit->kvm = kvm;
> @@ -684,55 +745,11 @@ void kvm_free_pit(struct kvm *kvm)
>  				&kvm->arch.vpit->pit_state.irq_ack_notifier);
>  		mutex_lock(&kvm->arch.vpit->pit_state.lock);
>  		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
> -		hrtimer_cancel(timer);
> +		if (hrtimer_cancel(timer))
> +			cancel_work_sync(&kvm->arch.vpit->expired);
>  		kvm_free_irq_source_id(kvm, kvm->arch.vpit->irq_source_id);
>  		mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> +		destroy_workqueue(kvm->arch.vpit->wq);
>  		kfree(kvm->arch.vpit);
>  	}
>  }
> -
> -static void __inject_pit_timer_intr(struct kvm *kvm)
> -{
> -	struct kvm_vcpu *vcpu;
> -	int i;
> -
> -	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);
> -}
> -
> -void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
> -{
> -	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
> -	struct kvm *kvm = vcpu->kvm;
> -	struct kvm_kpit_state *ps;
> -
> -	if (pit) {
> -		int inject = 0;
> -		ps = &pit->pit_state;
> -
> -		/* Try to inject pending interrupts when
> -		 * last one has been acked.
> -		 */
> -		raw_spin_lock(&ps->inject_lock);
> -		if (atomic_read(&ps->pit_timer.pending) && ps->irq_ack) {
> -			ps->irq_ack = 0;
> -			inject = 1;
> -		}
> -		raw_spin_unlock(&ps->inject_lock);
> -		if (inject)
> -			__inject_pit_timer_intr(kvm);
> -	}
> -}
> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> index 900d6b0..46d08ca 100644
> --- a/arch/x86/kvm/i8254.h
> +++ b/arch/x86/kvm/i8254.h
> @@ -27,7 +27,7 @@ struct kvm_kpit_state {
>  	u32    speaker_data_on;
>  	struct mutex lock;
>  	struct kvm_pit *pit;
> -	raw_spinlock_t inject_lock;
> +	spinlock_t inject_lock;
>  	unsigned long irq_ack;
>  	struct kvm_irq_ack_notifier irq_ack_notifier;
>  };
> @@ -40,6 +40,8 @@ struct kvm_pit {
>  	struct kvm_kpit_state pit_state;
>  	int irq_source_id;
>  	struct kvm_irq_mask_notifier mask_notifier;
> +	struct workqueue_struct *wq;
> +	struct work_struct expired;
>  };
>  
>  #define KVM_PIT_BASE_ADDRESS	    0x40
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 0f4e488..2095a04 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -90,7 +90,6 @@ EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
>  {
>  	kvm_inject_apic_timer_irqs(vcpu);
> -	kvm_inject_pit_timer_irqs(vcpu);
>  	/* TODO: PIT, RTC etc. */
>  }
>  EXPORT_SYMBOL_GPL(kvm_inject_pending_timer_irqs);
> -- 
> 1.6.6.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.