On 10 June 2016 at 01:59, Andrew Jeffery <and...@aj.id.au> wrote:
> On Thu, 2016-06-09 at 19:15 +0100, Peter Maydell wrote:
>> On 27 May 2016 at 06:08, Andrew Jeffery <and...@aj.id.au> wrote:
>> >
>> > Value matching allows Linux to boot with CONFIG_NO_HZ_IDLE=y on the
>> > palmetto-bmc machine. Two match registers are provided for each timer.
>> >
>> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
>> > ---
>> >
>> > The change pulls out ptimer in favour of the regular timer infrastructure.
>> > As a
>> > consequence it implements the conversions between ticks and time which
>> > feels a
>> > little tedious. Any comments there would be appreciated.
>> Couple of minor comments below.
>>
>> >
>> > hw/timer/aspeed_timer.c | 135
>> > ++++++++++++++++++++++++++++++----------
>> > include/hw/timer/aspeed_timer.h | 6 +-
>> > 2 files changed, 105 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> > index 4b94808821b4..905de0f788b2 100644
>> > --- a/hw/timer/aspeed_timer.c
>> > +++ b/hw/timer/aspeed_timer.c
>> > @@ -9,13 +9,12 @@
>> > * the COPYING file in the top-level directory.
>> > */
>> >
>> > +#include
>> > #include "qemu/osdep.h"
>> osdep.h must always be the first include.
>
> Thanks for picking that up.
If you like you can run scripts/clean-includes file ...
and it will automatically make any necessary changes like this to
your files.
>> > +static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t
>> > now_ns)
>> > +{
>> > + uint64_t delta_ns = now_ns - MIN(now_ns, t->start);
>> > + uint32_t ticks = (uint32_t) floor(delta_ns / calculate_period(t));
>> Do we really need to do this with floating point ?
>
> I went with floating point to avoid accumulating errors from truncation
> by integer division. At eg 24MHz truncation increases the tick rate by
> approximately 1 in 63. We're using floor() here, but that only
> truncates at the end of the calculation, so the fractional current
> tick.
Right, but you only have a risk of truncation because of the way
you've structured the calculation. You have
floor(delta_ns / calculate_period())
== floor(delta_ns / (calculate_rate() / NS_PER_SEC))
== floor((delta_ns * NS_PER_SEC) / calculate_rate())
and calculate_rate() returns either 1000000 or 24000000.
So you have a 64 bit delta_ns, a 32 bit NANOSECONDS_PER_SECOND
and a 32 bit frequency. We have a function for doing this
accurately with integer arithmetic: muldiv64() (which takes
a 64 bit a, 32 bit b and 32 bit c and returns (a*b)/c using
an intermediate 96 bit precision to avoid overflow).
Doing ticks-to-ns and ns-to-ticks with muldiv64() is pretty
standard (grep the codebase and you'll see a lot of it).
> Having said that, grep'ing around under {,include/}hw/ doesn't show a
> lot of use of floating point. It looks like we are explicitly avoiding
> it, however with a quick search I didn't find it mentioned in any of
> the docs. What's the reasoning? Should I use a fixed-point approach
> like ptimer?
My view is that hardware doesn't generally use floating point
so it's odd to see it in a software model of hardware. Double
doesn't actually get you any more bits than uint64_t anyway...
>> > + return calculate_next(t);
>> Why is this recursing ?
>
> The common case is that we return during iterating over seq array i.e.
> we're anticipating another match event (either from the match values or
> the timer reaching zero). If not then we've overrun, so we reinitialise
> the timer by resetting the 'start' reference point then searching again
> for the next event (iterating over seq). As the search for the next
> event is encoded in the function, I've recursed to reuse it.
>
> Would you prefer a loop here?
>
> Considering the two approaches (recursion vs loop), if TCO doesn't
> apply we could blow the stack, and with loops or TCO it's possible we
> could spin here indefinitely if the timer period is shorter than the
> time it takes to recalculate. Arguably, not crashing is better so I'm
> happy to rework it.
Yes, I'd prefer a loop -- TCO is an optimization, not a guarantee
by the compiler.
> As an aside, the approach for reinitialising start skips countdown
> periods that were completely missed through high service latency, and
> there will be no interrupts issued for the missing events. Is that
> reasonable?
If the countdown period is so short that we can't service it
in time then the system is probably not going to be functional
anyway, so I don't think it matters very much. (In ptimer we
impose an artificial limit on the timeout rate for a periodic timer
and just refuse to fire any faster than that: see ptimer_reload().)
thanks
-- PMM
