Re: [PATCH 2/3] dyntick - Fix lost tick calculation in timer pm.c

[Thomas Schlichter <thomas.schlichter@web.de>]

Hi Srivatsa,

thank you for improving your patch by fixing the two problems. Now I do have 
just two minor nits which you may consider:

1. You don't need to hold the monotonic_lock that long, it is only necessary 
when updating offset_last and monotonic_base. So I would propose something 
like this:

  offset_now = read_pmtmr();

  /* calculate tick interval */
  delta = (offset_now - offset_last) & ACPI_PM_MASK;

  /* convert to ticks */
  lost = delta / pm_ticks_per_jiffy;

  /* convert ticks to usecs */
  deltaus = cyc2us(lost * pm_ticks_per_jiffy);
  // can we use this instead: ?
  // deltaus = jiffies_to_usecs(lost);

  write_seqlock(&monotonic_lock);
  offset_last += lost * pm_ticks_per_jiffy;
  offset_last &= ACPI_PM_MASK;

  /* update the monotonic base value */
  monotonic_base += deltaus * NSEC_PER_USEC;
  write_sequnlock(&monotonic_lock);

2. Can we really assure that the monotonic clock is still monotonic?
I think with your new code we estimate the monotonic clock value and the 
offset_last at the last tick.
But if we underestimate monotonic_base or overestimate offset_last (even 
simply by rounding errors), the time will make a small step backwards with 
the value-update.
And as far as I understand the monotonic clock its not that bad if it drifts a 
bit, but it is really bad if time makes steps backward...

But maybe you can show me that I am wrong with my second point.
I hope I don't bother you too much with this kind of stuff...

  Thomas

P.S.: I CC'd John because he knows the monotonic clock better than I do... :-)

Am Freitag, 2. September 2005 19:25 schrieb Srivatsa Vaddagiri:
> Con,
> 	Pls use this updated "Lost tick" calculation patch, which rectifies the
> two problems Thomas pointed out. I have done some basic test with it.
>
> Would it be possible to incorporate this updated patch in
> http://ck.kolivas.org/patches/dyn-ticks/2.6.13-mm1-dtck1.patch?
>
> Sorry for the inconvenience!

Attachment: pgpioYekEuTe6.pgp
Description: signature