| Message ID | 1444779048-10096-1-git-send-email-digetx@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 14 October 2015 at 00:30, Dmitry Osipenko <digetx@gmail.com> wrote: > ptimer_get_count() returns incorrect value for the disabled timer after > reloading the counter with a small value, because corrected limit value > is used instead of the original. > > For instance: > 1) ptimer_stop(t) > 2) ptimer_set_period(t, 1) > 3) ptimer_set_limit(t, 0, 1) > 4) ptimer_get_count(t) <-- would return 10000 instead of 0 > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > hw/core/ptimer.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c > index 8437bd6..abc3a20 100644 > --- a/hw/core/ptimer.c > +++ b/hw/core/ptimer.c > @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq) > count = limit. */ > void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) > { > + uint64_t count = limit; > + > /* > * Artificially limit timeout rate to something > * achievable under QEMU. Otherwise, QEMU spends all > @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) > > s->limit = limit; > if (reload) > - s->delta = limit; > + s->delta = count; > if (s->enabled && reload) { > s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > ptimer_reload(s); 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. 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. 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... (Calling ptimer_set_count() also bypasses the ratelimiting at the moment, incidentally.) thanks -- PMM
19.10.2015 20:06, Peter Maydell пишет: > On 14 October 2015 at 00:30, Dmitry Osipenko <digetx@gmail.com> wrote: >> ptimer_get_count() returns incorrect value for the disabled timer after >> reloading the counter with a small value, because corrected limit value >> is used instead of the original. >> >> For instance: >> 1) ptimer_stop(t) >> 2) ptimer_set_period(t, 1) >> 3) ptimer_set_limit(t, 0, 1) >> 4) ptimer_get_count(t) <-- would return 10000 instead of 0 >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> hw/core/ptimer.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c >> index 8437bd6..abc3a20 100644 >> --- a/hw/core/ptimer.c >> +++ b/hw/core/ptimer.c >> @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq) >> count = limit. */ >> void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) >> { >> + uint64_t count = limit; >> + >> /* >> * Artificially limit timeout rate to something >> * achievable under QEMU. Otherwise, QEMU spends all >> @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) >> >> s->limit = limit; >> if (reload) >> - s->delta = limit; >> + s->delta = count; >> if (s->enabled && reload) { >> s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> ptimer_reload(s); > > 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. > 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; 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 ... } } } and clear .limit_corrected on the one-shot timer start. That would bump ptimer VMSD version, but keep .minimum_version_id. > (Calling ptimer_set_count() also bypasses the ratelimiting at > the moment, incidentally.) > Should be okay if my "aiming the periodic timer only" thought is correct. > thanks > -- PMM >
On 19 October 2015 at 21:01, Dmitry Osipenko <digetx@gmail.com> 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
19.10.2015 23:01, Dmitry Osipenko пишет: > 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; > 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 ... > } > } > } > > and clear .limit_corrected on the one-shot timer start. That would bump ptimer > VMSD version, but keep .minimum_version_id. > However, that would break set_counter(). Also limit should be corrected on period / freq change. I'll work more on it.
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c index 8437bd6..abc3a20 100644 --- a/hw/core/ptimer.c +++ b/hw/core/ptimer.c @@ -180,6 +180,8 @@ void ptimer_set_freq(ptimer_state *s, uint32_t freq) count = limit. */ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) { + uint64_t count = limit; + /* * Artificially limit timeout rate to something * achievable under QEMU. Otherwise, QEMU spends all @@ -195,7 +197,7 @@ void ptimer_set_limit(ptimer_state *s, uint64_t limit, int reload) s->limit = limit; if (reload) - s->delta = limit; + s->delta = count; if (s->enabled && reload) { s->next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); ptimer_reload(s);
ptimer_get_count() returns incorrect value for the disabled timer after reloading the counter with a small value, because corrected limit value is used instead of the original. For instance: 1) ptimer_stop(t) 2) ptimer_set_period(t, 1) 3) ptimer_set_limit(t, 0, 1) 4) ptimer_get_count(t) <-- would return 10000 instead of 0 Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- hw/core/ptimer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)