Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit()

[Peter Maydell]

On 19 October 2015 at 21:01, Dmitry Osipenko <address@hidden> wrote:
> 19.10.2015 20:06, Peter Maydell пишет:
>>
>> Doesn't this defeat the rate limiting if the timer is enabled,
>> though? ptimer_reload() sets the underlying timer based on
>> s->delta, so if s->delta isn't the rate-limited value then the
>> timer will be incorrectly set to a very close-in value.
>>
>
> Yes, it defeats the rate limiting for the first timer expire. As I
> understand, the idea of the rate limit correction aims periodic timer only.
>
> "Otherwise, QEMU spends all its time generating timer interrupts, and there
> is no forward progress.", as stated in the comment.
>
> I think it's not a problem to get "instant" timer trigger for the first
> expire.

Yes, that makes sense. If you trigger a one-shot timer then we
should prefer accuracy, on the assumption that the next setting
of the one-shot timer will be further in the future. (The guest
could in theory still starve us of forward progress with an
infinite series of near-future one-shot timers, but we can worry
about that if we see it.)

>> I think we'll return "incorrect" values from ptimer_get_count()
>> in the "counter is running" case too, because we calculate those
>> by looking at when the underlying timer's due to expire, and
>> we set the expiry time based on the adjusted value.
>>
>
> That's a good point.
>
>> What's the underlying model we should have for what values
>> we return from reading the count if we've decided to adjust
>> the actual timer expiry with the rate limit? Should the
>> count go down from the specified value and then just hang
>> at 1 until the extended timer expiry time hits? Or something
>> else? Clearly defining what we want to happen ought to make
>> it easier to review attempts to fix it...
>>
>
> What about the following:
>
> Add additional ptimer struct member, say "limit_corrected", to check whether
> the limit was corrected or not.
>
> ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload)
> {
>         .limit_corrected = 0;
>
>         // on the limit correction:
>         .limit_corrected = (limit == 0) ? 1 : 2;

It's a bit confusing that a field that sounds like it ought
to be a bool is taking values 0/1/2.

>         limit = 10000 / s->period;
> }
>
> ptimer_get_count()
> {
>         if (enabled) {
>                 if (expired || .limit_corrected == 1) {
>                         counter = 0;
>                 } else if (.limit_corrected == 2) {
>                         counter = 1;
>                 } else {
>                         // do the counter calculations ...
>                 }
>         }
> }

Not completely sure I understand this. A comment about what
the various states limit_corrected can be in might help.

> and clear .limit_corrected on the one-shot timer start. That would bump
> ptimer VMSD version, but keep .minimum_version_id.

Given how widely ptimer is used, you should probably handle it
via a subsection, not by a version number bump.

thanks
-- PMM