Message ID | 1296510679-12268-2-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On Mon, Jan 31, 2011 at 10:51:17PM +0100, Paolo Bonzini wrote: > Suggested by Aurelien Jarno. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > qemu-timer.c | 30 +++++++++++++++--------------- > 1 files changed, 15 insertions(+), 15 deletions(-) Reviewed-by: Aurelien Jarno <aurelien@aurel32.net> > diff --git a/qemu-timer.c b/qemu-timer.c > index db1ec49..60283a8 100644 > --- a/qemu-timer.c > +++ b/qemu-timer.c > @@ -197,8 +197,8 @@ static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t) > t->rearm(t); > } > > -/* TODO: MIN_TIMER_REARM_US should be optimized */ > -#define MIN_TIMER_REARM_US 250 > +/* TODO: MIN_TIMER_REARM_NS should be optimized */ > +#define MIN_TIMER_REARM_NS 250000 > > #ifdef _WIN32 > > @@ -698,11 +698,11 @@ int64_t qemu_next_deadline(void) > > if (active_timers[QEMU_CLOCK_VIRTUAL]) { > delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time - > - qemu_get_clock(vm_clock); > + qemu_get_clock_ns(vm_clock); > } > if (active_timers[QEMU_CLOCK_HOST]) { > int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time - > - qemu_get_clock(host_clock); > + qemu_get_clock_ns(host_clock); > if (hdelta < delta) > delta = hdelta; > } > @@ -727,17 +727,17 @@ static uint64_t qemu_next_deadline_dyntick(void) > if (use_icount) > delta = INT32_MAX; > else > - delta = (qemu_next_deadline() + 999) / 1000; > + delta = qemu_next_deadline(); > > if (active_timers[QEMU_CLOCK_REALTIME]) { > rtdelta = (active_timers[QEMU_CLOCK_REALTIME]->expire_time - > - qemu_get_clock(rt_clock))*1000; > + qemu_get_clock_ns(rt_clock)); > if (rtdelta < delta) > delta = rtdelta; > } > > - if (delta < MIN_TIMER_REARM_US) > - delta = MIN_TIMER_REARM_US; > + if (delta < MIN_TIMER_REARM_NS) > + delta = MIN_TIMER_REARM_NS; > > return delta; > } > @@ -887,8 +887,8 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) > { > timer_t host_timer = (timer_t)(long)t->priv; > struct itimerspec timeout; > - int64_t nearest_delta_us = INT64_MAX; > - int64_t current_us; > + int64_t nearest_delta_ns = INT64_MAX; > + int64_t current_ns; > > assert(alarm_has_dynticks(t)); > if (!active_timers[QEMU_CLOCK_REALTIME] && > @@ -896,7 +896,7 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) > !active_timers[QEMU_CLOCK_HOST]) > return; > > - nearest_delta_us = qemu_next_deadline_dyntick(); > + nearest_delta_ns = qemu_next_deadline_dyntick(); > > /* check whether a timer is already running */ > if (timer_gettime(host_timer, &timeout)) { > @@ -904,14 +904,14 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) > fprintf(stderr, "Internal timer error: aborting\n"); > exit(1); > } > - current_us = timeout.it_value.tv_sec * 1000000 + timeout.it_value.tv_nsec/1000; > - if (current_us && current_us <= nearest_delta_us) > + current_ns = timeout.it_value.tv_sec * 1000000000LL + timeout.it_value.tv_nsec; > + if (current_ns && current_ns <= nearest_delta_ns) > return; > > timeout.it_interval.tv_sec = 0; > timeout.it_interval.tv_nsec = 0; /* 0 for one-shot timer */ > - timeout.it_value.tv_sec = nearest_delta_us / 1000000; > - timeout.it_value.tv_nsec = (nearest_delta_us % 1000000) * 1000; > + timeout.it_value.tv_sec = nearest_delta_ns / 1000000000; > + timeout.it_value.tv_nsec = nearest_delta_ns % 1000000000; > if (timer_settime(host_timer, 0 /* RELATIVE */, &timeout, NULL)) { > perror("settime"); > fprintf(stderr, "Internal timer error: aborting\n"); > -- > 1.7.3.4 > > > >
On Mon, Jan 31, 2011 at 04:17:52PM -0600, Anthony Liguori wrote: > On 01/31/2011 03:51 PM, Paolo Bonzini wrote: >> Suggested by Aurelien Jarno. >> >> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> >> > > Something I've found is that we have a lot of bugs that are the result > of unit conversions when the unit can't be mapped directly to base 10. > > This happens in both the PIT and RTC and probably every other fixed bus > speed based clock we support. > > I think it would be better to have a Unit argument to qemu_get_clock to > specify the desired units. That way, we could request NS as the time > unit or something like PC_FREQ0 which would map to the bus speed of the > PIT. It's more or less what is already done with the two versions of the functions, qemu_get_clock() which can return different unit depending on what you ask (and in my opinion should simply disappear because it's the best way to have bugs), and qemu_get_clock_ns() that always return it in nanoseconds. What you suggests is a generalization of this second function to different units than ns. That's an option, but we should pay attention that given we work in integer, a conversion decrease the precision, so it should usually be done at the last moment to keep the precision correct (assuming ns is precise enough, which I guess is true). In any case I think this patch series should be applied as it fixes a real bug. It's already a step in the right direction, as it removes useless conversions, as all the involved functions use ns in fine.
On 02/01/2011 07:47 PM, Aurelien Jarno wrote: > qemu_get_clock() which can return different unit depending on what > you ask (and in my opinion should simply disappear because it's the > best way to have bugs), Agreed. Paolo
diff --git a/qemu-timer.c b/qemu-timer.c index db1ec49..60283a8 100644 --- a/qemu-timer.c +++ b/qemu-timer.c @@ -197,8 +197,8 @@ static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t) t->rearm(t); } -/* TODO: MIN_TIMER_REARM_US should be optimized */ -#define MIN_TIMER_REARM_US 250 +/* TODO: MIN_TIMER_REARM_NS should be optimized */ +#define MIN_TIMER_REARM_NS 250000 #ifdef _WIN32 @@ -698,11 +698,11 @@ int64_t qemu_next_deadline(void) if (active_timers[QEMU_CLOCK_VIRTUAL]) { delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time - - qemu_get_clock(vm_clock); + qemu_get_clock_ns(vm_clock); } if (active_timers[QEMU_CLOCK_HOST]) { int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time - - qemu_get_clock(host_clock); + qemu_get_clock_ns(host_clock); if (hdelta < delta) delta = hdelta; } @@ -727,17 +727,17 @@ static uint64_t qemu_next_deadline_dyntick(void) if (use_icount) delta = INT32_MAX; else - delta = (qemu_next_deadline() + 999) / 1000; + delta = qemu_next_deadline(); if (active_timers[QEMU_CLOCK_REALTIME]) { rtdelta = (active_timers[QEMU_CLOCK_REALTIME]->expire_time - - qemu_get_clock(rt_clock))*1000; + qemu_get_clock_ns(rt_clock)); if (rtdelta < delta) delta = rtdelta; } - if (delta < MIN_TIMER_REARM_US) - delta = MIN_TIMER_REARM_US; + if (delta < MIN_TIMER_REARM_NS) + delta = MIN_TIMER_REARM_NS; return delta; } @@ -887,8 +887,8 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) { timer_t host_timer = (timer_t)(long)t->priv; struct itimerspec timeout; - int64_t nearest_delta_us = INT64_MAX; - int64_t current_us; + int64_t nearest_delta_ns = INT64_MAX; + int64_t current_ns; assert(alarm_has_dynticks(t)); if (!active_timers[QEMU_CLOCK_REALTIME] && @@ -896,7 +896,7 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) !active_timers[QEMU_CLOCK_HOST]) return; - nearest_delta_us = qemu_next_deadline_dyntick(); + nearest_delta_ns = qemu_next_deadline_dyntick(); /* check whether a timer is already running */ if (timer_gettime(host_timer, &timeout)) { @@ -904,14 +904,14 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t) fprintf(stderr, "Internal timer error: aborting\n"); exit(1); } - current_us = timeout.it_value.tv_sec * 1000000 + timeout.it_value.tv_nsec/1000; - if (current_us && current_us <= nearest_delta_us) + current_ns = timeout.it_value.tv_sec * 1000000000LL + timeout.it_value.tv_nsec; + if (current_ns && current_ns <= nearest_delta_ns) return; timeout.it_interval.tv_sec = 0; timeout.it_interval.tv_nsec = 0; /* 0 for one-shot timer */ - timeout.it_value.tv_sec = nearest_delta_us / 1000000; - timeout.it_value.tv_nsec = (nearest_delta_us % 1000000) * 1000; + timeout.it_value.tv_sec = nearest_delta_ns / 1000000000; + timeout.it_value.tv_nsec = nearest_delta_ns % 1000000000; if (timer_settime(host_timer, 0 /* RELATIVE */, &timeout, NULL)) { perror("settime"); fprintf(stderr, "Internal timer error: aborting\n");
Suggested by Aurelien Jarno. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qemu-timer.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-)