Date: Tue, 3 Aug 2010 08:15:26 -0700 From: Neel Natu <neelnatu@gmail.com> To: Alexander Motin <mav@freebsd.org> Cc: Randall Stewart <rrs@freebsd.org>, freebsd-mips@freebsd.org, Neel Natu <neel@freebsd.org> Subject: Re: [RFC] Event timers on MIPS Message-ID: <AANLkTimqAitBoFyKT-8bE%2BS6%2B1wjnso569E3exrY6EWB@mail.gmail.com> In-Reply-To: <4C555CF7.5080101@FreeBSD.org> 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> <AANLkTinH1itTauurEFCZVyz%2BW4T02niPV1jQeNART1Gm@mail.gmail.com> <4C555CF7.5080101@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimqAitBoFyKT-8bE%2BS6%2B1wjnso569E3exrY6EWB>