On 19 October 2015 at 21:01, Dmitry Osipenko <dig...@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