[Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-03 14:43, Ulrich Obergfell wrote:
> 
> Hi,
> 
> I am observing severe backward time drift in a MS Windows Vista(tm)
> guest running on a Fedora 14 KVM host. I can reproduce the problem
> with the following steps:
> 
> 1. Use 'vncviewer' to connect to the guest's desktop.
> 2. Click on the menu title bar of a window on the guest's desktop.
> 3. Move that window around on the guest's desktop.
> 
> While I keep on moving the window around for one minute, the guest
> time falls up to 15 seconds behind host time.
> 
> The problem is caused by delayed callbacks of hpet_timer(). A timer
> interrupt is injected into the guest during each callback. However,
> interrupts are lost if delays are greater than a comparator period.
> 

Yes, that's a well known limitation of qemu, in fact. We are lacking a
generic irq coalescing infrastructure. That, once designed and
available, would also allow to fix the HPET.

> 
> This is an RFC through which I would like to get feedback on how the
> idea of a patch to compensate those lost interrupts would be received:
> 
> The patch determines the number of lost timer interrupts based on the
> number of elapsed comparator periods. Lost interrupts are compensated

That neglects coalescing of the HPET IRQs: If the timer is run regularly
but the guest is not able to retrieve the injected IRQs, you should
still see drifts with your patches.

> by gradually injecting additional interrupts during the subsequent
> timer intervals, starting at a rate of one additional interrupt per
> interval. If further interrupts are lost while compensation is still
> in progress, the rate is increased. The algorithm imposes a limit on
> the rate and on the 'backlog' of lost interrupts to be injected. The
> patch can be enabled via a qemu command line option.
> 
>   -hpet [device=none|present][,driftfix=none|slew]
> 
> The 'device=none' option is equivalent to the '-no-hpet' option, and
> the 'driftfix=slew' option enables the patch (similar to RTC).
> 
> 
> The second and third part of this series of email contain the patch:
> 
> - Code part 1 introduces the qemu command line option.
> - Code part 2 implements compensation of lost interrupts.
> 
> Please review and please comment.
> 

Generally, this issue needs to be attacked at qemu level (added to CC),
not qemu-kvm.

We had a lengthy discussion on the list last year. We (including qemu
people) basically agreed that we needs a generic feedback infrastructure
to track coalesced IRQs for periodic, clock providing devices to allow
reinjection (which would include reinjection of completely missed timer
events like in your series).

However, there was one unsolved design issue remain IIRC:

http://thread.gmane.org/gmane.comp.emulators.qemu/73181

Once we have a proper answer for this, we can resume creating the
de-coalescing framework.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/03/2011 09:28 AM, Jan Kiszka wrote:
> On 2011-02-03 14:43, Ulrich Obergfell wrote:
>    
>> Hi,
>>
>> I am observing severe backward time drift in a MS Windows Vista(tm)
>> guest running on a Fedora 14 KVM host. I can reproduce the problem
>> with the following steps:
>>
>> 1. Use 'vncviewer' to connect to the guest's desktop.
>> 2. Click on the menu title bar of a window on the guest's desktop.
>> 3. Move that window around on the guest's desktop.
>>
>> While I keep on moving the window around for one minute, the guest
>> time falls up to 15 seconds behind host time.
>>
>> The problem is caused by delayed callbacks of hpet_timer(). A timer
>> interrupt is injected into the guest during each callback. However,
>> interrupts are lost if delays are greater than a comparator period.
>>
>>      
> Yes, that's a well known limitation of qemu, in fact. We are lacking a
> generic irq coalescing infrastructure. That, once designed and
> available, would also allow to fix the HPET.
>    

I don't think it requires anything that sophisticated.

It's just the period calculation of the HPET that's wrong and doesn't 
account for loss.

>    
>> This is an RFC through which I would like to get feedback on how the
>> idea of a patch to compensate those lost interrupts would be received:
>>
>> The patch determines the number of lost timer interrupts based on the
>> number of elapsed comparator periods. Lost interrupts are compensated
>>      
> That neglects coalescing of the HPET IRQs: If the timer is run regularly
> but the guest is not able to retrieve the injected IRQs, you should
> still see drifts with your patches.
>    

FWIW, this isn't the most common failure scenario.  This is only really 
prominent when you have rapid reinject like we do with the in-kernel 
PIT.  This generally shouldn't be an issue with gradual reinjection.

>> by gradually injecting additional interrupts during the subsequent
>> timer intervals, starting at a rate of one additional interrupt per
>> interval. If further interrupts are lost while compensation is still
>> in progress, the rate is increased. The algorithm imposes a limit on
>> the rate and on the 'backlog' of lost interrupts to be injected. The
>> patch can be enabled via a qemu command line option.
>>
>>    -hpet [device=none|present][,driftfix=none|slew]
>>
>> The 'device=none' option is equivalent to the '-no-hpet' option, and
>> the 'driftfix=slew' option enables the patch (similar to RTC).
>>
>>
>> The second and third part of this series of email contain the patch:
>>
>> - Code part 1 introduces the qemu command line option.
>> - Code part 2 implements compensation of lost interrupts.
>>
>> Please review and please comment.
>>
>>      
> Generally, this issue needs to be attacked at qemu level (added to CC),
> not qemu-kvm.
>
> We had a lengthy discussion on the list last year. We (including qemu
> people) basically agreed that we needs a generic feedback infrastructure
> to track coalesced IRQs for periodic, clock providing devices to allow
> reinjection (which would include reinjection of completely missed timer
> events like in your series).
>    

This really isn't the main problem.

Regards,

Anthony Liguori

> However, there was one unsolved design issue remain IIRC:
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/73181
>
> Once we have a proper answer for this, we can resume creating the
> de-coalescing framework.
>
> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

On 2011-02-03 21:07, Anthony Liguori wrote:
> On 02/03/2011 09:28 AM, Jan Kiszka wrote:
>> On 2011-02-03 14:43, Ulrich Obergfell wrote:
>>   
>>> Hi,
>>>
>>> I am observing severe backward time drift in a MS Windows Vista(tm)
>>> guest running on a Fedora 14 KVM host. I can reproduce the problem
>>> with the following steps:
>>>
>>> 1. Use 'vncviewer' to connect to the guest's desktop.
>>> 2. Click on the menu title bar of a window on the guest's desktop.
>>> 3. Move that window around on the guest's desktop.
>>>
>>> While I keep on moving the window around for one minute, the guest
>>> time falls up to 15 seconds behind host time.
>>>
>>> The problem is caused by delayed callbacks of hpet_timer(). A timer
>>> interrupt is injected into the guest during each callback. However,
>>> interrupts are lost if delays are greater than a comparator period.
>>>
>>>      
>> Yes, that's a well known limitation of qemu, in fact. We are lacking a
>> generic irq coalescing infrastructure. That, once designed and
>> available, would also allow to fix the HPET.
>>    
> 
> I don't think it requires anything that sophisticated.
> 
> It's just the period calculation of the HPET that's wrong and doesn't
> account for loss.

Blind (/wrt the guest state) reinjection from the iothread will
compensate for lost time of *that* thread but not of the target vcpu(s).
So, depending on your workload, you may reduce the drift more or less,
but you won't fix it this way.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/03/2011 03:24 PM, Jan Kiszka wrote:
> On 2011-02-03 21:07, Anthony Liguori wrote:
>    
>> On 02/03/2011 09:28 AM, Jan Kiszka wrote:
>>      
>>> On 2011-02-03 14:43, Ulrich Obergfell wrote:
>>>
>>>        
>>>> Hi,
>>>>
>>>> I am observing severe backward time drift in a MS Windows Vista(tm)
>>>> guest running on a Fedora 14 KVM host. I can reproduce the problem
>>>> with the following steps:
>>>>
>>>> 1. Use 'vncviewer' to connect to the guest's desktop.
>>>> 2. Click on the menu title bar of a window on the guest's desktop.
>>>> 3. Move that window around on the guest's desktop.
>>>>
>>>> While I keep on moving the window around for one minute, the guest
>>>> time falls up to 15 seconds behind host time.
>>>>
>>>> The problem is caused by delayed callbacks of hpet_timer(). A timer
>>>> interrupt is injected into the guest during each callback. However,
>>>> interrupts are lost if delays are greater than a comparator period.
>>>>
>>>>
>>>>          
>>> Yes, that's a well known limitation of qemu, in fact. We are lacking a
>>> generic irq coalescing infrastructure. That, once designed and
>>> available, would also allow to fix the HPET.
>>>
>>>        
>> I don't think it requires anything that sophisticated.
>>
>> It's just the period calculation of the HPET that's wrong and doesn't
>> account for loss.
>>      
> Blind (/wrt the guest state) reinjection from the iothread will
> compensate for lost time of *that* thread but not of the target vcpu(s).
>    

Is the case your concern about where you try to set an interrupt from 
the I/O thread and the VCPU is not scheduled such that it doesn't 
actually get injected into KVM until after the period is over?

This should be a rare event.  If you are missing 50% of your 
notifications, not amount of gradual catchup is going to help you out.

> So, depending on your workload, you may reduce the drift more or less,
> but you won't fix it this way.
>    

There is no such thing as "fix" it.  Time drift can happen on bare metal 
too.  Interrupts can be coalesced due to crappy SMM code.  It's 
something we see quite a lot in practice.

My point is that there's really low hanging fruit and while for some 
curious reason I don't actually see this patch, I believe that a patch 
like this probably can help us quite a lot in the short term.

The think the two biggest problems we have right now are bad period 
calculations due to sloppy unit conversion (PIT/RTC) and lack of 
accounting for missed periods.

It's worth noting again that if you don't use a gradual catchup policy, 
interrupt notifiers are extremely important because you're not going to 
inject before the end of the interrupt window.   However, with a gradual 
policy, it shouldn't be a huge issue.

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

On 2011-02-04 03:06, Anthony Liguori wrote:
> On 02/03/2011 03:24 PM, Jan Kiszka wrote:
>> On 2011-02-03 21:07, Anthony Liguori wrote:
>>   
>>> On 02/03/2011 09:28 AM, Jan Kiszka wrote:
>>>     
>>>> On 2011-02-03 14:43, Ulrich Obergfell wrote:
>>>>
>>>>       
>>>>> Hi,
>>>>>
>>>>> I am observing severe backward time drift in a MS Windows Vista(tm)
>>>>> guest running on a Fedora 14 KVM host. I can reproduce the problem
>>>>> with the following steps:
>>>>>
>>>>> 1. Use 'vncviewer' to connect to the guest's desktop.
>>>>> 2. Click on the menu title bar of a window on the guest's desktop.
>>>>> 3. Move that window around on the guest's desktop.
>>>>>
>>>>> While I keep on moving the window around for one minute, the guest
>>>>> time falls up to 15 seconds behind host time.
>>>>>
>>>>> The problem is caused by delayed callbacks of hpet_timer(). A timer
>>>>> interrupt is injected into the guest during each callback. However,
>>>>> interrupts are lost if delays are greater than a comparator period.
>>>>>
>>>>>
>>>>>          
>>>> Yes, that's a well known limitation of qemu, in fact. We are lacking a
>>>> generic irq coalescing infrastructure. That, once designed and
>>>> available, would also allow to fix the HPET.
>>>>
>>>>        
>>> I don't think it requires anything that sophisticated.
>>>
>>> It's just the period calculation of the HPET that's wrong and doesn't
>>> account for loss.
>>>      
>> Blind (/wrt the guest state) reinjection from the iothread will
>> compensate for lost time of *that* thread but not of the target vcpu(s).
>>    
> 
> Is the case your concern about where you try to set an interrupt from
> the I/O thread and the VCPU is not scheduled such that it doesn't
> actually get injected into KVM until after the period is over?
> 
> This should be a rare event.  If you are missing 50% of your
> notifications, not amount of gradual catchup is going to help you out.

But that's the only thing this patch is after: lost ticks at QEMU level.

> 
>> So, depending on your workload, you may reduce the drift more or less,
>> but you won't fix it this way.
>>    
> 
> There is no such thing as "fix" it.  Time drift can happen on bare metal
> too.  Interrupts can be coalesced due to crappy SMM code.  It's
> something we see quite a lot in practice.

We don't have this issues. Relative to the host's time base, we can
actually prevent lost ticks, thus drift.

> 
> My point is that there's really low hanging fruit and while for some
> curious reason I don't actually see this patch, I believe that a patch
> like this probably can help us quite a lot in the short term.
> 
> The think the two biggest problems we have right now are bad period
> calculations due to sloppy unit conversion (PIT/RTC) and lack of
> accounting for missed periods.

I don't disagree. The question is just how to detect guest-missed ticks.

> 
> It's worth noting again that if you don't use a gradual catchup policy,
> interrupt notifiers are extremely important because you're not going to
> inject before the end of the interrupt window.   However, with a gradual
> policy, it shouldn't be a huge issue.

Again, I don't disagree, but I would like to finally attack this in a
non-ad-hoc manner, specifically when going upstream.

The input & output infrastructure (aka injection feedback), and all
these calculations should be generically available so that we do not
reinvent the wheel every time we add another timer device that guests me
use for time keeping. This issue is surely not an x86-only thing. Even
on x86, we need it also for the PIT and the RTC, conceptually also for
the APIC when configured in periodic mode.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Ulrich Obergfell]

Anthony,

in reply to: 
> My point is that there's really low hanging fruit and while for some
> curious reason I don't actually see this patch, I believe that a patch
> like this probably can help us quite a lot in the short term.

I've sent the patch in two separate emails:

- code part 1 (introduces the qemu command line option)
  http://article.gmane.org/gmane.comp.emulators.kvm.devel/67347

- code part 2 (implements compensation of lost interrupts)
  http://article.gmane.org/gmane.comp.emulators.kvm.devel/67348


Regards,

Uli Obergfell

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Ulrich Obergfell]

On 02/03/2011 9:07 PM, Anthony Liguori wrote:
> On 02/03/2011 09:28 AM, Jan Kiszka wrote:
>> On 2011-02-03 14:43, Ulrich Obergfell wrote:
...
>>> This is an RFC through which I would like to get feedback on how the
>>> idea of a patch to compensate those lost interrupts would be received:
>>>
>>> The patch determines the number of lost timer interrupts based on the
>>> number of elapsed comparator periods. Lost interrupts are compensated
>>>
>> That neglects coalescing of the HPET IRQs: If the timer is run regularly
>> but the guest is not able to retrieve the injected IRQs, you should
>> still see drifts with your patches.
>>
> 
> FWIW, this isn't the most common failure scenario. This is only really
> prominent when you have rapid reinject like we do with the in-kernel
> PIT. This generally shouldn't be an issue with gradual reinjection.
> 
>>> by gradually injecting additional interrupts during the subsequent
>>> timer intervals, starting at a rate of one additional interrupt per
>>> interval. If further interrupts are lost while compensation is still
>>> in progress, the rate is increased. The algorithm imposes a limit on
>>> the rate and on the 'backlog' of lost interrupts to be injected.


Anthony, Jan,

many thanks for your feedback. I'm sorry for not responding earlier.

I used the following KVM kernel module trace point in __apic_accept_irq()
to trace coalesced interrupts related to HPET timer 0:

        result = !apic_test_and_set_irr(vector, apic);
        trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
                                  trig_mode, vector, !result);

While running the test outlined in my original email, I observed between
5 and 15 of these events occurring during one minute:

 qemu-system-x86-7632  [000]  3755.888909: kvm_apic_accept_irq: apicid 0
 vec 209 (Fixed|edge) (coalesced)

kvm->arch->vioapic.redirtbl[2] indicated that IRQ 2 was routed to vector
number 209.

With a comparator period of 15.6 milliseconds, the coalesced interrupts
apparently contribute less than 250 milliseconds of drift. However, the
total drift that I observed during one minute was up to 15 seconds.

I think the patch could possibly handle coalesced interrupts too:

- In update_irq(), use a similar method as in RTC emulation to detect
  coalesced interrupts. Return a delivery status to hpet_timer().

- In hpet_timer(), decrement t->irqs_to_inject (number of interrupts
  remaining to inject) only if the interrupt was not coalesced.


Regards,

Uli Obergfell

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/04/2011 10:56 AM, Jan Kiszka wrote:
> >
> >  This should be a rare event.  If you are missing 50% of your
> >  notifications, not amount of gradual catchup is going to help you out.
>
> But that's the only thing this patch is after: lost ticks at QEMU level.

Most lost ticks will happen at the vcpu level.  The iothread has low 
utilization and will therefore be scheduled promptly, whereas the vcpu 
thread may have high utilization and will thus be preempted.  When it is 
preempted for longer than the timer tick, we will see vcpu-level 
coalescing.  All it takes is 2:1 overcommit to see time go half as fast; 
I don't think you'll ever see that on bare metal.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 06:34 AM, Avi Kivity wrote:
> On 02/04/2011 10:56 AM, Jan Kiszka wrote:
>> >
>> >  This should be a rare event.  If you are missing 50% of your
>> >  notifications, not amount of gradual catchup is going to help you 
>> out.
>>
>> But that's the only thing this patch is after: lost ticks at QEMU level.
>
> Most lost ticks will happen at the vcpu level.  The iothread has low 
> utilization and will therefore be scheduled promptly, whereas the vcpu 
> thread may have high utilization and will thus be preempted.  When it 
> is preempted for longer than the timer tick, we will see vcpu-level 
> coalescing.  All it takes is 2:1 overcommit to see time go half as 
> fast; I don't think you'll ever see that on bare metal.

But that's not to say that doing something about lost ticks in QEMU 
isn't still useful.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 03:11 PM, Anthony Liguori wrote:
> On 02/07/2011 06:34 AM, Avi Kivity wrote:
>> On 02/04/2011 10:56 AM, Jan Kiszka wrote:
>>> >
>>> >  This should be a rare event.  If you are missing 50% of your
>>> >  notifications, not amount of gradual catchup is going to help you 
>>> out.
>>>
>>> But that's the only thing this patch is after: lost ticks at QEMU 
>>> level.
>>
>> Most lost ticks will happen at the vcpu level.  The iothread has low 
>> utilization and will therefore be scheduled promptly, whereas the 
>> vcpu thread may have high utilization and will thus be preempted.  
>> When it is preempted for longer than the timer tick, we will see 
>> vcpu-level coalescing.  All it takes is 2:1 overcommit to see time go 
>> half as fast; I don't think you'll ever see that on bare metal.
>
> But that's not to say that doing something about lost ticks in QEMU 
> isn't still useful.
>

If it doesn't solve the majority of the problems it isn't very useful 
IMO.  It's a good first step, but not sufficient for real world use with 
overcommit.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 07:14 AM, Avi Kivity wrote:
> On 02/07/2011 03:11 PM, Anthony Liguori wrote:
>> On 02/07/2011 06:34 AM, Avi Kivity wrote:
>>> On 02/04/2011 10:56 AM, Jan Kiszka wrote:
>>>> >
>>>> >  This should be a rare event.  If you are missing 50% of your
>>>> >  notifications, not amount of gradual catchup is going to help 
>>>> you out.
>>>>
>>>> But that's the only thing this patch is after: lost ticks at QEMU 
>>>> level.
>>>
>>> Most lost ticks will happen at the vcpu level.  The iothread has low 
>>> utilization and will therefore be scheduled promptly, whereas the 
>>> vcpu thread may have high utilization and will thus be preempted.  
>>> When it is preempted for longer than the timer tick, we will see 
>>> vcpu-level coalescing.  All it takes is 2:1 overcommit to see time 
>>> go half as fast; I don't think you'll ever see that on bare metal.
>>
>> But that's not to say that doing something about lost ticks in QEMU 
>> isn't still useful.
>>
>
> If it doesn't solve the majority of the problems it isn't very useful 
> IMO.  It's a good first step, but not sufficient for real world use 
> with overcommit.

Even if we have a way to detect coalescing, we still need to make sure 
we don't lose ticks in QEMU.  So regardless of whether it solves the 
majority of problems, we need this anyway.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

Hi Ulrich,

Mike Roth just posted a mechanism to implement per-device unit tests.  I 
think this is a great example to use for what type of testing we should 
be doing in QEMU.

Please take a look at Mike's series and see if you can do something 
similar for HPET as he's doing for the RTC.

Regards,

Anthony Liguori

On 02/03/2011 02:07 PM, Anthony Liguori wrote:
> On 02/03/2011 09:28 AM, Jan Kiszka wrote:
>> On 2011-02-03 14:43, Ulrich Obergfell wrote:
>>> Hi,
>>>
>>> I am observing severe backward time drift in a MS Windows Vista(tm)
>>> guest running on a Fedora 14 KVM host. I can reproduce the problem
>>> with the following steps:
>>>
>>> 1. Use 'vncviewer' to connect to the guest's desktop.
>>> 2. Click on the menu title bar of a window on the guest's desktop.
>>> 3. Move that window around on the guest's desktop.
>>>
>>> While I keep on moving the window around for one minute, the guest
>>> time falls up to 15 seconds behind host time.
>>>
>>> The problem is caused by delayed callbacks of hpet_timer(). A timer
>>> interrupt is injected into the guest during each callback. However,
>>> interrupts are lost if delays are greater than a comparator period.
>>>
>> Yes, that's a well known limitation of qemu, in fact. We are lacking a
>> generic irq coalescing infrastructure. That, once designed and
>> available, would also allow to fix the HPET.
>
> I don't think it requires anything that sophisticated.
>
> It's just the period calculation of the HPET that's wrong and doesn't 
> account for loss.
>
>>> This is an RFC through which I would like to get feedback on how the
>>> idea of a patch to compensate those lost interrupts would be received:
>>>
>>> The patch determines the number of lost timer interrupts based on the
>>> number of elapsed comparator periods. Lost interrupts are compensated
>> That neglects coalescing of the HPET IRQs: If the timer is run regularly
>> but the guest is not able to retrieve the injected IRQs, you should
>> still see drifts with your patches.
>
> FWIW, this isn't the most common failure scenario.  This is only 
> really prominent when you have rapid reinject like we do with the 
> in-kernel PIT.  This generally shouldn't be an issue with gradual 
> reinjection.
>
>>> by gradually injecting additional interrupts during the subsequent
>>> timer intervals, starting at a rate of one additional interrupt per
>>> interval. If further interrupts are lost while compensation is still
>>> in progress, the rate is increased. The algorithm imposes a limit on
>>> the rate and on the 'backlog' of lost interrupts to be injected. The
>>> patch can be enabled via a qemu command line option.
>>>
>>>    -hpet [device=none|present][,driftfix=none|slew]
>>>
>>> The 'device=none' option is equivalent to the '-no-hpet' option, and
>>> the 'driftfix=slew' option enables the patch (similar to RTC).
>>>
>>>
>>> The second and third part of this series of email contain the patch:
>>>
>>> - Code part 1 introduces the qemu command line option.
>>> - Code part 2 implements compensation of lost interrupts.
>>>
>>> Please review and please comment.
>>>
>> Generally, this issue needs to be attacked at qemu level (added to CC),
>> not qemu-kvm.
>>
>> We had a lengthy discussion on the list last year. We (including qemu
>> people) basically agreed that we needs a generic feedback infrastructure
>> to track coalesced IRQs for periodic, clock providing devices to allow
>> reinjection (which would include reinjection of completely missed timer
>> events like in your series).
>
> This really isn't the main problem.
>
> Regards,
>
> Anthony Liguori
>
>> However, there was one unsolved design issue remain IIRC:
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/73181
>>
>> Once we have a proper answer for this, we can resume creating the
>> de-coalescing framework.
>>
>> Jan
>>
>

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 03:23 PM, Anthony Liguori wrote:
>> If it doesn't solve the majority of the problems it isn't very useful 
>> IMO.  It's a good first step, but not sufficient for real world use 
>> with overcommit.
>
>
> Even if we have a way to detect coalescing, we still need to make sure 
> we don't lose ticks in QEMU.  So regardless of whether it solves the 
> majority of problems, we need this anyway.

Ok, agreed.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Gleb Natapov]

On Mon, Feb 07, 2011 at 07:23:22AM -0600, Anthony Liguori wrote:
> On 02/07/2011 07:14 AM, Avi Kivity wrote:
> >On 02/07/2011 03:11 PM, Anthony Liguori wrote:
> >>On 02/07/2011 06:34 AM, Avi Kivity wrote:
> >>>On 02/04/2011 10:56 AM, Jan Kiszka wrote:
> >>>>>
> >>>>>  This should be a rare event.  If you are missing 50% of your
> >>>>>  notifications, not amount of gradual catchup is going to
> >>>>help you out.
> >>>>
> >>>>But that's the only thing this patch is after: lost ticks at
> >>>>QEMU level.
> >>>
> >>>Most lost ticks will happen at the vcpu level.  The iothread
> >>>has low utilization and will therefore be scheduled promptly,
> >>>whereas the vcpu thread may have high utilization and will
> >>>thus be preempted.  When it is preempted for longer than the
> >>>timer tick, we will see vcpu-level coalescing.  All it takes
> >>>is 2:1 overcommit to see time go half as fast; I don't think
> >>>you'll ever see that on bare metal.
> >>
> >>But that's not to say that doing something about lost ticks in
> >>QEMU isn't still useful.
> >>
> >
> >If it doesn't solve the majority of the problems it isn't very
> >useful IMO.  It's a good first step, but not sufficient for real
> >world use with overcommit.
> 
> Even if we have a way to detect coalescing, we still need to make
> sure we don't lose ticks in QEMU.  So regardless of whether it
> solves the majority of problems, we need this anyway.
> 
Actually it is very strange we lose them. Last time I checked vm_clock
worked in such a way that if ticks were lost due to qemu not been scheduled
for a long time timer callback was repeatedly fired to compensate for
missed wakeups.

--
			Gleb.

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 03:41 PM, Gleb Natapov wrote:
> On Mon, Feb 07, 2011 at 07:23:22AM -0600, Anthony Liguori wrote:
> >  On 02/07/2011 07:14 AM, Avi Kivity wrote:
> >  >On 02/07/2011 03:11 PM, Anthony Liguori wrote:
> >  >>On 02/07/2011 06:34 AM, Avi Kivity wrote:
> >  >>>On 02/04/2011 10:56 AM, Jan Kiszka wrote:
> >  >>>>>
> >  >>>>>   This should be a rare event.  If you are missing 50% of your
> >  >>>>>   notifications, not amount of gradual catchup is going to
> >  >>>>help you out.
> >  >>>>
> >  >>>>But that's the only thing this patch is after: lost ticks at
> >  >>>>QEMU level.
> >  >>>
> >  >>>Most lost ticks will happen at the vcpu level.  The iothread
> >  >>>has low utilization and will therefore be scheduled promptly,
> >  >>>whereas the vcpu thread may have high utilization and will
> >  >>>thus be preempted.  When it is preempted for longer than the
> >  >>>timer tick, we will see vcpu-level coalescing.  All it takes
> >  >>>is 2:1 overcommit to see time go half as fast; I don't think
> >  >>>you'll ever see that on bare metal.
> >  >>
> >  >>But that's not to say that doing something about lost ticks in
> >  >>QEMU isn't still useful.
> >  >>
> >  >
> >  >If it doesn't solve the majority of the problems it isn't very
> >  >useful IMO.  It's a good first step, but not sufficient for real
> >  >world use with overcommit.
> >
> >  Even if we have a way to detect coalescing, we still need to make
> >  sure we don't lose ticks in QEMU.  So regardless of whether it
> >  solves the majority of problems, we need this anyway.
> >
> Actually it is very strange we lose them. Last time I checked vm_clock
> worked in such a way that if ticks were lost due to qemu not been scheduled
> for a long time timer callback was repeatedly fired to compensate for
> missed wakeups.
>

That's quite pointless, since those interrupts will be coalesced by the 
guest.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Gleb Natapov]

On Mon, Feb 07, 2011 at 03:46:54PM +0200, Avi Kivity wrote:
> On 02/07/2011 03:41 PM, Gleb Natapov wrote:
> >On Mon, Feb 07, 2011 at 07:23:22AM -0600, Anthony Liguori wrote:
> >>  On 02/07/2011 07:14 AM, Avi Kivity wrote:
> >>  >On 02/07/2011 03:11 PM, Anthony Liguori wrote:
> >>  >>On 02/07/2011 06:34 AM, Avi Kivity wrote:
> >>  >>>On 02/04/2011 10:56 AM, Jan Kiszka wrote:
> >>  >>>>>
> >>  >>>>>   This should be a rare event.  If you are missing 50% of your
> >>  >>>>>   notifications, not amount of gradual catchup is going to
> >>  >>>>help you out.
> >>  >>>>
> >>  >>>>But that's the only thing this patch is after: lost ticks at
> >>  >>>>QEMU level.
> >>  >>>
> >>  >>>Most lost ticks will happen at the vcpu level.  The iothread
> >>  >>>has low utilization and will therefore be scheduled promptly,
> >>  >>>whereas the vcpu thread may have high utilization and will
> >>  >>>thus be preempted.  When it is preempted for longer than the
> >>  >>>timer tick, we will see vcpu-level coalescing.  All it takes
> >>  >>>is 2:1 overcommit to see time go half as fast; I don't think
> >>  >>>you'll ever see that on bare metal.
> >>  >>
> >>  >>But that's not to say that doing something about lost ticks in
> >>  >>QEMU isn't still useful.
> >>  >>
> >>  >
> >>  >If it doesn't solve the majority of the problems it isn't very
> >>  >useful IMO.  It's a good first step, but not sufficient for real
> >>  >world use with overcommit.
> >>
> >>  Even if we have a way to detect coalescing, we still need to make
> >>  sure we don't lose ticks in QEMU.  So regardless of whether it
> >>  solves the majority of problems, we need this anyway.
> >>
> >Actually it is very strange we lose them. Last time I checked vm_clock
> >worked in such a way that if ticks were lost due to qemu not been scheduled
> >for a long time timer callback was repeatedly fired to compensate for
> >missed wakeups.
> >
> 
> That's quite pointless, since those interrupts will be coalesced by
> the guest.
> 
Yes, of course, and this is what I remember happening. At this point
interrupt de-coalescing kicks in.

--
			Gleb.

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 03:48 PM, Gleb Natapov wrote:
> >
> >  That's quite pointless, since those interrupts will be coalesced by
> >  the guest.
> >
> Yes, of course, and this is what I remember happening. At this point
> interrupt de-coalescing kicks in.

Maybe a more useful API would be to supply the callback with the number 
of missed wakeups since the last callback.  The callback could then 
change the wakeup frequency to compensate, or do other clever things.  
If it needs the old behaviour, it can just execute its code in a loop.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Gleb Natapov]

On Mon, Feb 07, 2011 at 03:51:27PM +0200, Avi Kivity wrote:
> On 02/07/2011 03:48 PM, Gleb Natapov wrote:
> >>
> >>  That's quite pointless, since those interrupts will be coalesced by
> >>  the guest.
> >>
> >Yes, of course, and this is what I remember happening. At this point
> >interrupt de-coalescing kicks in.
> 
> Maybe a more useful API would be to supply the callback with the
> number of missed wakeups since the last callback.  The callback
> could then change the wakeup frequency to compensate, or do other
> clever things.  If it needs the old behaviour, it can just execute
> its code in a loop.
> 
Mat be. Or may be we can ask for new behaviour during timer creation.
I was just surprised about the claim that we lost wakeups.

--
			Gleb.

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 14:23, Anthony Liguori wrote:
> On 02/07/2011 07:14 AM, Avi Kivity wrote:
>> On 02/07/2011 03:11 PM, Anthony Liguori wrote:
>>> On 02/07/2011 06:34 AM, Avi Kivity wrote:
>>>> On 02/04/2011 10:56 AM, Jan Kiszka wrote:
>>>>> >
>>>>> >  This should be a rare event.  If you are missing 50% of your
>>>>> >  notifications, not amount of gradual catchup is going to help
>>>>> you out.
>>>>>
>>>>> But that's the only thing this patch is after: lost ticks at QEMU
>>>>> level.
>>>>
>>>> Most lost ticks will happen at the vcpu level.  The iothread has low
>>>> utilization and will therefore be scheduled promptly, whereas the
>>>> vcpu thread may have high utilization and will thus be preempted. 
>>>> When it is preempted for longer than the timer tick, we will see
>>>> vcpu-level coalescing.  All it takes is 2:1 overcommit to see time
>>>> go half as fast; I don't think you'll ever see that on bare metal.
>>>
>>> But that's not to say that doing something about lost ticks in QEMU
>>> isn't still useful.
>>>
>>
>> If it doesn't solve the majority of the problems it isn't very useful
>> IMO.  It's a good first step, but not sufficient for real world use
>> with overcommit.
> 
> Even if we have a way to detect coalescing, we still need to make sure
> we don't lose ticks in QEMU.  So regardless of whether it solves the
> majority of problems, we need this anyway.

Again: please not in an ad-hoc fashion but as a generic services usable
by _all_ periodic timer sources that want to implement compensation.
This infrastructure should also be designed to once integrate IRQ
coalescing information as well.

The point why I'm insisting on a broader solution is that both sources
for lost ticks (iothread and vcpu) end up in the same output: an
adjustment of the injection frequency of the affected timer device.
There is not "HPET" or "RTC" or "PIT" in this, all this may apply to the
SoC timer of some emulated ARM board as well.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 08:10 AM, Jan Kiszka wrote:
> Again: please not in an ad-hoc fashion but as a generic services usable
> by _all_ periodic timer sources that want to implement compensation.
> This infrastructure should also be designed to once integrate IRQ
> coalescing information as well.
>
> The point why I'm insisting on a broader solution is that both sources
> for lost ticks (iothread and vcpu) end up in the same output: an
> adjustment of the injection frequency of the affected timer device.
> There is not "HPET" or "RTC" or "PIT" in this, all this may apply to the
> SoC timer of some emulated ARM board as well.
>    

Fair enough, how about:

typedef struct PeriodicTimer PeriodicTimer;

/**
  * @accumulated_ticks:  the number of unacknowledged ticks in total 
since the creation of the timer
  **/
typedef void (PeriodicTimer)(void *opaque, int accumulated_ticks);

PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);

void periodic_timer_mod(PeriodicTimer *timer, int64_t interval, TimeUnit 
unit);

/**
  * @policy: the drift catch-up policy
  *                DRIFT_COMP_FAST, deliver next tick as soon as any 
tick is acknowledged if accumulated_ticks > 1
  *                DRIFT_COMP_NONE, do not change interval regardless of 
accumulated ticks
  *                DRIFT_COMP_GRADUAL, shorten interval by half until 
accumulated_ticks <= 1
  */
void periodic_timer_set_policy(PeriodicTimer *timer, 
DriftCompensationPolicy policy);

/**
  * @ticks: number of ticks to acknowledge that are currently outstanding.
  **/
void periodic_timer_ack(PeriodicTimer *timer, int ticks);

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 15:28, Anthony Liguori wrote:
> On 02/07/2011 08:10 AM, Jan Kiszka wrote:
>> Again: please not in an ad-hoc fashion but as a generic services usable
>> by _all_ periodic timer sources that want to implement compensation.
>> This infrastructure should also be designed to once integrate IRQ
>> coalescing information as well.
>>
>> The point why I'm insisting on a broader solution is that both sources
>> for lost ticks (iothread and vcpu) end up in the same output: an
>> adjustment of the injection frequency of the affected timer device.
>> There is not "HPET" or "RTC" or "PIT" in this, all this may apply to the
>> SoC timer of some emulated ARM board as well.
>>    
> 
> Fair enough, how about:
> 
> typedef struct PeriodicTimer PeriodicTimer;
> 
> /**
>   * @accumulated_ticks:  the number of unacknowledged ticks in total 
> since the creation of the timer
>   **/
> typedef void (PeriodicTimer)(void *opaque, int accumulated_ticks);

I guess you mean PeriodicTimerFunc. Why the accumulated_ticks argument?

> 
> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
> 
> void periodic_timer_mod(PeriodicTimer *timer, int64_t interval, TimeUnit 
> unit);
> 
> /**
>   * @policy: the drift catch-up policy
>   *                DRIFT_COMP_FAST, deliver next tick as soon as any 
> tick is acknowledged if accumulated_ticks > 1
>   *                DRIFT_COMP_NONE, do not change interval regardless of 
> accumulated ticks
>   *                DRIFT_COMP_GRADUAL, shorten interval by half until 
> accumulated_ticks <= 1
>   */
> void periodic_timer_set_policy(PeriodicTimer *timer, 
> DriftCompensationPolicy policy);
> 
> /**
>   * @ticks: number of ticks to acknowledge that are currently outstanding.
>   **/
> void periodic_timer_ack(PeriodicTimer *timer, int ticks);
> 

Looks reasonable otherwise.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 08:43 AM, Jan Kiszka wrote:
> On 2011-02-07 15:28, Anthony Liguori wrote:
>    
>> On 02/07/2011 08:10 AM, Jan Kiszka wrote:
>>      
>>> Again: please not in an ad-hoc fashion but as a generic services usable
>>> by _all_ periodic timer sources that want to implement compensation.
>>> This infrastructure should also be designed to once integrate IRQ
>>> coalescing information as well.
>>>
>>> The point why I'm insisting on a broader solution is that both sources
>>> for lost ticks (iothread and vcpu) end up in the same output: an
>>> adjustment of the injection frequency of the affected timer device.
>>> There is not "HPET" or "RTC" or "PIT" in this, all this may apply to the
>>> SoC timer of some emulated ARM board as well.
>>>
>>>        
>> Fair enough, how about:
>>
>> typedef struct PeriodicTimer PeriodicTimer;
>>
>> /**
>>    * @accumulated_ticks:  the number of unacknowledged ticks in total
>> since the creation of the timer
>>    **/
>> typedef void (PeriodicTimer)(void *opaque, int accumulated_ticks);
>>      
> I guess you mean PeriodicTimerFunc.

Yes.

>   Why the accumulated_ticks argument?
>    

Then the missing ticks is stored in the PeriodicTimer instead of storing 
it in the device state.  That means we won't forget to save it in vmstate.

It's convenient because then if we lose ticks in the PeriodicTimer 
layer, the devices have instance access to that info.  When you do a 
read() from timerfd, it returns the number of coalesced events.  That's 
the interface I had in my mind.

We could just add a getter for PeriodicTimer and it would serve the same 
purpose.

Regards,

Anthony Liguori

>> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
>>
>> void periodic_timer_mod(PeriodicTimer *timer, int64_t interval, TimeUnit
>> unit);
>>
>> /**
>>    * @policy: the drift catch-up policy
>>    *                DRIFT_COMP_FAST, deliver next tick as soon as any
>> tick is acknowledged if accumulated_ticks>  1
>>    *                DRIFT_COMP_NONE, do not change interval regardless of
>> accumulated ticks
>>    *                DRIFT_COMP_GRADUAL, shorten interval by half until
>> accumulated_ticks<= 1
>>    */
>> void periodic_timer_set_policy(PeriodicTimer *timer,
>> DriftCompensationPolicy policy);
>>
>> /**
>>    * @ticks: number of ticks to acknowledge that are currently outstanding.
>>    **/
>> void periodic_timer_ack(PeriodicTimer *timer, int ticks);
>>
>>      
> Looks reasonable otherwise.
>
> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 15:54, Anthony Liguori wrote:
> On 02/07/2011 08:43 AM, Jan Kiszka wrote:
>> On 2011-02-07 15:28, Anthony Liguori wrote:
>>    
>>> On 02/07/2011 08:10 AM, Jan Kiszka wrote:
>>>      
>>>> Again: please not in an ad-hoc fashion but as a generic services usable
>>>> by _all_ periodic timer sources that want to implement compensation.
>>>> This infrastructure should also be designed to once integrate IRQ
>>>> coalescing information as well.
>>>>
>>>> The point why I'm insisting on a broader solution is that both sources
>>>> for lost ticks (iothread and vcpu) end up in the same output: an
>>>> adjustment of the injection frequency of the affected timer device.
>>>> There is not "HPET" or "RTC" or "PIT" in this, all this may apply to the
>>>> SoC timer of some emulated ARM board as well.
>>>>
>>>>        
>>> Fair enough, how about:
>>>
>>> typedef struct PeriodicTimer PeriodicTimer;
>>>
>>> /**
>>>    * @accumulated_ticks:  the number of unacknowledged ticks in total
>>> since the creation of the timer
>>>    **/
>>> typedef void (PeriodicTimer)(void *opaque, int accumulated_ticks);
>>>      
>> I guess you mean PeriodicTimerFunc.
> 
> Yes.
> 
>>   Why the accumulated_ticks argument?
>>    
> 
> Then the missing ticks is stored in the PeriodicTimer instead of storing 
> it in the device state.  That means we won't forget to save it in vmstate.

There should rather be a special vmstate struct for PeriodicTimer, just
like we already have for normal timers.

> 
> It's convenient because then if we lose ticks in the PeriodicTimer 
> layer, the devices have instance access to that info.  When you do a 
> read() from timerfd, it returns the number of coalesced events.  That's 
> the interface I had in my mind.
> 
> We could just add a getter for PeriodicTimer and it would serve the same 
> purpose.

I'm still not sure what the device model is supposed to do with that
information. I think at could remain private to the PeriodicTimer
implementation (unless we want to dump some stats or such).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 04:54 PM, Anthony Liguori wrote:
>
>>   Why the accumulated_ticks argument?
>
> Then the missing ticks is stored in the PeriodicTimer instead of 
> storing it in the device state.  That means we won't forget to save it 
> in vmstate.
>
> It's convenient because then if we lose ticks in the PeriodicTimer 
> layer, the devices have instance access to that info.  When you do a 
> read() from timerfd, it returns the number of coalesced events.  
> That's the interface I had in my mind.
>
> We could just add a getter for PeriodicTimer and it would serve the 
> same purpose.

If a drift compensation policy is in effect, you don't need the missed 
ticks, since you will get one callback for each (delayed) tick.  If 
there is no drift compensation policy, presumably you aren't interested 
in lost ticks.  So the ticks argument isn't very useful.

On the other hand, we need a way to inject lost ticks into a 
PeriodicTimer.  If interrupt injection detects that an interrupt was 
coalesced, we want the timer to schedule a new tick for us.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 08:57 AM, Jan Kiszka wrote:
> There should rather be a special vmstate struct for PeriodicTimer, just
> like we already have for normal timers.
>    

Agreed.

>> It's convenient because then if we lose ticks in the PeriodicTimer
>> layer, the devices have instance access to that info.  When you do a
>> read() from timerfd, it returns the number of coalesced events.  That's
>> the interface I had in my mind.
>>
>> We could just add a getter for PeriodicTimer and it would serve the same
>> purpose.
>>      
> I'm still not sure what the device model is supposed to do with that
> information. I think at could remain private to the PeriodicTimer
> implementation (unless we want to dump some stats or such).
>    

Yeah, I've been thinking about it too and I think I agree with you.

So here's the new proposal:

typedef struct PeriodicTimer PeriodicTimer;

/**
  * @accumulated_ticks:  the number of unacknowledged ticks in total 
since the creation of the timer
  **/
typedef void (PeriodicTimerFunc)(void *opaque);

PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);

void periodic_timer_mod(PeriodicTimer *timer, int64_t interval, TimeUnit 
unit);

/**
  * @policy: the drift catch-up policy
  *                DRIFT_COMP_FAST, deliver next tick as soon as any 
tick is acknowledged if accumulated_ticks > 1
  *                DRIFT_COMP_NONE, do not change interval regardless of 
accumulated ticks
  *                DRIFT_COMP_GRADUAL, shorten interval by half until 
accumulated_ticks <= 1
  */
void periodic_timer_set_policy(PeriodicTimer *timer, 
DriftCompensationPolicy policy);

/**
  * @ticks: number of ticks to acknowledge that are currently outstanding.
  **/
void periodic_timer_ack(PeriodicTimer *timer, int ticks);

int periodic_timer_get_accumulated_ticks(PeriodicTimer *timer);

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 15:58, Avi Kivity wrote:
> On 02/07/2011 04:54 PM, Anthony Liguori wrote:
>>
>>>   Why the accumulated_ticks argument?
>>
>> Then the missing ticks is stored in the PeriodicTimer instead of 
>> storing it in the device state.  That means we won't forget to save it 
>> in vmstate.
>>
>> It's convenient because then if we lose ticks in the PeriodicTimer 
>> layer, the devices have instance access to that info.  When you do a 
>> read() from timerfd, it returns the number of coalesced events.  
>> That's the interface I had in my mind.
>>
>> We could just add a getter for PeriodicTimer and it would serve the 
>> same purpose.
> 
> If a drift compensation policy is in effect, you don't need the missed 
> ticks, since you will get one callback for each (delayed) tick.  If 
> there is no drift compensation policy, presumably you aren't interested 
> in lost ticks.  So the ticks argument isn't very useful.

Exactly.

> 
> On the other hand, we need a way to inject lost ticks into a 
> PeriodicTimer.  If interrupt injection detects that an interrupt was 
> coalesced, we want the timer to schedule a new tick for us.

Isn't absence of corresponding call to periodic_timer_ack() sufficient?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 05:01 PM, Jan Kiszka wrote:
> >
> >  On the other hand, we need a way to inject lost ticks into a
> >  PeriodicTimer.  If interrupt injection detects that an interrupt was
> >  coalesced, we want the timer to schedule a new tick for us.
>
> Isn't absence of corresponding call to periodic_timer_ack() sufficient?

It probably is.  However, that API is easy to misuse; if you forget to 
call it, the timer goes crazy.  The default behaviour should be to 
assume an ack and the API should provide adjustments.

Also need to design the API carefully for changing frequency (Windows is 
known to do that) and switching from periodic to single shot.  For the 
first case I guess we need to adjust the deferred ticks to the new time 
base (so if the frequency doubles, the lost ticks up to that point 
double as well).  For the second case, I guess we just lose time.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 16:01, Anthony Liguori wrote:
> On 02/07/2011 08:57 AM, Jan Kiszka wrote:
>> There should rather be a special vmstate struct for PeriodicTimer, just
>> like we already have for normal timers.
>>    
> 
> Agreed.
> 
>>> It's convenient because then if we lose ticks in the PeriodicTimer
>>> layer, the devices have instance access to that info.  When you do a
>>> read() from timerfd, it returns the number of coalesced events.  That's
>>> the interface I had in my mind.
>>>
>>> We could just add a getter for PeriodicTimer and it would serve the same
>>> purpose.
>>>      
>> I'm still not sure what the device model is supposed to do with that
>> information. I think at could remain private to the PeriodicTimer
>> implementation (unless we want to dump some stats or such).
>>    
> 
> Yeah, I've been thinking about it too and I think I agree with you.
> 
> So here's the new proposal:
> 
> typedef struct PeriodicTimer PeriodicTimer;
> 
> /**
>   * @accumulated_ticks:  the number of unacknowledged ticks in total 
> since the creation of the timer
>   **/
> typedef void (PeriodicTimerFunc)(void *opaque);

Or just use QEMUTimerCB directly. BTW, QEMUPeriodicTimer* would probably
be more consistent with the existing naming scheme.

> 
> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
> 
> void periodic_timer_mod(PeriodicTimer *timer, int64_t interval, TimeUnit 
> unit);
> 
> /**
>   * @policy: the drift catch-up policy
>   *                DRIFT_COMP_FAST, deliver next tick as soon as any 
> tick is acknowledged if accumulated_ticks > 1
>   *                DRIFT_COMP_NONE, do not change interval regardless of 
> accumulated ticks
>   *                DRIFT_COMP_GRADUAL, shorten interval by half until 
> accumulated_ticks <= 1
>   */
> void periodic_timer_set_policy(PeriodicTimer *timer, 
> DriftCompensationPolicy policy);
> 
> /**
>   * @ticks: number of ticks to acknowledge that are currently outstanding.
>   **/
> void periodic_timer_ack(PeriodicTimer *timer, int ticks);
> 
> int periodic_timer_get_accumulated_ticks(PeriodicTimer *timer);
> 

Looks like a plan.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 05:01 PM, Anthony Liguori wrote:
>
> typedef struct PeriodicTimer PeriodicTimer;
>
> /**
>  * @accumulated_ticks:  the number of unacknowledged ticks in total 
> since the creation of the timer
>  **/

Outdated comment even before the code is committed.  Will be hard to beat.

> typedef void (PeriodicTimerFunc)(void *opaque);

s/void *opaque/PeriodicTimer *timer/

Down with opaques!

>
> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
>

void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);

It is better to embed than to reference.


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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 16:08, Avi Kivity wrote:
> On 02/07/2011 05:01 PM, Jan Kiszka wrote:
>>>
>>>  On the other hand, we need a way to inject lost ticks into a
>>>  PeriodicTimer.  If interrupt injection detects that an interrupt was
>>>  coalesced, we want the timer to schedule a new tick for us.
>>
>> Isn't absence of corresponding call to periodic_timer_ack() sufficient?
> 
> It probably is.  However, that API is easy to misuse; if you forget to 
> call it, the timer goes crazy.  The default behaviour should be to 
> assume an ack and the API should provide adjustments.

Explicit nack'ing only works smoothly if we have immediate (synchronous)
feedback about the injected event. My feeling is that ack'ing results in
simpler, thus also easier to review code.

> 
> Also need to design the API carefully for changing frequency (Windows is 
> known to do that) and switching from periodic to single shot.  For the 
> first case I guess we need to adjust the deferred ticks to the new time 
> base (so if the frequency doubles, the lost ticks up to that point 
> double as well).  For the second case, I guess we just lose time.

I think this is rather a question how the logic behind
periodic_timer_mod works with updates. The API should be sufficient as
simple as it is.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 05:14 PM, Jan Kiszka wrote:
> On 2011-02-07 16:08, Avi Kivity wrote:
> >  On 02/07/2011 05:01 PM, Jan Kiszka wrote:
> >>>
> >>>   On the other hand, we need a way to inject lost ticks into a
> >>>   PeriodicTimer.  If interrupt injection detects that an interrupt was
> >>>   coalesced, we want the timer to schedule a new tick for us.
> >>
> >>  Isn't absence of corresponding call to periodic_timer_ack() sufficient?
> >
> >  It probably is.  However, that API is easy to misuse; if you forget to
> >  call it, the timer goes crazy.  The default behaviour should be to
> >  assume an ack and the API should provide adjustments.
>
> Explicit nack'ing only works smoothly if we have immediate (synchronous)
> feedback about the injected event. My feeling is that ack'ing results in
> simpler, thus also easier to review code.

Yes.  Maybe we should have an auto-ack mode, so that code that doesn't 
have ack notification doesn't need to bother with it.

> >
> >  Also need to design the API carefully for changing frequency (Windows is
> >  known to do that) and switching from periodic to single shot.  For the
> >  first case I guess we need to adjust the deferred ticks to the new time
> >  base (so if the frequency doubles, the lost ticks up to that point
> >  double as well).  For the second case, I guess we just lose time.
>
> I think this is rather a question how the logic behind
> periodic_timer_mod works with updates. The API should be sufficient as
> simple as it is.

Yes, it's an implementation detail.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 16:13, Avi Kivity wrote:
> On 02/07/2011 05:01 PM, Anthony Liguori wrote:
>>
>> typedef struct PeriodicTimer PeriodicTimer;
>>
>> /**
>>  * @accumulated_ticks:  the number of unacknowledged ticks in total 
>> since the creation of the timer
>>  **/
> 
> Outdated comment even before the code is committed.  Will be hard to beat.
> 
>> typedef void (PeriodicTimerFunc)(void *opaque);
> 
> s/void *opaque/PeriodicTimer *timer/
> 
> Down with opaques!

What else? DeviceState?

> 
>>
>> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
>>
> 
> void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);
> 
> It is better to embed than to reference.

Likely, though this diverges from exiting QEMUTimer.

BTW, QEMUClock argument is missing in new/init.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 08:58 AM, Avi Kivity wrote:
> On 02/07/2011 04:54 PM, Anthony Liguori wrote:
>>
>>>   Why the accumulated_ticks argument?
>>
>> Then the missing ticks is stored in the PeriodicTimer instead of 
>> storing it in the device state.  That means we won't forget to save 
>> it in vmstate.
>>
>> It's convenient because then if we lose ticks in the PeriodicTimer 
>> layer, the devices have instance access to that info.  When you do a 
>> read() from timerfd, it returns the number of coalesced events.  
>> That's the interface I had in my mind.
>>
>> We could just add a getter for PeriodicTimer and it would serve the 
>> same purpose.
>
> If a drift compensation policy is in effect, you don't need the missed 
> ticks, since you will get one callback for each (delayed) tick.  If 
> there is no drift compensation policy, presumably you aren't 
> interested in lost ticks.  So the ticks argument isn't very useful.
>
> On the other hand, we need a way to inject lost ticks into a 
> PeriodicTimer.  If interrupt injection detects that an interrupt was 
> coalesced, we want the timer to schedule a new tick for us.

As an optimization, if you can do something useful with the knowledge 
that we missed 20 ticks instead of waiting for 20 callbacks, it's useful 
to have.  But adding a getter makes that possible and I agree that it 
clutters the interface for an edge use-case.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Jan Kiszka]

On 2011-02-07 16:13, Avi Kivity wrote:
>>
>> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
>>
> 
> void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);
> 
> It is better to embed than to reference.

And embedding means making the layout (at least the size) of
PeriodicTimer public. I guess that's why QEMUTimer works via new.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 09:08 AM, Avi Kivity wrote:
> On 02/07/2011 05:01 PM, Jan Kiszka wrote:
>> >
>> >  On the other hand, we need a way to inject lost ticks into a
>> >  PeriodicTimer.  If interrupt injection detects that an interrupt was
>> >  coalesced, we want the timer to schedule a new tick for us.
>>
>> Isn't absence of corresponding call to periodic_timer_ack() sufficient?
>
> It probably is.  However, that API is easy to misuse; if you forget to 
> call it, the timer goes crazy.  The default behaviour should be to 
> assume an ack and the API should provide adjustments.

I disagree.  You typically only call it in one place and as such, if you 
forget to add that call, you'll find out pretty quickly that you did 
something wrong.

OTOH, accounting missed ticks is easy to not do correctly and it's hard 
to get feedback when that happens.

>
> Also need to design the API carefully for changing frequency (Windows 
> is known to do that) and switching from periodic to single shot.  For 
> the first case I guess we need to adjust the deferred ticks to the new 
> time base (so if the frequency doubles, the lost ticks up to that 
> point double as well).  For the second case, I guess we just lose time.

Bus cycle based one shot timers probably need to be treated as a special 
case of changing an interval followed by setting the interval to zero.

For non-bus cycle based timers, it probably doesn't matter and is easy 
to just use a one shot timer (like QEMUTimer).

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 05:17 PM, Jan Kiszka wrote:
> On 2011-02-07 16:13, Avi Kivity wrote:
> >  On 02/07/2011 05:01 PM, Anthony Liguori wrote:
> >>
> >>  typedef struct PeriodicTimer PeriodicTimer;
> >>
> >>  /**
> >>   * @accumulated_ticks:  the number of unacknowledged ticks in total
> >>  since the creation of the timer
> >>   **/
> >
> >  Outdated comment even before the code is committed.  Will be hard to beat.
> >
> >>  typedef void (PeriodicTimerFunc)(void *opaque);
> >
> >  s/void *opaque/PeriodicTimer *timer/
> >
> >  Down with opaques!
>
> What else? DeviceState?

typedef void (PeriodicTimerFunc)(PeriodicTimer *timer);

the callback then uses container_of() to get whatever its internal data 
structure is from the embedded PeriodicTimer.

> >>
> >>  PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
> >>
> >
> >  void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);
> >
> >  It is better to embed than to reference.
>
> Likely, though this diverges from exiting QEMUTimer.

That's the more modern style.  Saves allocations and dereferences, and 
is more type safe.

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 05:20 PM, Jan Kiszka wrote:
> On 2011-02-07 16:13, Avi Kivity wrote:
> >>
> >>  PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
> >>
> >
> >  void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);
> >
> >  It is better to embed than to reference.
>
> And embedding means making the layout (at least the size) of
> PeriodicTimer public. I guess that's why QEMUTimer works via new.

Why do we care?  We don't have an stable module interface.

(the way to provide a size-stable interface is

     struct PeriodicTimer {
         struct PeriodicTimerImpl *impl
     };

though of course it negates some of the advantages of embedding).

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

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 09:20 AM, Jan Kiszka wrote:
> On 2011-02-07 16:13, Avi Kivity wrote:
>    
>>> PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void *opaque);
>>>
>>>        
>> void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc *cb);
>>
>> It is better to embed than to reference.
>>      
> And embedding means making the layout (at least the size) of
> PeriodicTimer public. I guess that's why QEMUTimer works via new.
>    

Yeah, QEMU generally follows a construction-is-allocation model.

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Anthony Liguori]

On 02/07/2011 09:29 AM, Avi Kivity wrote:
> On 02/07/2011 05:17 PM, Jan Kiszka wrote:
>> On 2011-02-07 16:13, Avi Kivity wrote:
>> >  On 02/07/2011 05:01 PM, Anthony Liguori wrote:
>> >>
>> >>  typedef struct PeriodicTimer PeriodicTimer;
>> >>
>> >>  /**
>> >>   * @accumulated_ticks:  the number of unacknowledged ticks in total
>> >>  since the creation of the timer
>> >>   **/
>> >
>> >  Outdated comment even before the code is committed.  Will be hard 
>> to beat.
>> >
>> >>  typedef void (PeriodicTimerFunc)(void *opaque);
>> >
>> >  s/void *opaque/PeriodicTimer *timer/
>> >
>> >  Down with opaques!
>>
>> What else? DeviceState?
>
> typedef void (PeriodicTimerFunc)(PeriodicTimer *timer);
>
> the callback then uses container_of() to get whatever its internal 
> data structure is from the embedded PeriodicTimer.

For the purposes of this, I think passing an opaque is better because 
the signature stays the same as the existing timer callback.  That makes 
conversion a bit friendlier.

I think it's better to avoid introducing stylistic changes with new 
interfaces.  I think they should be done separately.

Regards,

Anthony Liguori

>
>> >>
>> >>  PeriodicTimer *periodic_timer_new(PeriodicTimerFunc *cb, void 
>> *opaque);
>> >>
>> >
>> >  void periodic_timer_init(PeriodicTimer *timer, PeriodicTimerFunc 
>> *cb);
>> >
>> >  It is better to embed than to reference.
>>
>> Likely, though this diverges from exiting QEMUTimer.
>
> That's the more modern style.  Saves allocations and dereferences, and 
> is more type safe.
>

Re: [Qemu-devel] Re: [RFC: 0/2] patch for QEMU HPET periodic timer emulation to alleviate time drift

[Avi Kivity]

On 02/07/2011 09:30 PM, Anthony Liguori wrote:
>
> For the purposes of this, I think passing an opaque is better because 
> the signature stays the same as the existing timer callback.  That 
> makes conversion a bit friendlier.
>
> I think it's better to avoid introducing stylistic changes with new 
> interfaces.  I think they should be done separately.

Suppose I convert all 100-odd QEMUTimer users to the new style?  Might 
be a good exercise for Coccinelle.

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