Re: [Qemu-arm] [PATCH v12 8/9] hw/ptimer: Perform delayed tick instead of immediate if delta = 0

[Dmitry Osipenko]

09.03.2016 00:08, Peter Crosthwaite пишет:
> On Sat, Jan 30, 2016 at 8:43 AM, Dmitry Osipenko <address@hidden> wrote:
> > It might be necessary by some emulated HW to perform the tick after one
> > period if delta = 0. Given that it is much less churny to implement immediate
> > tick by the ptimer user itself, let's make ptimer do the delayed tick.
>
> Isn't this related to previous patch? It is kind of a rounding problem
> that will vary from timer to timer. Some timers may interpret the
> "tick" as the wrap-around back to the load value (even if that is 0)
> while others interpret the tick as the transition to zero (makes more
> sense for a one-shot). It's hard to set a universal policy here.

If by previous patch you are meaning "Fix counter - 1 ...", then no, it's a 
distinct issue/feature. I endeavored to add some sort of "flags" to ptimer in 
order to support various policies, but quickly realized that it doesn't really 
worth an effort since there are no users of those policies (currently "delta = 
0" is a forbidden case) and ptimer VMSD version would be bumped.

It's not possible (doesn't make sense) for ptimer user to implement policy 
provided by this patch by itself. Other policies could be easily supported in 
future if desired (or implemented by ptimer user).

> But is this a non-issue when we consider that event latency (usually
> interrupt latency) is undefined anyway?

It's not an issue for a timer that has quite short period. Some of the 
arm_mptimer tests would fail (in icount mode) if that policy not implemented.

BTW, I think "deferred" might be better than "delayed", however not sure :)

> Regards,
> Peter

> > Signed-off-by: Dmitry Osipenko <address@hidden>
> > ---
> >   hw/core/ptimer.c | 34 +++++++++++++++-------------------
> >   1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> > index b2044fb..bcd090c 100644
> > --- a/hw/core/ptimer.c
> > +++ b/hw/core/ptimer.c
> > @@ -36,19 +36,7 @@ static void ptimer_reload(ptimer_state *s)
> >   {
> >       uint32_t period_frac = s->period_frac;
> >       uint64_t period = s->period;
> > -
> > -    if (s->delta == 0) {
> > -        ptimer_trigger(s);
> > -    }
> > -
> > -    if (s->delta == 0 && s->enabled == 1) {
> > -        s->delta = s->limit;
> > -    }
> > -
> > -    if (s->delta == 0) {
> > -        ptimer_stop(s);
> > -        return;
> > -    }
> > +    uint64_t delta = MAX(1, s->delta);
> >
> >       /*
> >        * Artificially limit timeout rate to something
> > @@ -59,15 +47,15 @@ static void ptimer_reload(ptimer_state *s)
> >        * on the current generation of host machines.
> >        */
> >
> > -    if (s->enabled == 1 && (s->delta * period < 10000) && !use_icount) {
> > -        period = 10000 / s->delta;
> > +    if (s->enabled == 1 && (delta * period < 10000) && !use_icount) {
> > +        period = 10000 / delta;
> >           period_frac = 0;
> >       }
> >
> >       s->last_event = s->next_event;
> > -    s->next_event = s->last_event + s->delta * period;
> > +    s->next_event = s->last_event + delta * period;
> >       if (period_frac) {
> > -        s->next_event += ((int64_t)period_frac * s->delta) >> 32;
> > +        s->next_event += ((int64_t)period_frac * delta) >> 32;
> >       }
> >       timer_mod(s->timer, s->next_event);
> >   }
> > @@ -75,8 +63,16 @@ static void ptimer_reload(ptimer_state *s)
> >   static void ptimer_tick(void *opaque)
> >   {
> >       ptimer_state *s = (ptimer_state *)opaque;
> > -    s->delta = 0;
> > -    ptimer_reload(s);
> > +
> > +    s->delta = (s->enabled == 1) ? s->limit : 0;
> > +
> > +    if (s->delta == 0) {
> > +        s->enabled = 0;
> > +    } else {
> > +        ptimer_reload(s);
> > +    }
> > +
> > +    ptimer_trigger(s);
> >   }
> >
> >   uint64_t ptimer_get_count(ptimer_state *s)
> > --
> > 2.7.0


--
Dmitry