[Qemu-devel] [PATCH 0/3] Simplify and fix alarm deadline computation

[Qemu-devel] [PATCH 0/3] Simplify and fix alarm deadline computation

[Paolo Bonzini]

This is a rebased and updated version of the series to fix alarm
deadline computation.  I adopted Aurelien's suggestion to change
everything to nanoseconds.  I also did more testing than just
compiling, by booting with -icount.

Paolo Bonzini (3):
  use nanoseconds everywhere for timeout computation
  Correct alarm deadline computation
  Unify alarm deadline computation

 qemu-timer.c |   65 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 33 insertions(+), 32 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/3] use nanoseconds everywhere for timeout computation

[Paolo Bonzini]

Suggested by Aurelien Jarno.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

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

[Qemu-devel] [PATCH 2/3] Correct alarm deadline computation

[Paolo Bonzini]

When the QEMU_CLOCK_HOST clock was added, computation of its
deadline was added to qemu_next_deadline, which is correct but
incomplete.

I noticed this by reading the very convoluted rules whereby
qemu_next_deadline_dyntick is computed, which miss QEMU_CLOCK_HOST
when use_icount is true.  This patch inlines qemu_next_deadline
into qemu_next_deadline_dyntick, and then corrects the logic to skip
only QEMU_CLOCK_VIRTUAL when use_icount is true.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-timer.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 60283a8..c19d0a2 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -724,11 +724,18 @@ static uint64_t qemu_next_deadline_dyntick(void)
     int64_t delta;
     int64_t rtdelta;
 
-    if (use_icount)
+    if (!use_icount && active_timers[QEMU_CLOCK_VIRTUAL]) {
+        delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time -
+                     qemu_get_clock(vm_clock);
+    } else {
         delta = INT32_MAX;
-    else
-        delta = qemu_next_deadline();
-
+    }
+    if (active_timers[QEMU_CLOCK_HOST]) {
+        int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time -
+                 qemu_get_clock(host_clock);
+        if (hdelta < delta)
+            delta = hdelta;
+    }
     if (active_timers[QEMU_CLOCK_REALTIME]) {
         rtdelta = (active_timers[QEMU_CLOCK_REALTIME]->expire_time -
                  qemu_get_clock_ns(rt_clock));
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/3] Unify alarm deadline computation

[Paolo Bonzini]

This patch shows how using the correct formula for
qemu_next_deadline_dyntick can simplify the code of
host_alarm_handler and eliminate useless duplication.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c |   28 +++++++++++-----------------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index c19d0a2..d3c426c 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -635,6 +635,8 @@ void qemu_run_all_timers(void)
     qemu_run_timers(host_clock);
 }
 
+static int64_t qemu_next_alarm_deadline(void);
+
 #ifdef _WIN32
 static void CALLBACK host_alarm_handler(UINT uTimerID, UINT uMsg,
                                         DWORD_PTR dwUser, DWORD_PTR dw1,
@@ -677,14 +679,7 @@ static void host_alarm_handler(int host_signum)
     }
 #endif
     if (alarm_has_dynticks(t) ||
-        (!use_icount &&
-            qemu_timer_expired(active_timers[QEMU_CLOCK_VIRTUAL],
-                               qemu_get_clock(vm_clock))) ||
-        qemu_timer_expired(active_timers[QEMU_CLOCK_REALTIME],
-                           qemu_get_clock(rt_clock)) ||
-        qemu_timer_expired(active_timers[QEMU_CLOCK_HOST],
-                           qemu_get_clock(host_clock))) {
-
+        qemu_next_alarm_deadline () <= 0) {
         t->expired = alarm_has_dynticks(t);
         t->pending = 1;
         qemu_notify_event();
@@ -715,11 +710,7 @@ int64_t qemu_next_deadline(void)
 
 #ifndef _WIN32
 
-#if defined(__linux__)
-
-#define RTC_FREQ 1024
-
-static uint64_t qemu_next_deadline_dyntick(void)
+static int64_t qemu_next_alarm_deadline(void)
 {
     int64_t delta;
     int64_t rtdelta;
@@ -743,12 +734,13 @@ static uint64_t qemu_next_deadline_dyntick(void)
             delta = rtdelta;
     }
 
-    if (delta < MIN_TIMER_REARM_NS)
-        delta = MIN_TIMER_REARM_NS;
-
     return delta;
 }
 
+#if defined(__linux__)
+
+#define RTC_FREQ 1024
+
 static void enable_sigio_timer(int fd)
 {
     struct sigaction act;
@@ -903,7 +895,9 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t)
         !active_timers[QEMU_CLOCK_HOST])
         return;
 
-    nearest_delta_ns = qemu_next_deadline_dyntick();
+    nearest_delta_ns = qemu_next_alarm_deadline();
+    if (nearest_delta_ns < MIN_TIMER_REARM_NS)
+        nearest_delta_ns = MIN_TIMER_REARM_NS;
 
     /* check whether a timer is already running */
     if (timer_gettime(host_timer, &timeout)) {
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 1/3] use nanoseconds everywhere for timeout computation

[Anthony Liguori]

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.

That eliminates all the unit conversion code throughout QEMU and thereby 
a big class of bugs.

Regards,

Anthony Liguori

> ---
>   qemu-timer.c |   30 +++++++++++++++---------------
>   1 files changed, 15 insertions(+), 15 deletions(-)
>
> 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");
>    

[Qemu-devel] Re: [PATCH 2/3] Correct alarm deadline computation

[Jan Kiszka]

On 2011-01-31 22:51, Paolo Bonzini wrote:
> When the QEMU_CLOCK_HOST clock was added, computation of its
> deadline was added to qemu_next_deadline, which is correct but
> incomplete.
> 
> I noticed this by reading the very convoluted rules whereby
> qemu_next_deadline_dyntick is computed, which miss QEMU_CLOCK_HOST
> when use_icount is true.  This patch inlines qemu_next_deadline
> into qemu_next_deadline_dyntick, and then corrects the logic to skip
> only QEMU_CLOCK_VIRTUAL when use_icount is true.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  qemu-timer.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 60283a8..c19d0a2 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -724,11 +724,18 @@ static uint64_t qemu_next_deadline_dyntick(void)
>      int64_t delta;
>      int64_t rtdelta;
>  
> -    if (use_icount)
> +    if (!use_icount && active_timers[QEMU_CLOCK_VIRTUAL]) {
> +        delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time -
> +                     qemu_get_clock(vm_clock);
> +    } else {
>          delta = INT32_MAX;
> -    else
> -        delta = qemu_next_deadline();
> -
> +    }
> +    if (active_timers[QEMU_CLOCK_HOST]) {
> +        int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time -
> +                 qemu_get_clock(host_clock);
> +        if (hdelta < delta)
> +            delta = hdelta;
> +    }
>      if (active_timers[QEMU_CLOCK_REALTIME]) {
>          rtdelta = (active_timers[QEMU_CLOCK_REALTIME]->expire_time -
>                   qemu_get_clock_ns(rt_clock));

Looks good to me. I guess this applies without the first patch? Then it
should go in (unless you are working on a new version for 1/3).

Thanks for fixing this.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/3] Correct alarm deadline computation

[Paolo Bonzini]

On 02/01/2011 02:01 PM, Jan Kiszka wrote:
> Looks good to me. I guess this applies without the first patch? Then it
> should go in (unless you are working on a new version for 1/3).

It's wrong without the first patch (micro instead of nanoseconds). 
However, I read Anthony's message as a suggestion rather than a rejection.

Paolo

[Qemu-devel] Re: [PATCH 2/3] Correct alarm deadline computation

[Jan Kiszka]

On 2011-02-01 14:04, Paolo Bonzini wrote:
> On 02/01/2011 02:01 PM, Jan Kiszka wrote:
>> Looks good to me. I guess this applies without the first patch? Then it
>> should go in (unless you are working on a new version for 1/3).
> 
> It's wrong without the first patch (micro instead of nanoseconds). 
> However, I read Anthony's message as a suggestion rather than a rejection.
> 

Yes, it's probably a better idea anyway to do this stepwise.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/3] use nanoseconds everywhere for timeout computation

[Aurelien Jarno]

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
> 
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/3] use nanoseconds everywhere for timeout computation

[Aurelien Jarno]

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.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH 1/3] use nanoseconds everywhere for timeout computation

[Paolo Bonzini]

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