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

[Peter Crosthwaite]

On Wed, Jan 6, 2016 at 5:12 AM, Dmitry Osipenko <address@hidden> wrote:
> 06.01.2016 15:17, Peter Crosthwaite пишет:
>
>> 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 <address@hidden>
>>> ---
>>>   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).
>>
>
> Yeah, I just tried to keep original style here.
>
>>>
>>> -    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.
>>
>
> Okay, will fix it.
>
>
>>> -        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?
>>
>
> Because we would want to schedule next tick based on current wrapped around
> counter value and not some arbitrary delta.
>

So looking at ptimer_reload(), the new schedule is done relative to
the VM clock of the when the tick was expected to hit, not the current
time. But this new delta is going to be relative to the now time and
then used to update the next tick which will happen relative to
next_event. Unless you stop or scale the timer, I don't think you need
to do delta manipulation?

>> 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.
>>
>
> Yes, that's what I changed in V9. Have you received it?
>
> https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00272.html
>

Just had a look.

V9 still has the modulo I think?:

+            if (expired && (counter != 0)) {
+                /* Wrap around periodic counter.  */
+                counter = s->delta = s->limit - counter % s->limit;
+            }

Regards,
Peter

>>> +            }
>>> +        }
>>> +
>>> +        if (expired) {
>>> +            if (oneshot) {
>>
>>
>> This if-else has a lot of common structure with the one above. I think
>> they could be merged.
>>
>
> That's a good suggestion, will do it in V10. Thanks.
>
>
>> 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
>>>
>
>
> --
> Dmitry