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

[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.

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

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
>