[RFC] Event timers on MIPS

[Neel Natu]

Hi Alexander,

Thanks for taking the time to review the patch. Here is the updated patch:
http://people.freebsd.org/~neel/tick_diff.txt

On Sun, Aug 1, 2010 at 4:39 AM, Alexander Motin <mav at freebsd.org> wrote:
> Neel Natu wrote:
>> Here is the patch for mips/mips/tick.c to fix tick_ticker().
>>
>> In addition to incorporating the changes made in rmi/tick.c it fixes
>> the following:
>>
>> - There is a race between clock_intr() and tick_ticker() updating
>> counter_upper and counter_lower_last. This race exists because
>> interrupts are enabled even though tick_ticker() executes in a
>> critical section.
>
> While there is indeed a possible issue, I am not sure your solution is
> reliable. I haven't looked how DPCPU_GET implemented on MIPS, but can't
> compiler reorder them? I would thought about some lock or at least some
> atomics with barriers.
>

Good catch. I have changed the type of 'counter_upper' and
'counter_lower_last' from 'uint32_t' to 'volatile uint32_t'. That
should prevent the compiler from doing any reordering or loop
optimization.

I also verified the assembly listing of tick_ticker() and made sure
that the such optimizations are not occurring.

> "t_upper++;" there looks a bit strange, as it is not written back. The
> wrapping stuff won't work if this timer interrupts were not used.
>

This part is intentional.

I wanted only clock_intr() to update the cached values of
'counter_upper' and 'counter_lower_last' and tick_ticker() to sample a
consistent snapshot of the tuple and then operate on it.

I have added an XXX comment to describe the dependency. We can revisit
this if we change the default timer in mips.

>> - Fix a bug in clock_intr() in how it updates counter_upper and
>> counter_lower_last. It updates it only once every time the COUNT
>> register wraps around. More interestingly it will *never* update the
>> cached values of 'counter_upper' and 'counter_lower_last' if the
>> previous value of 'counter_lower_last' happens to be '0'.
>
> Reasonable. It would be nice if both wrapping places were implemented
> alike or the same way.
>

This was intentional. See above.

>> - Get rid of the superfluous critical section in clock_intr(). There
>> is no reason for it because clock_intr() executes in hard interrupt
>> context.
>
> Seems OK.
>

best
Neel

> --
> Alexander Motin
>