[RFC] Event timers on MIPS

[Neel Natu]

Hi Alexander,

On Wed, Jul 21, 2010 at 10:36 PM, Alexander Motin <mav at freebsd.org> wrote:
> Hi.
>
> Neel Natu wrote:
>> Your patch works fine on a Sibyte. I verified that wall clock time is
>> progressing as expected. Is there any other test that you recommend?
>>
>> I have a few comments about the patch:
>>
>> 1. set_cputicker() in clock_attach() should be moved back up to
>>     mips_timer_init_params(). Otherwise the platform-specific CPU tickers
>>    used by Sibyte, Octeon and RMI will be overwritten by the mips32 ticker.
>
> Fixed.
>
>> 2. The local variable 'cycles_per_tick' in clock_intr() can now be a uint32_t.
>
> Fixed.
>
>> 3. Is there a race between setting 'cycles_per_tick' in clock_start() and
>>     clock_intr()? Would it be better to write the compare register first
>>     and then set 'cycles_per_tick' to a non-zero value?
>
> And how does it help?
>

So, the timeline I have in mind is this:

A: clock_stop() sets compare register to 0xffffffff and cycles_per_tick to 0

B: clock_start() is called and we update cycles_per_tick to a non-zero
value but we have not updated the compare register yet

C: the COUNT register is a free running counter and it happens to
reach 0xffffffff exactly at this time

D: the clock_intr() handler sees a non-zero cycles_per_tick value and
proceeds as if this was a legitimate interrupt (when in reality it is
not)

If we update the cycles_per_tick at the very end then it is guaranteed
that any interrupt that happens while it is zero is essentially
ignored. And any interrupt that happens after it has been updated to a
non-zero value is legitimate.

>>     If I understand the code correctly we are liable to get clock interrupts
>>     even afer the clock is stopped when the CP0 COUNT register matches
>>     0xffffffff.
>
> You mean we can receive interrupt from previous clock_start() after new
> one? Theoretically I think it is possible, but what can we do about it?
>

I don't think we can do anything about it. I was referring to the race
when the clock was stopped and we are in the process of restarting it.
See above.

>> 4. We need to update the DPCPU(compare_ticks) value when we start the timer
>>     in clock_start().
>
> Fixed.
>
>> 5. I think the entire 'lost_ticks' processing in clock_intr() should be
>>     conditional on (cycles_per_tick > 0). Without this we may incorrectly
>>     update the value of DPCPU(lost_ticks).
>
> Fixed. Indeed, in one-shot mode extra callback can be confusing.
>
>> 6. Can you a couple of lines of explaining the computatation of
>>     'et_min_period' and 'et_max_period'? It is not completely obvious to me.
>
> Minimal period is set artificialy to reduce possibility to lost very
> short time interval during comparator programming. I've done it after
> marius@ asked to do the same for sparc64. If this comparator is more
> clever to handle missed time, it may not be needed.
> Maximal period is calculated from maximal counter value, minus one to
> possibly avoid some rounding overflows. Again, if this comparator is
> "clever" may be it need to be reduced.
> I don't have documentation for this hardware, so fix me if I am wrong.
>

I see. Thanks for the explanation.

> New patch: http://people.freebsd.org/~mav/timers_mips3.patch
>

In clock_intr() it would seem that the last 'et_event_cb()' should be
called conditionally only if (cycles_per_tick > 0). Of course, this is
necessary only if my explanation about spurious clock_intr()
invocations is correct.

I have tested the latest patch on the Sibyte as well and it works correctly.

best
Neel

> Thank you!
>
> --
> Alexander Motin
>