Re: [Qemu-devel] [PATCH v8 1/4] hw/ptimer: Fix issues caused by the adjusted timer limit value

[Dmitry Osipenko <digetx@gmail.com>]

06.01.2016 15:15, Peter Crosthwaite пишет:
>> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
>> index edf077c..035af97 100644
>> --- a/hw/core/ptimer.c
>> +++ b/hw/core/ptimer.c
>> @@ -34,20 +34,39 @@ static void ptimer_trigger(ptimer_state *s)
>>
>>   static void ptimer_reload(ptimer_state *s)
>>   {
>> -    if (s->delta == 0) {
>> +    uint32_t period_frac = s->period_frac;
>> +    uint64_t period = s->period;
>> +    uint64_t delta = s->delta;
>> +    uint64_t limit = s->limit;
>> +
>
> Localising these variables is out of scope of the change. I think you can
> just use s->foo and if you want to cleanup, it should be a separate
> patch.
>

Okay

>> +    if (delta == 0) {
>>           ptimer_trigger(s);
>> -        s->delta = s->limit;
>> +        delta = limit;
>>       }
>> -    if (s->delta == 0 || s->period == 0) {
>> +    if (delta == 0 || period == 0) {
>>           fprintf(stderr, "Timer with period zero, disabling\n");
>>           s->enabled = 0;
>>           return;
>>       }
>>
>> +    /*
>> +     * Artificially limit timeout rate to something
>> +     * achievable under QEMU.  Otherwise, QEMU spends all
>> +     * its time generating timer interrupts, and there
>> +     * is no forward progress.
>> +     * About ten microseconds is the fastest that really works
>> +     * on the current generation of host machines.
>> +     */
>> +
>> +    if ((s->enabled == 1) && (limit * period < 10000)) {
>> +        period = 10000 / limit;
>> +        period_frac = 0;
>> +    }
>> +
>
> I think it should be ok to just update s->period and s->period_frac ...
>

No, then it would be irreversibly lost. What if'd decide to change the limit to 
some large value?

>>       s->last_event = s->next_event;
>> -    s->next_event = s->last_event + s->delta * s->period;
>> -    if (s->period_frac) {
>> -        s->next_event += ((int64_t)s->period_frac * s->delta) >> 32;
>> +    s->next_event = s->last_event + delta * period;
>> +    if (period_frac) {
>> +        s->next_event += ((int64_t)period_frac * delta) >> 32;
>>       }
>>       timer_mod(s->timer, s->next_event);
>>   }
>> @@ -82,6 +101,8 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>               uint64_t div;
>>               int clz1, clz2;
>>               int shift;
>> +            uint32_t period_frac = s->period_frac;
>> +            uint64_t period = s->period;
>>
>>               /* We need to divide time by period, where time is stored in
>>                  rem (64-bit integer) and period is stored in period/period_frac
>> @@ -93,8 +114,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>                  backwards.
>>               */
>>
>> +            if ((s->enabled == 1) && (s->limit * period < 10000)) {
>> +                period = 10000 / s->limit;
>> +                period_frac = 0;
>> +            }
>> +
>
> ... and then this (and the local variables) become obsolete. Can get_count()
> blindly use the period and period_frac as used by ptimer_reload?
>
>>               rem = s->next_event - now;
>> -            div = s->period;
>> +            div = period;
>>
>>               clz1 = clz64(rem);
>>               clz2 = clz64(div);
>> @@ -103,13 +129,13 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>               rem <<= shift;
>>               div <<= shift;
>>               if (shift >= 32) {
>> -                div |= ((uint64_t)s->period_frac << (shift - 32));
>> +                div |= ((uint64_t)period_frac << (shift - 32));
>>               } else {
>>                   if (shift != 0)
>> -                    div |= (s->period_frac >> (32 - shift));
>> +                    div |= (period_frac >> (32 - shift));
>>                   /* Look at remaining bits of period_frac and round div up if
>>                      necessary.  */
>> -                if ((uint32_t)(s->period_frac << shift))
>> +                if ((uint32_t)(period_frac << shift))
>>                       div += 1;
>>               }
>>               counter = rem / div;
>> @@ -181,19 +207,6 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq)
>>      count = limit.  */
>>   void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
>>   {
>> -    /*
>> -     * Artificially limit timeout rate to something
>> -     * achievable under QEMU.  Otherwise, QEMU spends all
>> -     * its time generating timer interrupts, and there
>> -     * is no forward progress.
>> -     * About ten microseconds is the fastest that really works
>> -     * on the current generation of host machines.
>> -     */
>> -
>> -    if (!use_icount && limit * s->period < 10000 && s->period) {
>
> This original rate limiting code is gated on icount, so I think then
> new way should be the same.
>

Shoot :) That's second time I'm missing it. Good catch!

> Regards,
> Peter
>
>> -        limit = 10000 / s->period;
>> -    }
>> -
>>       s->limit = limit;
>>       if (reload)
>>           s->delta = limit;
>> --
>> 2.6.4
>>


-- 
Dmitry