[RFC] Event timers on MIPS

[Neel Natu]

Hi,

On Tue, Jul 27, 2010 at 7:43 AM, Jayachandran C.
<c.jayachandran at gmail.com> wrote:
> On Tue, Jul 27, 2010 at 6:04 PM, Alexander Motin <mav at freebsd.org> wrote:
>> Jayachandran C. wrote:
>>> On Sun, Jul 18, 2010 at 1:04 AM, Alexander Motin <mav at freebsd.org> wrote:
>>>> Jayachandran C. wrote:
>>>>> On XLR we would like to use the count/compare which is faster but less
>>>>> accurate on all cpus - we can have upto 32 cpus now.  We also have a
>>>>> PIC which can provide a better timestamp and timer interrupts.  This
>>>>> PIC timestamp can be read from all CPUs but the timer interrupt can be
>>>>> delivered to just one CPU at a time.  I think this is how we ended up
>>>>> with the current implementation, but any suggestions on how to improve
>>>>> this is welcome.
>>>
>>> As a first step, I have copied the count /compare code from mips with
>>> minor modifications into mips/rmi, this lets me boot up (checked in as
>>> r210528).
>>>
>>> I would like to add the PIC based clock next.
>>
>> Thanks.
>>
>>>> I would prefer to not mix the things.
>>>>
>>>> I think:
>>>>  - PIC timestamp looks like the best candidate for system timecounter.
>>>>  - per-CPU counters could be registered as per-CPU timecounters with
>>>> set_cputicker() - the main criteria there is a speed.
>>>>  - if per-CPU counters are synchronized between CPUs - they could be
>>>> registered as alternative timecounter for people who wish fastest
>>>> timecounting; if they are not - they are useless in that role.
>>>>  - both PIC timer and per-CPU comparators should be independently
>>>> registered as eventtimers - it is better to have two of them to from
>>>> accounting correctness PoV, and it will allow user to experiment which
>>>> one he likes more.
>>>>  - if there is any other timer hardware - it also should be registered -
>>>> it will give additional flexibility.
>>>
>>> The per-cpu count/compare counters are not synchronized on XLR.
>>
>> Then tick_ticker() function looks broken. counter_lower_last and
>> counter_upper should be tracked per-CPU. Otherwise you will have huge
>> forward jumps due to false overflows.
>>
>>> So your suggestion would be to add a PIC based clock which calls
>>> tc_init() and et_register(), and to leave the set_cputicker() to be
>>> the count/compare?
>>
>> Yes. And I would leave count/compare also calling tc_init() and
>> et_register() as it is now. It won't hurt.
>>
>>> Also, with just the count/compare, I get these print on early mutiuser bootup.
>>> ---
>>> calcru: runtime went backwards from 85936878 usec to 236488 usec for
>>> pid 1286 (rpcbind)
>>> calcru: runtime went backwards from 7158742 usec to 19700 usec for pid
>>> 1285 (nfsiod 0)
>>> calcru: runtime went backwards from 111005442 usec to 305474 usec for
>>> pid 1257 (syslogd)
>>> calcru: runtime went backwards from 10740196 usec to 29555 usec for
>>> pid 1048 (devd)
>>> --
>>> Did not get much time to investigate, any idea what the cause  can be?
>>
>> I think it can easily be result of broken tick_ticker().
>
> I'm planning to check-in the attached patch for mips/rmi, I think
> mips/mips would need something similar.
>

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.

- 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'.

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

Comments?

best
Neel

Index: sys/mips/sibyte/sb_machdep.c
===================================================================
--- sys/mips/sibyte/sb_machdep.c	(revision 210664)
+++ sys/mips/sibyte/sb_machdep.c	(working copy)
@@ -454,6 +454,4 @@
 	mips_init();

 	mips_timer_init_params(sb_cpu_speed(), 0);
-
-	set_cputicker(sb_zbbus_cycle_count, sb_cpu_speed() / 2, 1);
 }
Index: sys/mips/mips/tick.c
===================================================================
--- sys/mips/mips/tick.c	(revision 210664)
+++ sys/mips/mips/tick.c	(working copy)
@@ -60,9 +60,8 @@
 static DPCPU_DEFINE(uint32_t, cycles_per_tick);
 static uint32_t cycles_per_usec;

-static u_int32_t counter_upper = 0;
-static u_int32_t counter_lower_last = 0;
-
+static DPCPU_DEFINE(uint32_t, counter_upper);
+static DPCPU_DEFINE(uint32_t, counter_lower_last);
 static DPCPU_DEFINE(uint32_t, compare_ticks);
 static DPCPU_DEFINE(uint32_t, lost_ticks);

@@ -104,21 +103,32 @@
 {
 	uint64_t ret;
 	uint32_t ticktock;
+	uint32_t t_lower_last, t_upper;

 	/*
-	 * XXX: MIPS64 platforms can read 64-bits of counter directly.
-	 * Also: the tc code is supposed to cope with things wrapping
-	 * from the time counter, so I'm not sure why all these hoops
-	 * are even necessary.
+	 * Disable preemption because we are working with cpu specific data.
 	 */
+	critical_enter();
+
+	/*
+	 * Note that even though preemption is disabled, interrupts are
+	 * still enabled. In particular there is a race with clock_intr()
+	 * reading the values of 'counter_upper' and 'counter_lower_last'.
+	 */
+	do {
+		t_upper = DPCPU_GET(counter_upper);
+		t_lower_last = DPCPU_GET(counter_lower_last);
+	} while (t_upper != DPCPU_GET(counter_upper));
+
 	ticktock = mips_rd_count();
-	critical_enter();
-	if (ticktock < counter_lower_last)
-		counter_upper++;
-	counter_lower_last = ticktock;
+
 	critical_exit();

-	ret = ((uint64_t) counter_upper << 32) | counter_lower_last;
+	/* COUNT register wrapped around */
+	if (ticktock < t_lower_last)
+		t_upper++;
+
+	ret = ((uint64_t)t_upper << 32) | ticktock;
 	return (ret);
 }

@@ -262,11 +272,11 @@
 	} else	/* In one-shot mode timer should be stopped after the event. */
 		mips_wr_compare(0xffffffff);

-	critical_enter();
-	if (count < counter_lower_last) {
-		counter_upper++;
-		counter_lower_last = count;
+	/* COUNT register wrapped around */
+	if (count < DPCPU_GET(counter_lower_last)) {
+		DPCPU_SET(counter_upper, DPCPU_GET(counter_upper) + 1);
 	}
+	DPCPU_SET(counter_lower_last, count);

 	if (cycles_per_tick > 0) {

@@ -296,7 +306,6 @@
 	}
 	if (sc->et.et_active)
 		sc->et.et_event_cb(&sc->et, sc->et.et_arg);
-	critical_exit();
 	return (FILTER_HANDLED);
 }


> JC.
>