Re: [Qemu-devel] [PATCH v8 2/4] hw/ptimer: Perform tick and counter wrap around if timer already expired

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Tue, Jan 05, 2016 at 05:33:27AM +0300, Dmitry Osipenko wrote:
> ptimer_get_count() might be called while QEMU timer already been expired.
> In that case ptimer would return counter = 0, which might be undesirable
> in case of polled timer. Do counter wrap around for periodic timer to keep
> it distributed.
> 
> In addition, there is no reason to keep expired timer tick deferred, so
> just perform the tick from ptimer_get_count().
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/core/ptimer.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 035af97..96a6c7a 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -85,15 +85,21 @@ static void ptimer_tick(void *opaque)
>  
>  uint64_t ptimer_get_count(ptimer_state *s)
>  {
> +    int enabled = s->enabled;

Variable localisation not needed.

>      int64_t now;
> +    int64_t next;
>      uint64_t counter;
> +    int expired;
> +    int oneshot;

Variable defs can be localised to the if (enabled) (even though now
in original code doesn't do that).

>  
> -    if (s->enabled) {
> +    if (enabled) {
>          now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        next = s->next_event;
> +        expired = (now - next >= 0);
> +        oneshot = (enabled == 2);
>          /* Figure out the current counter value.  */

This comment is now out of place.

> -        if (now - s->next_event > 0
> -            || s->period == 0) {
> -            /* Prevent timer underflowing if it should already have
> +        if (s->period == 0 || (expired && oneshot)) {
> +            /* Prevent one-shot timer underflowing if it should already have
>                 triggered.  */
>              counter = 0;
>          } else {
> @@ -114,12 +120,12 @@ uint64_t ptimer_get_count(ptimer_state *s)
>                 backwards.
>              */
>  
> -            if ((s->enabled == 1) && (s->limit * period < 10000)) {
> +            if (!oneshot && (s->limit * period < 10000)) {
>                  period = 10000 / s->limit;
>                  period_frac = 0;
>              }
>  
> -            rem = s->next_event - now;
> +            rem = expired ? now - next : next - now;
>              div = period;
>  
>              clz1 = clz64(rem);
> @@ -139,6 +145,23 @@ uint64_t ptimer_get_count(ptimer_state *s)
>                      div += 1;
>              }
>              counter = rem / div;
> +
> +            if (expired) {
> +                /* Wrap around periodic counter.  */
> +                counter = s->delta = s->limit - counter % s->limit;

Why do you update the delta here?

Also can you just get ptimer_reload to do the modulo math for you? If the
timer is !oneshot and expired, then you call ptimer_reload anyway,
which will update next_event. When the expired test returns false
you can just reliably use the original logic involving now and next.

> +            }
> +        }
> +
> +        if (expired) {
> +            if (oneshot) {

This if-else has a lot of common structure with the one above. I think
they could be merged.

Regards,
Peter

> +                ptimer_tick(s);
> +            } else {
> +                /* Don't use ptimer_tick() for the periodic timer since it
> +                 * would reset the delta value.
> +                 */
> +                ptimer_trigger(s);
> +                ptimer_reload(s);
> +            }
>          }
>      } else {
>          counter = s->delta;
> -- 
> 2.6.4
>