[Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Gerd Hoffmann]

Kill nographic timer.  Have a global gui_timer instead.  Have the gui
timer enabled unconditionally.  We need a timer running anyway for mmio
flush, so the whole have-gui-timer-only-when-needed logic is pretty
pointless.  It also simplifies displaylisteners coming and going at
runtime, we don't need to care about the timer then as it runs anyway.

Don't allocate the timer twice in case we have two display listeners.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 console.h |    1 -
 vl.c      |   37 +++++++++++--------------------------
 2 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/console.h b/console.h
index 264a396..cdf8dee 100644
--- a/console.h
+++ b/console.h
@@ -173,7 +173,6 @@ struct DisplayAllocator {
 struct DisplayState {
     struct DisplaySurface *surface;
     void *opaque;
-    struct QEMUTimer *gui_timer;
 
     struct DisplayAllocator* allocator;
     QLIST_HEAD(, DisplayChangeListener) listeners;
diff --git a/vl.c b/vl.c
index f66f420..670705c 100644
--- a/vl.c
+++ b/vl.c
@@ -236,7 +236,7 @@ int nb_numa_nodes;
 uint64_t node_mem[MAX_NODES];
 uint64_t node_cpumask[MAX_NODES];
 
-static QEMUTimer *nographic_timer;
+static QEMUTimer *gui_timer;
 
 uint8_t qemu_uuid[16];
 
@@ -1633,22 +1633,17 @@ static void gui_update(void *opaque)
     DisplayChangeListener *dcl;
 
     qemu_flush_coalesced_mmio_buffer();
-    dpy_refresh(ds);
 
-    QLIST_FOREACH(dcl, &ds->listeners, next) {
-        if (dcl->gui_timer_interval &&
-            dcl->gui_timer_interval < interval)
-            interval = dcl->gui_timer_interval;
+    if (ds != NULL && !QLIST_EMPTY(&ds->listeners)) {
+        dpy_refresh(ds);
+        QLIST_FOREACH(dcl, &ds->listeners, next) {
+            if (dcl->gui_timer_interval &&
+                dcl->gui_timer_interval < interval)
+                interval = dcl->gui_timer_interval;
+        }
     }
-    qemu_mod_timer(ds->gui_timer, interval + qemu_get_clock(rt_clock));
-}
-
-static void nographic_update(void *opaque)
-{
-    uint64_t interval = GUI_REFRESH_INTERVAL;
 
-    qemu_flush_coalesced_mmio_buffer();
-    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
+    qemu_mod_timer(gui_timer, interval + qemu_get_clock(rt_clock));
 }
 
 struct vm_change_state_entry {
@@ -2577,7 +2572,6 @@ int main(int argc, char **argv, char **envp)
     const char *kernel_filename, *kernel_cmdline;
     char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
     DisplayState *ds;
-    DisplayChangeListener *dcl;
     int cyls, heads, secs, translation;
     QemuOpts *hda_opts = NULL, *opts;
     int optind;
@@ -3817,17 +3811,8 @@ int main(int argc, char **argv, char **envp)
     }
     dpy_resize(ds);
 
-    QLIST_FOREACH(dcl, &ds->listeners, next) {
-        if (dcl->dpy_refresh != NULL) {
-            ds->gui_timer = qemu_new_timer(rt_clock, gui_update, ds);
-            qemu_mod_timer(ds->gui_timer, qemu_get_clock(rt_clock));
-        }
-    }
-
-    if (display_type == DT_NOGRAPHIC || display_type == DT_VNC) {
-        nographic_timer = qemu_new_timer(rt_clock, nographic_update, NULL);
-        qemu_mod_timer(nographic_timer, qemu_get_clock(rt_clock));
-    }
+    gui_timer = qemu_new_timer(rt_clock, gui_update, ds);
+    qemu_mod_timer(gui_timer, qemu_get_clock(rt_clock));
 
     text_consoles_set_display(ds);
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] Fix and simplify gui timer logic.

[Gerd Hoffmann]

On 06/04/10 15:44, Gerd Hoffmann wrote:
> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
> timer enabled unconditionally.  We need a timer running anyway for mmio
> flush, so the whole have-gui-timer-only-when-needed logic is pretty
> pointless.  It also simplifies displaylisteners coming and going at
> runtime, we don't need to care about the timer then as it runs anyway.

Oops, scratch that.  Doesn't apply cleanly on master due to dependencies 
on other local patches.

/me goes preparing a patch series ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Paul Brook]

> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
> timer enabled unconditionally.  We need a timer running anyway for mmio
> flush, so the whole have-gui-timer-only-when-needed logic is pretty
> pointless.  It also simplifies displaylisteners coming and going at
> runtime, we don't need to care about the timer then as it runs anyway.

Linking mmio flushes to the gui is completely bogus.  The fact that we're 
doing arbitrary periodic mmio flushes suggests something else is horribly 
broken.

Paul

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Gerd Hoffmann]

On 06/07/10 18:12, Paul Brook wrote:
>> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
>> timer enabled unconditionally.  We need a timer running anyway for mmio
>> flush, so the whole have-gui-timer-only-when-needed logic is pretty
>> pointless.  It also simplifies displaylisteners coming and going at
>> runtime, we don't need to care about the timer then as it runs anyway.
>
> Linking mmio flushes to the gui is completely bogus.  The fact that we're
> doing arbitrary periodic mmio flushes suggests something else is horribly
> broken.

Was added by commit

http://git.qemu.org/qemu.git/commit/?id=62a2744ca09a0b44b8406ea0c430c4c67a2c3231

Patch description makes sense to me.  Of course we could have a separate 
timer for the mmio flushes instead of re-using the gui timer.  But we 
would have more wakeups then ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Paul Brook]

> >> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
> >> timer enabled unconditionally.  We need a timer running anyway for mmio
> >> flush, so the whole have-gui-timer-only-when-needed logic is pretty
> >> pointless.  It also simplifies displaylisteners coming and going at
> >> runtime, we don't need to care about the timer then as it runs anyway.
> > 
> > Linking mmio flushes to the gui is completely bogus.  The fact that we're
> > doing arbitrary periodic mmio flushes suggests something else is horribly
> > broken.
> 
> Was added by commit
> 
> http://git.qemu.org/qemu.git/commit/?id=62a2744ca09a0b44b8406ea0c430c4c67a2
> c3231
> 
> Patch description makes sense to me.  Of course we could have a separate
> timer for the mmio flushes instead of re-using the gui timer.  But we
> would have more wakeups then ...

This suggests we are incorrectly coalescing writes, and we should actually be 
notifying qemu when (at least) he first write occurs. If we aren't outputting 
anything we don't want to be waking up periodically just to flush an empty 
MMIO buffer.

Paul

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Gerd Hoffmann]

On 06/08/10 13:50, Paul Brook wrote:
>>>> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
>>>> timer enabled unconditionally.  We need a timer running anyway for mmio
>>>> flush, so the whole have-gui-timer-only-when-needed logic is pretty
>>>> pointless.  It also simplifies displaylisteners coming and going at
>>>> runtime, we don't need to care about the timer then as it runs anyway.
>>>
>>> Linking mmio flushes to the gui is completely bogus.  The fact that we're
>>> doing arbitrary periodic mmio flushes suggests something else is horribly
>>> broken.
>>
>> Was added by commit
>>
>> http://git.qemu.org/qemu.git/commit/?id=62a2744ca09a0b44b8406ea0c430c4c67a2
>> c3231
>>
>> Patch description makes sense to me.  Of course we could have a separate
>> timer for the mmio flushes instead of re-using the gui timer.  But we
>> would have more wakeups then ...
>
> This suggests we are incorrectly coalescing writes, and we should actually be
> notifying qemu when (at least) he first write occurs. If we aren't outputting
> anything we don't want to be waking up periodically just to flush an empty
> MMIO buffer.

That is completely unrelated to this patch though.  The patch doesn't 
change mmio flush behaviour at all.  And the periodic wakeup was there 
even before the mmio flush patch was added.  Even without gui, although 
I can't see a obvious reason for it ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Anthony Liguori]

On 06/08/2010 08:18 AM, Gerd Hoffmann wrote:
> On 06/08/10 13:50, Paul Brook wrote:
>>>>> Kill nographic timer.  Have a global gui_timer instead.  Have the gui
>>>>> timer enabled unconditionally.  We need a timer running anyway for 
>>>>> mmio
>>>>> flush, so the whole have-gui-timer-only-when-needed logic is pretty
>>>>> pointless.  It also simplifies displaylisteners coming and going at
>>>>> runtime, we don't need to care about the timer then as it runs 
>>>>> anyway.
>>>>
>>>> Linking mmio flushes to the gui is completely bogus.  The fact that 
>>>> we're
>>>> doing arbitrary periodic mmio flushes suggests something else is 
>>>> horribly
>>>> broken.
>>>
>>> Was added by commit
>>>
>>> http://git.qemu.org/qemu.git/commit/?id=62a2744ca09a0b44b8406ea0c430c4c67a2 
>>>
>>> c3231
>>>
>>> Patch description makes sense to me.  Of course we could have a 
>>> separate
>>> timer for the mmio flushes instead of re-using the gui timer.  But we
>>> would have more wakeups then ...
>>
>> This suggests we are incorrectly coalescing writes, and we should 
>> actually be
>> notifying qemu when (at least) he first write occurs. If we aren't 
>> outputting
>> anything we don't want to be waking up periodically just to flush an 
>> empty
>> MMIO buffer.
>
> That is completely unrelated to this patch though.  The patch doesn't 
> change mmio flush behaviour at all.  And the periodic wakeup was there 
> even before the mmio flush patch was added.  Even without gui, 
> although I can't see a obvious reason for it ...

Agreed.  Regardless of the periodic mmio flush, having a separate 
nographic timer isn't terribly helpful IMHO.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>

Re: [Qemu-devel] [PATCH] Fix and simplify gui timer logic.

[Paul Brook]

> >> This suggests we are incorrectly coalescing writes, and we should
> >> actually be
> >> notifying qemu when (at least) he first write occurs. If we aren't
> >> outputting
> >> anything we don't want to be waking up periodically just to flush an
> >> empty
> >> MMIO buffer.
> > 
> > That is completely unrelated to this patch though.  The patch doesn't
> > change mmio flush behaviour at all.  And the periodic wakeup was there
> > even before the mmio flush patch was added.  Even without gui,
> > although I can't see a obvious reason for it ...
> 
> Agreed.  Regardless of the periodic mmio flush, having a separate
> nographic timer isn't terribly helpful IMHO.

I guess what I was objecting to is formalising this a "gui_timer" rather than 
"arbitrary_polling_hack_timer".

Paul