Date: Wed, 21 Jul 2010 23:33:10 -0700 From: Neel Natu <neelnatu@gmail.com> To: Alexander Motin <mav@freebsd.org> Cc: freebsd-mips@freebsd.org Subject: Re: [RFC] Event timers on MIPS Message-ID: <AANLkTinvboQiT65Nc7901uwWKUNyaNh9HbX0yuFVGpnc@mail.gmail.com> In-Reply-To: <4C47D8CD.7020209@FreeBSD.org> References: <4C41A248.8090605@FreeBSD.org> <4C4698D6.2090104@FreeBSD.org> <AANLkTinrTzPZF0NbnT2e8kf8E4KtCwXUfFH7i1nBP_kz@mail.gmail.com> <4C47D8CD.7020209@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Alexander, On Wed, Jul 21, 2010 at 10:36 PM, Alexander Motin <mav@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 >> =A0 =A0 mips_timer_init_params(). Otherwise the platform-specific CPU ti= ckers >> =A0 =A0used 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 uin= t32_t. > > Fixed. > >> 3. Is there a race between setting 'cycles_per_tick' in clock_start() an= d >> =A0 =A0 clock_intr()? Would it be better to write the compare register f= irst >> =A0 =A0 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. >> =A0 =A0 If I understand the code correctly we are liable to get clock in= terrupts >> =A0 =A0 even afer the clock is stopped when the CP0 COUNT register match= es >> =A0 =A0 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 ti= mer >> =A0 =A0 in clock_start(). > > Fixed. > >> 5. I think the entire 'lost_ticks' processing in clock_intr() should be >> =A0 =A0 conditional on (cycles_per_tick > 0). Without this we may incorr= ectly >> =A0 =A0 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 >> =A0 =A0 'et_min_period' and 'et_max_period'? It is not completely obviou= s 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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinvboQiT65Nc7901uwWKUNyaNh9HbX0yuFVGpnc>