Re: add back pending timer irqs for kernel APIC timer

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Dong, Eddie wrote:
> kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org wrote:
>   
>>    Add back pending irqs for apic timer to get precise guest    APIC
>> timer interrupt. 
>>
>>    Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>>
>>     
> a typo, please use this one.
>
> With above patches, now guest timer is much accurate! sleep 60 
> get exactly 60 seconds in host per my rough test.
>
> BTW, AMD platform is not tested.
>
> thx,eddie
>
>
> diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
> index ed6d20a..8867c82 100644
> --- a/drivers/kvm/irq.h
> +++ b/drivers/kvm/irq.h
> @@ -110,7 +110,7 @@ struct kvm_lapic {
>  	unsigned long base_address;
>  	struct kvm_io_device dev;
>  	struct {
> -		unsigned long pending;
> +		atomic_t pending;
>  		s64 period;	/* unit: ns */
>  		u32 divide_count;
>  		ktime_t last_update;
> @@ -153,5 +153,7 @@ int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec,
> u8 trig);
>  void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
>  int kvm_ioapic_init(struct kvm *kvm);
>  void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> +void kvm_pt_intr_post(struct kvm_vcpu *vcpu, int vec);
> +void kvm_pt_update_irq(struct kvm_vcpu *vcpu);
>   

What does "pt" mean in this context? "pending timer"?

Suggested descriptive names below.

> *----------------------------------------------------------------------
>   */
> +
> +/* TODO: make sure __apic_timer_fn runs in current pCPU */
>  static int __apic_timer_fn(struct kvm_lapic *apic)
>  {
> -	u32 vector;
>  	int result = 0;
>  
> -	if (unlikely(!apic_enabled(apic) ||
> -		     !apic_lvt_enabled(apic, APIC_LVTT))) {
> -		apic_debug("%s: time interrupt although apic is down\n",
> -			   __FUNCTION__);
> -		return 0;
> -	}
> -
> -	vector = apic_lvt_vector(apic, APIC_LVTT);
> -	apic->timer.last_update = apic->timer.dev.expires;
> -	apic->timer.pending++;
> -	__apic_accept_irq(apic, APIC_DM_FIXED, vector, 1, 0);
> -
> +	atomic_inc (&apic->timer.pending);
>   

Extra space.

>  	if (apic_lvtt_period(apic)) {
> -		u32 offset;
> -		u32 tmict = apic_get_reg(apic, APIC_TMICT);
> -
> -		offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count *
> tmict;
> -
>  		result = 1;
>  		apic->timer.dev.expires = ktime_add_ns(
>  					apic->timer.dev.expires,
>  					apic->timer.period);
>  	}
> -
>  	return result;
>  }
>   

While this improves throughput, doesn't it decrease responsiveness?  
Suppose the guest is sleeping and there is no activity except for lapic 
interrupts.  Won't the wakeup get deferred indefinitely?

>  
> +void kvm_pt_update_irq(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->apic;
> +
> +	if (apic && atomic_read(&apic->timer.pending) > 0) {
>   

Extra braces.  Also this smells like a race.  What if timer.pending is 
updated just after this?

> +		if (inject_apic_timer_irq(apic))
> +			atomic_dec(&apic->timer.pending);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(kvm_pt_update_irq);
>   

How about "kvm_inject_pending_timer_irqs"

> +
> +void kvm_pt_intr_post(struct kvm_vcpu *vcpu, int vec)
> +{
> +	struct kvm_lapic *apic = vcpu->apic;
> +
> +	if (apic && apic_lvt_vector(apic, APIC_LVTT) == vec)
> +		apic->timer.last_update = ktime_add_ns(
> +				apic->timer.last_update,
> +				apic->timer.period);
> +}
> +EXPORT_SYMBOL_GPL(kvm_pt_intr_post);
>   

kvm_update_pending_timer_irq?



-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/