>-----Original Message-----
>From: Avi Kivity [mailto:[EMAIL PROTECTED]
>Sent: 2007年9月6日 17:47
>To: He, Qing
>Cc: [email protected]
>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when
>inactive
>
>Avi Kivity wrote:
>> He, Qing wrote:
>>> When local apic timer is inactive or is expired in oneshot
>>> mode, it
>>> should not be restarted in save/restore or hrtimer migration. This
>>> patch fixes this.
>>>
>>> diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
>>> index 5f97e25..68d454c 100644
>>> --- a/drivers/kvm/irq.h
>>> +++ b/drivers/kvm/irq.h
>>> @@ -110,6 +110,7 @@ struct kvm_lapic {
>>> struct kvm_io_device dev;
>>> struct {
>>> atomic_t pending;
>>> + atomic_t active;
>>>
>>
>> This is atomic, but you never use read-modify-write instructions (read
>> and write are atomic on a simple int).
>>
>>> +
>>> + if (atomic_read(&apic->timer.active))
>>>
>>
>> What if the timer fires here?
>>
>>> + apic_set_reg(apic, APIC_TMCCT, tmict);
>>> + else
>>> + apic_set_reg(apic, APIC_TMCCT, 0);
>>> +}
>>> +
>>> void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>> @@ -1036,11 +1062,14 @@ void kvm_migrate_apic_timer(struct kvm_vcpu
>>> *vcpu)
>>> struct kvm_lapic *apic = vcpu->apic;
>>> struct hrtimer *timer;
>>>
>>> - if (apic) {
>>> - timer = &apic->timer.dev;
>>> - hrtimer_cancel(timer);
>>> - hrtimer_start(timer, timer->expires, HRTIMER_MODE_ABS);
>>> - }
>>> + if (!apic)
>>> + return;
>>> +
>>> + timer = &apic->timer.dev;
>>> + hrtimer_cancel(timer);
>>> + if (atomic_read(&apic->timer.active))
>>>
>>
>> Or here?
>>
>>> + hrtimer_start(timer, timer->expires,
>>> + HRTIMER_MODE_ABS);
>>> }
>>> EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
>>>
>>>
>>
>
>I think you could use the return value of hrtimer_cancel() here:
>
> if (hrtimer_cancel(...))
> hrtimer_start(...);
Thanks for commenting, previously I have thought of something like:
if (hrtimer_active(...)) {
hrtimer_cancel(...);
hrtimer_start(...);
}
But it is not able to handle pending state this way. Seems the return value of
hrtimer_cancel is fine. Another question is, when the hrtimer function returns
HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>
>
>
>--
>Any sufficiently difficult bug is indistinguishable from a feature.
-------------------------------------------------------------------------
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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
