Patchwork [1/2] Correct alarm deadline computation

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 29, 2011, 11 a.m.
Message ID <1296298848-3080-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/80936/
State New
Headers show

Comments

Paolo Bonzini - Jan. 29, 2011, 11 a.m.
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 while trying to make sense of the rules whereby
qemu_next_deadline_dyntick is computed, which miss QEMU_CLOCK_HOST
when use_icount is true.  Looking at the history showed this to
be just an oversight, as the next patch shows clearly.

This patch inlines qemu_next_deadline into qemu_next_deadline_dyntick,
and corrects the logic so that only QEMU_CLOCK_VIRTUAL is skipped for
use_icount == 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(-)
Aurelien Jarno - Jan. 30, 2011, 4:46 p.m.
On Sat, Jan 29, 2011 at 12:00:47PM +0100, 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 while trying to make sense of the rules whereby
> qemu_next_deadline_dyntick is computed, which miss QEMU_CLOCK_HOST
> when use_icount is true.  Looking at the history showed this to
> be just an oversight, as the next patch shows clearly.
> 
> This patch inlines qemu_next_deadline into qemu_next_deadline_dyntick,
> and corrects the logic so that only QEMU_CLOCK_VIRTUAL is skipped for
> use_icount == 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 db1ec49..174fd0c 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() + 999) / 1000;
> -

This computation handled the fact that the value is returned in ns, and
we want us.

> +    }
> +    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;
> +    }

And this doesn't appear anymore after your changes.

>      if (active_timers[QEMU_CLOCK_REALTIME]) {
>          rtdelta = (active_timers[QEMU_CLOCK_REALTIME]->expire_time -
>                   qemu_get_clock(rt_clock))*1000;

Here we multiply because the value is returned in ms and we want us.

IMHO we should just use qemu_get_clock_ns() instead, replace
MIN_TIMER_REARM_US by MIN_TIMER_REARM_NS, and return a ns value
instead. Of course dynticks_rearm_timer() has to be adjusted, but it
should not be problematic given it uses ns internally.

Otherwise good catch, the issue is real, and the way to fix it looks
correct.

Patch

diff --git a/qemu-timer.c b/qemu-timer.c
index db1ec49..174fd0c 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() + 999) / 1000;
-
+    }
+    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(rt_clock))*1000;