[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original lim
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/2] hw/ptimer: Set delta to the original limit on reload in ptimer_set_limit() |
Date: |
Fri, 23 Oct 2015 15:07:05 +0100 |
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