[Qemu-devel] [6391] Fix nographic mode and VNC

[Qemu-devel] [6391] Fix nographic mode and VNC

[Blue Swirl]

Revision: 6391
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6391
Author:   blueswir1
Date:     2009-01-21 19:28:13 +0000 (Wed, 21 Jan 2009)

Log Message:
-----------
Fix nographic mode and VNC

Modified Paths:
--------------
    trunk/vl.c

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2009-01-21 19:18:00 UTC (rev 6390)
+++ trunk/vl.c	2009-01-21 19:28:13 UTC (rev 6391)
@@ -263,6 +263,7 @@
 static int64_t qemu_icount_bias;
 static QEMUTimer *icount_rt_timer;
 static QEMUTimer *icount_vm_timer;
+static QEMUTimer *nographic_timer;
 
 uint8_t qemu_uuid[16];
 
@@ -3414,6 +3415,13 @@
     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_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
+}
+
 struct vm_change_state_entry {
     VMChangeStateHandler *cb;
     void *opaque;
@@ -5600,6 +5608,11 @@
         dcl = dcl->next;
     }
 
+    if (nographic || (vnc_display && !sdl)) {
+        nographic_timer = qemu_new_timer(rt_clock, nographic_update, NULL);
+        qemu_mod_timer(nographic_timer, qemu_get_clock(rt_clock));
+    }
+
     text_consoles_set_display(display_state);
 
     if (monitor_device && monitor_hd)

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Stefano Stabellini]

If you just want to always have a timer enabled maybe this patch is better.
I wouldn't want you to add another timer when the gui_timer is available
for this job :)


diff --git a/vl.c b/vl.c
index 63d954b..2e84dce 100644
--- a/vl.c
+++ b/vl.c
@@ -5553,14 +5553,8 @@ int main(int argc, char **argv, char **envp)
     }
     dpy_resize(ds);
 
-    dcl = ds->listeners;
-    while (dcl != NULL) {
-        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));
-        }
-        dcl = dcl->next;
-    }
+    ds->gui_timer = qemu_new_timer(rt_clock, gui_update, ds);
+    qemu_mod_timer(ds->gui_timer, qemu_get_clock(rt_clock));
 
     text_consoles_set_display(display_state);

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Blue Swirl]

On 1/21/09, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> If you just want to always have a timer enabled maybe this patch is better.
>  I wouldn't want you to add another timer when the gui_timer is available
>  for this job :)

I thought about that, but I didn't want to run gui_update when a dummy
function is enough and more accurate.

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Stefano Stabellini]

Blue Swirl wrote:

> On 1/21/09, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>> If you just want to always have a timer enabled maybe this patch is better.
>>  I wouldn't want you to add another timer when the gui_timer is available
>>  for this job :)
> 
> I thought about that, but I didn't want to run gui_update when a dummy
> function is enough and more accurate.
> 


In that case gui_update wouldn't do anything because none registered a
dpy_refresh function.

But I agree that a dummy function is more accurate.

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Samuel Thibault]

Blue Swirl, le Wed 21 Jan 2009 19:28:14 +0000, a écrit :
> +static void nographic_update(void *opaque)
> +{
> +    uint64_t interval = GUI_REFRESH_INTERVAL;
> +
> +    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
> +}

Maybe less often than 33 times per second?  SDL uses 2 times per second
when the window is minimized.

Samuel

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Blue Swirl]

On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Blue Swirl, le Wed 21 Jan 2009 19:28:14 +0000, a écrit :
>
> > +static void nographic_update(void *opaque)
>  > +{
>  > +    uint64_t interval = GUI_REFRESH_INTERVAL;
>  > +
>  > +    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
>  > +}
>
>
> Maybe less often than 33 times per second?  SDL uses 2 times per second
>  when the window is minimized.

I just tried: it's not enough, the serial console is unusably slow.

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Samuel Thibault]

Blue Swirl, le Wed 21 Jan 2009 22:16:15 +0200, a écrit :
> On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> > Blue Swirl, le Wed 21 Jan 2009 19:28:14 +0000, a écrit :
> >
> > > +static void nographic_update(void *opaque)
> >  > +{
> >  > +    uint64_t interval = GUI_REFRESH_INTERVAL;
> >  > +
> >  > +    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
> >  > +}
> >
> >
> > Maybe less often than 33 times per second?  SDL uses 2 times per second
> >  when the window is minimized.
> 
> I just tried: it's not enough, the serial console is unusably slow.

Mmm, I haven't followed everything so I probably misunderstand, but I
guess you are talking about a serial console on stdio, shouldn't we
already be monitoring the stdin fd to break the cpu emulation loop?
It'd be a shame to have to poll every 30ms all the time because of the
cpu emulation loop, even when we can just select in main_loop_wait
because the cpu is idle.

Samuel

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Blue Swirl]

On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Blue Swirl, le Wed 21 Jan 2009 22:16:15 +0200, a écrit :
>
> > On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>  > > Blue Swirl, le Wed 21 Jan 2009 19:28:14 +0000, a écrit :
>  > >
>  > > > +static void nographic_update(void *opaque)
>  > >  > +{
>  > >  > +    uint64_t interval = GUI_REFRESH_INTERVAL;
>  > >  > +
>  > >  > +    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
>  > >  > +}
>  > >
>  > >
>  > > Maybe less often than 33 times per second?  SDL uses 2 times per second
>  > >  when the window is minimized.
>  >
>  > I just tried: it's not enough, the serial console is unusably slow.
>
>
> Mmm, I haven't followed everything so I probably misunderstand, but I
>  guess you are talking about a serial console on stdio, shouldn't we
>  already be monitoring the stdin fd to break the cpu emulation loop?

Some other way, like SIGIO or IO worker thread, may work too. It may
also be a big change.

>  It'd be a shame to have to poll every 30ms all the time because of the
>  cpu emulation loop, even when we can just select in main_loop_wait
>  because the cpu is idle.

OpenBIOS reads the console in a busy loop, the CPU will never be idle.

Maybe the timer period could be scaled automatically: if for X periods
there has been no activity on the select() file descriptors, increase
the period by factor Y. Also if for X periods there has always been
activity, decrease the period.

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Anthony Liguori]

Blue Swirl wrote:
> On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>   
>> Blue Swirl, le Wed 21 Jan 2009 22:16:15 +0200, a écrit :
>>
>>     
>>> On 1/21/09, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>>>       
>>  > > Blue Swirl, le Wed 21 Jan 2009 19:28:14 +0000, a écrit :
>>  > >
>>  > > > +static void nographic_update(void *opaque)
>>  > >  > +{
>>  > >  > +    uint64_t interval = GUI_REFRESH_INTERVAL;
>>  > >  > +
>>  > >  > +    qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
>>  > >  > +}
>>  > >
>>  > >
>>  > > Maybe less often than 33 times per second?  SDL uses 2 times per second
>>  > >  when the window is minimized.
>>  >
>>  > I just tried: it's not enough, the serial console is unusably slow.
>>
>>
>> Mmm, I haven't followed everything so I probably misunderstand, but I
>>  guess you are talking about a serial console on stdio, shouldn't we
>>  already be monitoring the stdin fd to break the cpu emulation loop?
>>     
>
> Some other way, like SIGIO or IO worker thread, may work too. It may
> also be a big change.
>   

The solution is an IO thread.  I've been working on this on and off for 
KVM and now I think I see how to do it for TCG.

With an IO thread, select() could run while TCG ran and when select() 
returned, it would signal to TCG as if a signal handler was invoked.

Regards,

Anthony Liguori

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Paul Brook]

> >  > > > +static void nographic_update(void *opaque)
> >  > > >
> >  > >  > +{
> >  > >  > +    uint64_t interval = GUI_REFRESH_INTERVAL;
> >  > >  > +
> >  > >  > +    qemu_mod_timer(nographic_timer, interval +
> >  > >  > qemu_get_clock(rt_clock)); +}
> >  > >
> >  > > Maybe less often than 33 times per second?  SDL uses 2 times per
> >  > > second when the window is minimized.
> >  >
> >  > I just tried: it's not enough, the serial console is unusably slow.
> >
> > Mmm, I haven't followed everything so I probably misunderstand, but I
> >  guess you are talking about a serial console on stdio, shouldn't we
> >  already be monitoring the stdin fd to break the cpu emulation loop?
>
> Some other way, like SIGIO or IO worker thread, may work too. It may
> also be a big change.

Using a side effect of the the gui refresh timer to implement IO polling is 
absolutely the wrong way to fix things.

All IO should be event driven. If it isn't then then it needs fixing. The 
polling timeout is determined by timeout = 5000 in vl.c.

Paul

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Anthony Liguori]

Paul Brook wrote:
>> Some other way, like SIGIO or IO worker thread, may work too. It may
>> also be a big change.
>>     
>
> Using a side effect of the the gui refresh timer to implement IO polling is 
> absolutely the wrong way to fix things.
>
> All IO should be event driven. If it isn't then then it needs fixing. The 
> polling timeout is determined by timeout = 5000 in vl.c.
>   

No, this is a fundamentally broken aspect of TCG (and dyngen).

cpu_exec() spins in a loop checking for 'event_pending'.  This is set 
whenever the host_alarm_timer callback signals.  Alternatively, a halted 
guest will cause cpu_exec() to exit.  Only then do you do a select() on 
pending IO.

If you are using dynticks as your clock, the guest has a slow clock, and 
the guest is not using halt instructions, then you will not poll IO 
until the next guest timer tick b/c your guest is just going to spin.

There are three possible solutions.  Set SIGIO on every file descriptor 
so that TCG breaks whenever IO is pending.  Besides ugliness, this fails 
because not every type of file descriptor supports SIGIO.

A second solution is to use a polling select() in cpu_exec.  Since 
you're adding a system call (and a rather heavy one) in the fast path, 
this is going to likely hurt TCG performance.

The third possibility is to have the select() run in a separate thread 
from the TCG cpu_exec() loop.  cpu_exec() would do an atomic read of 
'event_pending' and the IO thread would do an atomic write of 
'event_pending' whenever select() returned a writable file descriptor.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Paul Brook]

> cpu_exec() spins in a loop checking for 'event_pending'.  This is set
> whenever the host_alarm_timer callback signals.  Alternatively, a halted
> guest will cause cpu_exec() to exit.  Only then do you do a select() on
> pending IO.

Ah, I see.

> If you are using dynticks as your clock, the guest has a slow clock, and
> the guest is not using halt instructions, then you will not poll IO
> until the next guest timer tick b/c your guest is just going to spin.
>
> There are three possible solutions.  Set SIGIO on every file descriptor
> so that TCG breaks whenever IO is pending.  Besides ugliness, this fails
> because not every type of file descriptor supports SIGIO.
>
> A second solution is to use a polling select() in cpu_exec.  Since
> you're adding a system call (and a rather heavy one) in the fast path,
> this is going to likely hurt TCG performance.

This won't work. If the guest really is in a tight loop then TB chaining means 
it will never exit translated code.

> The third possibility is to have the select() run in a separate thread
> from the TCG cpu_exec() loop.  cpu_exec() would do an atomic read of
> 'event_pending' and the IO thread would do an atomic write of
> 'event_pending' whenever select() returned a writable file descriptor.

This suffers from the same problem described above. You need to force the main 
execution thread to break out of the translated loop. In practice this 
probably means sending a signal to the main thread.  You could check 
event_pending at the start of every TB, but that's likely to incur a fairly 
big performance hit.

Paul

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Ian Jackson]

Paul Brook writes ("Re: [Qemu-devel] [6391] Fix nographic mode and VNC"):
> [Anthony:]
> > The third possibility is to have the select() run in a separate thread
> > from the TCG cpu_exec() loop.  cpu_exec() would do an atomic read of
> > 'event_pending' and the IO thread would do an atomic write of
> > 'event_pending' whenever select() returned a writable file descriptor.
>
> This suffers from the same problem described above. You need to
> force the main execution thread to break out of the translated
> loop. In practice this probably means sending a signal to the main
> thread.  You could check event_pending at the start of every TB, but
> that's likely to incur a fairly big performance hit.

What would the signal handler do ?  siglongjmp out of the translated
loop ?  Surely the timer implementation doesn't do that right now.

Ian.

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Anthony Liguori]

Paul Brook wrote:
>> cpu_exec() spins in a loop checking for 'event_pending'.  This is set
>> whenever the host_alarm_timer callback signals.  Alternatively, a halted
>> guest will cause cpu_exec() to exit.  Only then do you do a select() on
>> pending IO.
>>     
>
> Ah, I see.
>
>   
>> If you are using dynticks as your clock, the guest has a slow clock, and
>> the guest is not using halt instructions, then you will not poll IO
>> until the next guest timer tick b/c your guest is just going to spin.
>>
>> There are three possible solutions.  Set SIGIO on every file descriptor
>> so that TCG breaks whenever IO is pending.  Besides ugliness, this fails
>> because not every type of file descriptor supports SIGIO.
>>
>> A second solution is to use a polling select() in cpu_exec.  Since
>> you're adding a system call (and a rather heavy one) in the fast path,
>> this is going to likely hurt TCG performance.
>>     
>
> This won't work. If the guest really is in a tight loop then TB chaining means 
> it will never exit translated code.
>   

But then signal delivery wouldn't either, right?  That suggests that if 
the guest is in a tight loop right now, QEMU will freeze.

There's a fair bit of code that is safe to run along side of TCG.  If we 
separate the locking for the device model code from every thing (the 
monitor, vnc, sdl, etc.), then we could still let QEMU be responsive 
even in such a condition.

>> The third possibility is to have the select() run in a separate thread
>> from the TCG cpu_exec() loop.  cpu_exec() would do an atomic read of
>> 'event_pending' and the IO thread would do an atomic write of
>> 'event_pending' whenever select() returned a writable file descriptor.
>>     
>
> This suffers from the same problem described above. You need to force the main 
> execution thread to break out of the translated loop. In practice this 
> probably means sending a signal to the main thread.

How does a signal break translated loop execution other than by setting 
event_pending?  The actual signal isn't going to make a difference, it's 
just setting event_pending=1 that causes it to break out of the loop IIUC.

Regards,

Anthony Liguori

>   You could check 
> event_pending at the start of every TB, but that's likely to incur a fairly 
> big performance hit.
>
> Paul
>   

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Paul Brook]

> >> A second solution is to use a polling select() in cpu_exec.  Since
> >> you're adding a system call (and a rather heavy one) in the fast path,
> >> this is going to likely hurt TCG performance.
> >
> > This won't work. If the guest really is in a tight loop then TB chaining
> > means it will never exit translated code.
>
> But then signal delivery wouldn't either, right?  That suggests that if
> the guest is in a tight loop right now, QEMU will freeze.

No. The signal handler calls cpu_interrupt, which unlinks the TBs.

> There's a fair bit of code that is safe to run along side of TCG.  If we
> separate the locking for the device model code from every thing (the
> monitor, vnc, sdl, etc.), then we could still let QEMU be responsive
> even in such a condition.

Maybe. You risk having to put an SMP safe lock in the MMIO handler, which 
would probably do bad things to performance. Many of the embedded targets 
don't have DMA capable peripherals, so we want to avoid making MMIO too 
expensive. kvm is a bit different because MMIO is already horribly expensive.

Paul

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Paul Brook]

> > > The third possibility is to have the select() run in a separate thread
> > > from the TCG cpu_exec() loop.  cpu_exec() would do an atomic read of
> > > 'event_pending' and the IO thread would do an atomic write of
> > > 'event_pending' whenever select() returned a writable file descriptor.
> >
> > This suffers from the same problem described above. You need to
> > force the main execution thread to break out of the translated
> > loop. In practice this probably means sending a signal to the main
> > thread.  You could check event_pending at the start of every TB, but
> > that's likely to incur a fairly big performance hit.
>
> What would the signal handler do ?  siglongjmp out of the translated
> loop ?  Surely the timer implementation doesn't do that right now.

The same as the current one does. Call cpu_interrupt to unlink the TB and 
cause execution to fall back to cpu_exec sometime in the near future.
cpu_interrupt is not threadsafe[1] so has to be called from the main execution 
thread.

[1] TB [un-]chaining involves direct patching of the translated code. Most SMP 
systems do not have a coherent icache.

Paul

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Anthony Liguori]

Paul Brook wrote:
>>>> A second solution is to use a polling select() in cpu_exec.  Since
>>>> you're adding a system call (and a rather heavy one) in the fast path,
>>>> this is going to likely hurt TCG performance.
>>>>         
>>> This won't work. If the guest really is in a tight loop then TB chaining
>>> means it will never exit translated code.
>>>       
>> But then signal delivery wouldn't either, right?  That suggests that if
>> the guest is in a tight loop right now, QEMU will freeze.
>>     
>
> No. The signal handler calls cpu_interrupt, which unlinks the TBs.
>   

Which isn't thread safe.  Okay, then you also need to send a signal to 
the TCG thread.

Regards,

Anthony Liguori

>> There's a fair bit of code that is safe to run along side of TCG.  If we
>> separate the locking for the device model code from every thing (the
>> monitor, vnc, sdl, etc.), then we could still let QEMU be responsive
>> even in such a condition.
>>     
>
> Maybe. You risk having to put an SMP safe lock in the MMIO handler, which 
> would probably do bad things to performance. Many of the embedded targets 
> don't have DMA capable peripherals, so we want to avoid making MMIO too 
> expensive. kvm is a bit different because MMIO is already horribly expensive.
>
> Paul
>   

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Jamie Lokier]

Paul Brook wrote:
> Maybe. You risk having to put an SMP safe lock in the MMIO handler, which 
> would probably do bad things to performance.

Would it?  It's just one atomic operation in the non-contention case,
and MMIO writes could be queued the same way that KVM queues them,
without needing locks for each individual op.

-- Jamie

Re: [Qemu-devel] [6391] Fix nographic mode and VNC

[Paul Brook]

On Sunday 25 January 2009, Jamie Lokier wrote:
> Paul Brook wrote:
> > Maybe. You risk having to put an SMP safe lock in the MMIO handler, which
> > would probably do bad things to performance.
>
> Would it?  It's just one atomic operation in the non-contention case,
> and MMIO writes could be queued the same way that KVM queues them,
> without needing locks for each individual op.

Depends on the host. Even in the uncontended case grabbing a lock tends to 
require fairly strong memory barriers.

Like I said, kvm already exhibits fairly abysmal performance characteristics 
in this area. I'm not sure how much write coalescing actually helps in 
practice for anything other than VGA. It certainly doesn't help with reads.

Paul