Date: Fri, 30 Jul 2010 17:47:56 -0700 From: Neel Natu <neelnatu@gmail.com> To: "Jayachandran C." <c.jayachandran@gmail.com> Cc: Randall Stewart <rrs@freebsd.org>, Alexander Motin <mav@freebsd.org>, Neel Natu <neel@freebsd.org>, freebsd-mips@freebsd.org Subject: Re: [RFC] Event timers on MIPS Message-ID: <AANLkTinH1itTauurEFCZVyz%2BW4T02niPV1jQeNART1Gm@mail.gmail.com> In-Reply-To: <AANLkTiktMt87V5jXV0%2BnagHjpfTkBQ8Fu6CK7HqNXff3@mail.gmail.com> References: <4C41A248.8090605@FreeBSD.org> <AANLkTilKYw4UqmfEee9zHGosEDzy4hiFob1d8R9jcB25@mail.gmail.com> <4C41B4CF.6080409@FreeBSD.org> <AANLkTik8_NGm7nKYXT1d1E4Vj6vYQPWHnnLDi78YnvQD@mail.gmail.com> <4C4205CC.6080700@FreeBSD.org> <AANLkTikUpqLeogkqxqWzzejp=7FstHX2wVRWNrYoWGCp@mail.gmail.com> <4C4ED247.80701@FreeBSD.org> <AANLkTiktMt87V5jXV0%2BnagHjpfTkBQ8Fu6CK7HqNXff3@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
On Tue, Jul 27, 2010 at 7:43 AM, Jayachandran C.
<c.jayachandran@gmail.com> wrote:
> On Tue, Jul 27, 2010 at 6:04 PM, Alexander Motin <mav@freebsd.org> wrote:
>> Jayachandran C. wrote:
>>> On Sun, Jul 18, 2010 at 1:04 AM, Alexander Motin <mav@freebsd.org> wrot=
e:
>>>> Jayachandran C. wrote:
>>>>> On XLR we would like to use the count/compare which is faster but les=
s
>>>>> accurate on all cpus - we can have upto 32 cpus now. =A0We also have =
a
>>>>> PIC which can provide a better timestamp and timer interrupts. =A0Thi=
s
>>>>> PIC timestamp can be read from all CPUs but the timer interrupt can b=
e
>>>>> delivered to just one CPU at a time. =A0I think this is how we ended =
up
>>>>> with the current implementation, but any suggestions on how to improv=
e
>>>>> 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:
>>>> =A0- PIC timestamp looks like the best candidate for system timecounte=
r.
>>>> =A0- per-CPU counters could be registered as per-CPU timecounters with
>>>> set_cputicker() - the main criteria there is a speed.
>>>> =A0- 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.
>>>> =A0- 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.
>>>> =A0- if there is any other timer hardware - it also should be register=
ed -
>>>> 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 =A0can 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- 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 =3D 0;
-static u_int32_t counter_lower_last =3D 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 =3D DPCPU_GET(counter_upper);
+ t_lower_last =3D DPCPU_GET(counter_lower_last);
+ } while (t_upper !=3D DPCPU_GET(counter_upper));
+
ticktock =3D mips_rd_count();
- critical_enter();
- if (ticktock < counter_lower_last)
- counter_upper++;
- counter_lower_last =3D ticktock;
+
critical_exit();
- ret =3D ((uint64_t) counter_upper << 32) | counter_lower_last;
+ /* COUNT register wrapped around */
+ if (ticktock < t_lower_last)
+ t_upper++;
+
+ ret =3D ((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 =3D 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.
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinH1itTauurEFCZVyz%2BW4T02niPV1jQeNART1Gm>