On Wed, Jan 6, 2016 at 5:25 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> 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?
>
Ok makes sense.
>
>>> 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;
>>> + }
>>> +
As this now needs to be kept, another note I had is it should probably
go before the block comment as the comment relates specifically to the
math below.
Regards,
Peter
>>
>>
>> ... 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
