[patch 0/2] SIGIO handling changes

[patch 0/2] SIGIO handling changes

[Marcelo Tosatti]

First patch from Anders Melchiorsen cleans up SIGIO handling:

- SIGALRM for alarm timers
- enable SIGIO on qemu_set_fd_handler2()
- avoid tap from abusing enable_sigio_timer()

With that in place its possible to increase the dumb console (-nographic)
refresh rate to 1s (from 30ms).


-- 


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

[patch 1/2] QEMU: use SIGARLM for alarm timers, enable SIGIO on qemu_set_fd_handler2()

[Marcelo Tosatti]

[-- Attachment #1: sigalrm-sigio --]
[-- Type: text/plain, Size: 3724 bytes --]

From: Anders Melchiorsen <mail@flac.kalibalik.dk>

Without I/O signals, qemu is relying on periodic timer events to poll
the I/O. That seems wrong, even though it works reasonably well
because timers are so frequent. In KVM, timers are less frequent, and
it does not work quite as well.

Here is a quick try at a more elaborate patch.

It attaches a signal to all[1] file descriptors that will be used in
select(). Also, it uses a dedicated SIGIO handler rather than
piggybacking on the alarm handler, so alarm I/O is changed to use
SIGALRM.

I copied the handler function from the alarm case, quite frankly I do
not quite understand what is going on. Also, I left _WIN32 out, since
I have no idea how signals work there.

[1] The slirp file descriptors are not included yet.


Index: kvm-userspace.io/qemu/vl.c
===================================================================
--- kvm-userspace.io.orig/qemu/vl.c
+++ kvm-userspace.io/qemu/vl.c
@@ -1177,6 +1177,25 @@ static int timer_load(QEMUFile *f, void 
     return 0;
 }
 
+#ifndef _WIN32
+static void host_io_handler(int host_signum)
+{
+    CPUState *env = next_cpu;
+
+    if (env) {
+        /* stop the currently executing cpu because io occured */
+        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
+#ifdef USE_KQEMU
+        if (env->kqemu_enabled) {
+            kqemu_cpu_interrupt(env);
+        }
+#endif
+    }
+
+    event_pending = 1;
+}
+#endif
+
 #ifdef _WIN32
 void CALLBACK host_alarm_handler(UINT uTimerID, UINT uMsg,
                                  DWORD_PTR dwUser, DWORD_PTR dw1, DWORD_PTR dw2)
@@ -1270,7 +1289,20 @@ static uint64_t qemu_next_deadline(void)
 
 #define RTC_FREQ 1024
 
-static void enable_sigio_timer(int fd)
+static void enable_sigio(int fd)
+{
+    struct sigaction act;
+
+    sigfillset(&act.sa_mask);
+    act.sa_flags = 0;
+    act.sa_handler = host_io_handler;
+
+    sigaction(SIGIO, &act, NULL);
+    fcntl(fd, F_SETFL, O_ASYNC);
+    fcntl(fd, F_SETOWN, getpid());
+}
+
+static void enable_sigalrm(int fd)
 {
     struct sigaction act;
 
@@ -1279,8 +1311,9 @@ static void enable_sigio_timer(int fd)
     act.sa_flags = 0;
     act.sa_handler = host_alarm_handler;
 
-    sigaction(SIGIO, &act, NULL);
+    sigaction(SIGALRM, &act, NULL);
     fcntl(fd, F_SETFL, O_ASYNC);
+    fcntl(fd, F_SETSIG, SIGALRM);
     fcntl(fd, F_SETOWN, getpid());
 }
 
@@ -1317,7 +1350,7 @@ static int hpet_start_timer(struct qemu_
     if (r < 0)
         goto fail;
 
-    enable_sigio_timer(fd);
+    enable_sigalrm(fd);
     t->priv = (void *)(long)fd;
 
     return 0;
@@ -1355,7 +1388,7 @@ static int rtc_start_timer(struct qemu_a
         return -1;
     }
 
-    enable_sigio_timer(rtc_fd);
+    enable_sigalrm(rtc_fd);
 
     t->priv = (void *)(long)rtc_fd;
 
@@ -4029,7 +4062,6 @@ static TAPState *net_tap_fd_init(VLANSta
         return NULL;
     s->fd = fd;
     s->no_poll = 0;
-    enable_sigio_timer(fd);
     s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
     qemu_set_fd_handler2(s->fd, tap_read_poll, tap_send, NULL, s);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd);
@@ -5661,6 +5693,10 @@ int qemu_set_fd_handler2(int fd,
             return -1;
         ioh->next = first_io_handler;
         first_io_handler = ioh;
+#ifndef _WIN32
+        enable_sigio(fd);
+#endif
+
     found:
         ioh->fd = fd;
         ioh->fd_read_poll = fd_read_poll;

-- 


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 1/2] QEMU: use SIGARLM for alarm timers, enable SIGIO on qemu_set_fd_handler2()

[Anthony Liguori]

With the IO thread, shouldn't we be striving to perform the select()s 
within the IO thread itself to completely avoid the need to use SIGIO at 
all?

Regards,

Anthony Liguori

Marcelo Tosatti wrote:
> From: Anders Melchiorsen <mail@flac.kalibalik.dk>
>
> Without I/O signals, qemu is relying on periodic timer events to poll
> the I/O. That seems wrong, even though it works reasonably well
> because timers are so frequent. In KVM, timers are less frequent, and
> it does not work quite as well.
>
> Here is a quick try at a more elaborate patch.
>
> It attaches a signal to all[1] file descriptors that will be used in
> select(). Also, it uses a dedicated SIGIO handler rather than
> piggybacking on the alarm handler, so alarm I/O is changed to use
> SIGALRM.
>
> I copied the handler function from the alarm case, quite frankly I do
> not quite understand what is going on. Also, I left _WIN32 out, since
> I have no idea how signals work there.
>
> [1] The slirp file descriptors are not included yet.
>
>
> Index: kvm-userspace.io/qemu/vl.c
> ===================================================================
> --- kvm-userspace.io.orig/qemu/vl.c
> +++ kvm-userspace.io/qemu/vl.c
> @@ -1177,6 +1177,25 @@ static int timer_load(QEMUFile *f, void 
>      return 0;
>  }
>
> +#ifndef _WIN32
> +static void host_io_handler(int host_signum)
> +{
> +    CPUState *env = next_cpu;
> +
> +    if (env) {
> +        /* stop the currently executing cpu because io occured */
> +        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
> +#ifdef USE_KQEMU
> +        if (env->kqemu_enabled) {
> +            kqemu_cpu_interrupt(env);
> +        }
> +#endif
> +    }
> +
> +    event_pending = 1;
> +}
> +#endif
> +
>  #ifdef _WIN32
>  void CALLBACK host_alarm_handler(UINT uTimerID, UINT uMsg,
>                                   DWORD_PTR dwUser, DWORD_PTR dw1, DWORD_PTR dw2)
> @@ -1270,7 +1289,20 @@ static uint64_t qemu_next_deadline(void)
>
>  #define RTC_FREQ 1024
>
> -static void enable_sigio_timer(int fd)
> +static void enable_sigio(int fd)
> +{
> +    struct sigaction act;
> +
> +    sigfillset(&act.sa_mask);
> +    act.sa_flags = 0;
> +    act.sa_handler = host_io_handler;
> +
> +    sigaction(SIGIO, &act, NULL);
> +    fcntl(fd, F_SETFL, O_ASYNC);
> +    fcntl(fd, F_SETOWN, getpid());
> +}
> +
> +static void enable_sigalrm(int fd)
>  {
>      struct sigaction act;
>
> @@ -1279,8 +1311,9 @@ static void enable_sigio_timer(int fd)
>      act.sa_flags = 0;
>      act.sa_handler = host_alarm_handler;
>
> -    sigaction(SIGIO, &act, NULL);
> +    sigaction(SIGALRM, &act, NULL);
>      fcntl(fd, F_SETFL, O_ASYNC);
> +    fcntl(fd, F_SETSIG, SIGALRM);
>      fcntl(fd, F_SETOWN, getpid());
>  }
>
> @@ -1317,7 +1350,7 @@ static int hpet_start_timer(struct qemu_
>      if (r < 0)
>          goto fail;
>
> -    enable_sigio_timer(fd);
> +    enable_sigalrm(fd);
>      t->priv = (void *)(long)fd;
>
>      return 0;
> @@ -1355,7 +1388,7 @@ static int rtc_start_timer(struct qemu_a
>          return -1;
>      }
>
> -    enable_sigio_timer(rtc_fd);
> +    enable_sigalrm(rtc_fd);
>
>      t->priv = (void *)(long)rtc_fd;
>
> @@ -4029,7 +4062,6 @@ static TAPState *net_tap_fd_init(VLANSta
>          return NULL;
>      s->fd = fd;
>      s->no_poll = 0;
> -    enable_sigio_timer(fd);
>      s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
>      qemu_set_fd_handler2(s->fd, tap_read_poll, tap_send, NULL, s);
>      snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd);
> @@ -5661,6 +5693,10 @@ int qemu_set_fd_handler2(int fd,
>              return -1;
>          ioh->next = first_io_handler;
>          first_io_handler = ioh;
> +#ifndef _WIN32
> +        enable_sigio(fd);
> +#endif
> +
>      found:
>          ioh->fd = fd;
>          ioh->fd_read_poll = fd_read_poll;
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 1/2] QEMU: use SIGARLM for alarm timers, enable SIGIO on qemu_set_fd_handler2()

[Marcelo Tosatti]

On Fri, Apr 11, 2008 at 01:59:35PM -0500, Anthony Liguori wrote:
> With the IO thread, shouldn't we be striving to perform the select()s 
> within the IO thread itself to completely avoid the need to use SIGIO at 
> all?

Fully agree. Problem with it are the fundamental changes in qemu that
are required (and the difficulty merging those in qemu).

This is an immediate and easy fix to reduce CPU consumption of
-nographic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 1/2] QEMU: use SIGARLM for alarm timers, enable SIGIO on qemu_set_fd_handler2()

[Avi Kivity]

Marcelo Tosatti wrote:
> On Fri, Apr 11, 2008 at 01:59:35PM -0500, Anthony Liguori wrote:
>   
>> With the IO thread, shouldn't we be striving to perform the select()s 
>> within the IO thread itself to completely avoid the need to use SIGIO at 
>> all?
>>     
>
> Fully agree. Problem with it are the fundamental changes in qemu that
> are required (and the difficulty merging those in qemu).
>
>   

One is tempted to use pselect() to temporarily unblock the signals while 
waiting.  This has two problems, though:  one, pselect() is emulated in 
libc with older kernels, and this emulation has an (unavoidable) race.  
Two, I think pselect() will deliver the signal in addition to returning, 
which we want to avoid.  Not sure about this though.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 1/2] QEMU: use SIGARLM for alarm timers, enable SIGIO on qemu_set_fd_handler2()

[Marcelo Tosatti]

On Fri, Apr 11, 2008 at 01:59:35PM -0500, Anthony Liguori wrote:
> >-static void enable_sigio_timer(int fd)
> >+static void enable_sigio(int fd)
> >+{
> >+    struct sigaction act;
> >+
> >+    sigfillset(&act.sa_mask);
> >+    act.sa_flags = 0;
> >+    act.sa_handler = host_io_handler;
> >+
> >+    sigaction(SIGIO, &act, NULL);
> >+    fcntl(fd, F_SETFL, O_ASYNC);

This should be O_ASYNC|O_NONBLOCK, if there is interesting
in taking the patch.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

[patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Marcelo Tosatti]

[-- Attachment #1: disable-console-timer --]
[-- Type: text/plain, Size: 918 bytes --]

With SIGIO enabled on stdio, there's no need to wakeup the thread
performing IO every 30ms.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm-userspace.io/qemu/vl.c
===================================================================
--- kvm-userspace.io.orig/qemu/vl.c
+++ kvm-userspace.io/qemu/vl.c
@@ -5640,6 +5640,7 @@ static void dumb_display_init(DisplaySta
     ds->dpy_update = dumb_update;
     ds->dpy_resize = dumb_resize;
     ds->dpy_refresh = dumb_refresh;
+    ds->gui_timer_interval = 1000;
 }
 
 /***********************************************************/

-- 


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anders]

Marcelo Tosatti wrote:

> With SIGIO enabled on stdio, there's no need to wakeup the thread
> performing IO every 30ms.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: kvm-userspace.io/qemu/vl.c
> ===================================================================
> --- kvm-userspace.io.orig/qemu/vl.c
> +++ kvm-userspace.io/qemu/vl.c
> @@ -5640,6 +5640,7 @@ static void dumb_display_init(DisplaySta
>      ds->dpy_update = dumb_update;
>      ds->dpy_resize = dumb_resize;
>      ds->dpy_refresh = dumb_refresh;
> +    ds->gui_timer_interval = 1000;
>  }
>  
>  /***********************************************************/
> 

Why even the 1000ms timer? I was proposing to just set ds->dpy_refresh 
to NULL for the dumb_display, but hit the qemu-devel dead-end.

http://lists.gnu.org/archive/html/qemu-devel/2008-01/msg00374.html

Do you see a problem with that approach? If yes, then that problem is 
probably currently present in the unconnected VNC case, as that one now 
disables the periodic timer completely.

Cheers,
Anders.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Marcelo Tosatti]

On Sun, Apr 13, 2008 at 06:30:34PM +0200, Anders wrote:
> Marcelo Tosatti wrote:
> 
> >With SIGIO enabled on stdio, there's no need to wakeup the thread
> >performing IO every 30ms.
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >Index: kvm-userspace.io/qemu/vl.c
> >===================================================================
> >--- kvm-userspace.io.orig/qemu/vl.c
> >+++ kvm-userspace.io/qemu/vl.c
> >@@ -5640,6 +5640,7 @@ static void dumb_display_init(DisplaySta
> >     ds->dpy_update = dumb_update;
> >     ds->dpy_resize = dumb_resize;
> >     ds->dpy_refresh = dumb_refresh;
> >+    ds->gui_timer_interval = 1000;
> > }
> > 
> > /***********************************************************/
> >
> 
> Why even the 1000ms timer? I was proposing to just set ds->dpy_refresh 
> to NULL for the dumb_display, but hit the qemu-devel dead-end.
> 
> http://lists.gnu.org/archive/html/qemu-devel/2008-01/msg00374.html
> 
> Do you see a problem with that approach? 

Issue is that the dumb console timer "wakes up" the vcpu to do IO
processing in main_loop_wait().

So while you're right that vga_hw_update() is a no-op for the -nographic
case, the indirect effect of the timer triggering main_loop_wait() is
needed for reading input from stdio in a way that feels interactive for
the user.

Which is of course not what the code appears to achieve. Apparently it
works by luck :)

I believe that qemu also wants a separate io thread doing select() with a
timeout rather than relying on signals.

> If yes, then that problem is 
> probably currently present in the unconnected VNC case, as that one now 
> disables the periodic timer completely.

Note that setting gui_timer_interval to 0 does not disable the timer:

/* in ms */
#define GUI_REFRESH_INTERVAL 30

static void gui_update(void *opaque)
{
    DisplayState *ds = opaque;
    ds->dpy_refresh(ds);
    qemu_mod_timer(ds->gui_timer,
        (ds->gui_timer_interval ?
            ds->gui_timer_interval :
            GUI_REFRESH_INTERVAL)
        + qemu_get_clock(rt_clock));
}


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Apr 13, 2008 at 06:30:34PM +0200, Anders wrote:
>   
>> Marcelo Tosatti wrote:
>>
>>     
>>> With SIGIO enabled on stdio, there's no need to wakeup the thread
>>> performing IO every 30ms.
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>> Index: kvm-userspace.io/qemu/vl.c
>>> ===================================================================
>>> --- kvm-userspace.io.orig/qemu/vl.c
>>> +++ kvm-userspace.io/qemu/vl.c
>>> @@ -5640,6 +5640,7 @@ static void dumb_display_init(DisplaySta
>>>     ds->dpy_update = dumb_update;
>>>     ds->dpy_resize = dumb_resize;
>>>     ds->dpy_refresh = dumb_refresh;
>>> +    ds->gui_timer_interval = 1000;
>>> }
>>>
>>> /***********************************************************/
>>>
>>>       
>> Why even the 1000ms timer? I was proposing to just set ds->dpy_refresh 
>> to NULL for the dumb_display, but hit the qemu-devel dead-end.
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2008-01/msg00374.html
>>
>> Do you see a problem with that approach? 
>>     
>
> Issue is that the dumb console timer "wakes up" the vcpu to do IO
> processing in main_loop_wait().
>
> So while you're right that vga_hw_update() is a no-op for the -nographic
> case, the indirect effect of the timer triggering main_loop_wait() is
> needed for reading input from stdio in a way that feels interactive for
> the user.
>
>   

Why not enable SIGIO on stdio input, like the rest of the fd handling in 
qemu?

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Marcelo Tosatti]

On Mon, Apr 14, 2008 at 07:24:06PM +0300, Avi Kivity wrote:
> >Issue is that the dumb console timer "wakes up" the vcpu to do IO
> >processing in main_loop_wait().
> >
> >So while you're right that vga_hw_update() is a no-op for the -nographic
> >case, the indirect effect of the timer triggering main_loop_wait() is
> >needed for reading input from stdio in a way that feels interactive for
> >the user.
> >
> >  
> 
> Why not enable SIGIO on stdio input, like the rest of the fd handling in 
> qemu?

Thats a possibility, but I think we've now agreed that doing select() with a
timeout is cleaner and possibly half a cent faster.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Marcelo Tosatti wrote:
> On Mon, Apr 14, 2008 at 07:24:06PM +0300, Avi Kivity wrote:
>   
>>> Issue is that the dumb console timer "wakes up" the vcpu to do IO
>>> processing in main_loop_wait().
>>>
>>> So while you're right that vga_hw_update() is a no-op for the -nographic
>>> case, the indirect effect of the timer triggering main_loop_wait() is
>>> needed for reading input from stdio in a way that feels interactive for
>>> the user.
>>>
>>>  
>>>       
>> Why not enable SIGIO on stdio input, like the rest of the fd handling in 
>> qemu?
>>     
>
> Thats a possibility, but I think we've now agreed that doing select() with a
> timeout is cleaner and possibly half a cent faster.
>   

BTW, when we set O_ASYNC on the tap fd, we're eliminating O_NONBLOCK.  
This means that we have to poll loop select() when readv()'ing packets 
instead of just reading until hitting AGAIN.  This means at least an 
extra syscall per packet.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
>
> BTW, when we set O_ASYNC on the tap fd, we're eliminating O_NONBLOCK.  
> This means that we have to poll loop select() when readv()'ing packets 
> instead of just reading until hitting AGAIN.  This means at least an 
> extra syscall per packet.

I didn't know that O_ASYNC and O_NONBLOCK were mutually exclusive.  Can 
you point me at the relevant documentation?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Carsten Otte]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> BTW, when we set O_ASYNC on the tap fd, we're eliminating O_NONBLOCK.  
>> This means that we have to poll loop select() when readv()'ing packets 
>> instead of just reading until hitting AGAIN.  This means at least an 
>> extra syscall per packet.
> 
> I didn't know that O_ASYNC and O_NONBLOCK were mutually exclusive.  Can 
> you point me at the relevant documentation?
They should'nt be mutual exclusive. If they are, the tap driver 
requires fixing afaics. The relevant documentation is the man page 
open(2), and it doesn't state they are exclusive.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>
>> BTW, when we set O_ASYNC on the tap fd, we're eliminating 
>> O_NONBLOCK.  This means that we have to poll loop select() when 
>> readv()'ing packets instead of just reading until hitting AGAIN.  
>> This means at least an extra syscall per packet.
>
> I didn't know that O_ASYNC and O_NONBLOCK were mutually exclusive.  
> Can you point me at the relevant documentation?

I don't know that they are, but we're doing an:

fcntl(fd, F_SETFL, O_ASYNC);

F_SETFL is not additive so the previous O_NONBLOCK gets dropped.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>>
>>> BTW, when we set O_ASYNC on the tap fd, we're eliminating 
>>> O_NONBLOCK.  This means that we have to poll loop select() when 
>>> readv()'ing packets instead of just reading until hitting AGAIN.  
>>> This means at least an extra syscall per packet.
>>
>> I didn't know that O_ASYNC and O_NONBLOCK were mutually exclusive.  
>> Can you point me at the relevant documentation?
>
> I don't know that they are, but we're doing an:
>
> fcntl(fd, F_SETFL, O_ASYNC);
>
> F_SETFL is not additive so the previous O_NONBLOCK gets dropped.
>

Ah, I thought it's something fundamental I'm missing.  The above is just 
a bug.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Marcelo Tosatti]

On Tue, Apr 15, 2008 at 08:40:09AM -0500, Anthony Liguori wrote:
> Avi Kivity wrote:
> >Anthony Liguori wrote:
> >>
> >>BTW, when we set O_ASYNC on the tap fd, we're eliminating 
> >>O_NONBLOCK.  This means that we have to poll loop select() when 
> >>readv()'ing packets instead of just reading until hitting AGAIN.  
> >>This means at least an extra syscall per packet.

Yeah, I noticed that problem too.

> >
> >I didn't know that O_ASYNC and O_NONBLOCK were mutually exclusive.  
> >Can you point me at the relevant documentation?
> 
> I don't know that they are, but we're doing an:
> 
> fcntl(fd, F_SETFL, O_ASYNC);
> 
> F_SETFL is not additive so the previous O_NONBLOCK gets dropped.

Fortunately read() will only be issued for the tap fd when select()
returns with its fd set.

And when that happens there is always a packet available for reading...


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Marcelo Tosatti wrote:
> On Mon, Apr 14, 2008 at 07:24:06PM +0300, Avi Kivity wrote:
>   
>>> Issue is that the dumb console timer "wakes up" the vcpu to do IO
>>> processing in main_loop_wait().
>>>
>>> So while you're right that vga_hw_update() is a no-op for the -nographic
>>> case, the indirect effect of the timer triggering main_loop_wait() is
>>> needed for reading input from stdio in a way that feels interactive for
>>> the user.
>>>
>>>  
>>>       
>> Why not enable SIGIO on stdio input, like the rest of the fd handling in 
>> qemu?
>>     
>
> Thats a possibility, but I think we've now agreed that doing select() with a
> timeout is cleaner and possibly half a cent faster.
>
>   

Yes, just wanted to make sure I don't miss something.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anders]

Marcelo Tosatti wrote:
> On Mon, Apr 14, 2008 at 07:24:06PM +0300, Avi Kivity wrote:

>> Why not enable SIGIO on stdio input, like the rest of the fd handling in
>> qemu?
>
> Thats a possibility, but I think we've now agreed that doing select() with
> a timeout is cleaner and possibly half a cent faster.

Since I can only follow this list as a hobby, I managed to miss that
discussion. Can somebody point me to the relevant thread, as I would find
it interesting?


Thanks,
Anders.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anders wrote:
>
>>> Why not enable SIGIO on stdio input, like the rest of the fd handling in
>>> qemu?
>>>       
>> Thats a possibility, but I think we've now agreed that doing select() with
>> a timeout is cleaner and possibly half a cent faster.
>>     
>
> Since I can only follow this list as a hobby, I managed to miss that
> discussion. Can somebody point me to the relevant thread, as I would find
> it interesting?
>
>   

This was off-list.  The point is, that with the iothread we don't need 
to rely on signals at all (qemu needs signals to break out of the 
emulation loop, kvm without iothread needs them to exit guest mode, but 
the iothread can simply sit in select() waiting for an fd to become 
active (or for aio to complete via a signal).

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Avi Kivity wrote:
> Anders wrote:
>   
>>>> Why not enable SIGIO on stdio input, like the rest of the fd handling in
>>>> qemu?
>>>>       
>>>>         
>>> Thats a possibility, but I think we've now agreed that doing select() with
>>> a timeout is cleaner and possibly half a cent faster.
>>>     
>>>       
>> Since I can only follow this list as a hobby, I managed to miss that
>> discussion. Can somebody point me to the relevant thread, as I would find
>> it interesting?
>>
>>   
>>     
>
> This was off-list.  The point is, that with the iothread we don't need 
> to rely on signals at all (qemu needs signals to break out of the 
> emulation loop, kvm without iothread needs them to exit guest mode, but 
> the iothread can simply sit in select() waiting for an fd to become 
> active (or for aio to complete via a signal).
>   

Why did we ever need sigtimedwait() anyway?  Even if we were select()ing 
within the VCPU context, we should break out of the select() on signal 
delivery.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
>
> Why did we ever need sigtimedwait() anyway?  Even if we were 
> select()ing within the VCPU context, we should break out of the 
> select() on signal delivery.
>

select() is no good since if the signal is delivered after the select(), 
but before entry into guest mode, it is lost.  pselect() might work, but 
its is not supported on all hosts, and it (AFAICT) delivers the signals 
by calling their handlers, which is slow and unnecessary.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Marcelo Tosatti]

On Tue, Apr 15, 2008 at 05:45:28PM +0300, Avi Kivity wrote:
> Anthony Liguori wrote:
> >
> >Why did we ever need sigtimedwait() anyway?  Even if we were 
> >select()ing within the VCPU context, we should break out of the 
> >select() on signal delivery.
> >
> 
> select() is no good since if the signal is delivered after the select(), 
> but before entry into guest mode, it is lost.  pselect() might work, but 
> its is not supported on all hosts, and it (AFAICT) delivers the signals 
> by calling their handlers, which is slow and unnecessary.

Anthony tested a patch using signalfd:

http://people.redhat.com/~mtosatti/io-thread-select-timeout

Which is only available on newer hosts. I guess the signals will have to
stay for older hosts.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Marcelo Tosatti wrote:
> On Tue, Apr 15, 2008 at 05:45:28PM +0300, Avi Kivity wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> Why did we ever need sigtimedwait() anyway?  Even if we were 
>>> select()ing within the VCPU context, we should break out of the 
>>> select() on signal delivery.
>>>
>>>       
>> select() is no good since if the signal is delivered after the select(), 
>> but before entry into guest mode, it is lost.  pselect() might work, but 
>> its is not supported on all hosts, and it (AFAICT) delivers the signals 
>> by calling their handlers, which is slow and unnecessary.
>>     
>
> Anthony tested a patch using signalfd:
>
> http://people.redhat.com/~mtosatti/io-thread-select-timeout
>
> Which is only available on newer hosts. I guess the signals will have to
> stay for older hosts.
>   

With the IO thread, we don't have to worry about lost signals like we do 
in a VCPU thread so it's fine to just use select() and install signal 
handlers IIUC.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
>
> With the IO thread, we don't have to worry about lost signals like we 
> do in a VCPU thread so it's fine to just use select() and install 
> signal handlers IIUC.
>

What about aio completions?  The only race-free way to handle both posix 
aio completion and fd readiness is signals AFAIK.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>
>> With the IO thread, we don't have to worry about lost signals like we 
>> do in a VCPU thread so it's fine to just use select() and install 
>> signal handlers IIUC.
>>
>
> What about aio completions?  The only race-free way to handle both 
> posix aio completion and fd readiness is signals AFAIK.

We poll aio completion after the select don't we?  Worst case scenario 
we miss a signal and wait to poll after the next select event.  That's 
going to occur very often because of the timer.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
>>
>> What about aio completions?  The only race-free way to handle both 
>> posix aio completion and fd readiness is signals AFAIK.
>
> We poll aio completion after the select don't we?  Worst case scenario 
> we miss a signal and wait to poll after the next select event.  That's 
> going to occur very often because of the timer.

if select() doesn't enable signals (like you can do with pselect) you 
may sit for a long time in select() until the timer expires.

Consider a 100Hz Linux guest running 'ls -lR' out of a cold cache: 
instead of 1-2 ms disk latencies you'll see 10 ms latencies, killing 
performance by a factor of 5.

I see the following possible solutions:

1. Apply Anders' patch and keep I/O completions signal based.

2. Use signalfd() to convert aio completions to fd readiness, emulating 
signalfd() using a thread which does sigwait()+write() (to a pipe) on 
older hosts

3. Use a separate thread for aio completions

4. Use pselect(), live with the race on older hosts (it was introduced 
in 2.6.16, which we barely support anyway), live with the signal 
delivery inefficiency.

When I started writing this email I was in favor of (1), but now with 
the new signalfd emulation I'm leaning towards (2).  I still think (1) 
should be merged, preferably to qemu upstream.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anders]

Avi Kivity wrote:

> if select() doesn't enable signals (like you can do with pselect) you
> may sit for a long time in select() until the timer expires.

Hm. Does the guest timer affect host userspace in all configurations? The
original trigger for my SIGIO patch was that opening a VNC connection
would take up to a second, until the qemu RTC timer fired.


> Consider a 100Hz Linux guest running 'ls -lR' out of a cold cache:
> instead of 1-2 ms disk latencies you'll see 10 ms latencies, killing
> performance by a factor of 5.

I guess this works out even worse for a dyntick guest?


> I still think (1) should be merged, preferably to qemu upstream.

I will give it another try. They are not very receptive, though, and I am
not that confident in what the patch is actually doing :-). You guys are
helping me a lot in that regard, thanks.


Cheers,
Anders.



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anders wrote:
> Avi Kivity wrote:
>
>   
>> if select() doesn't enable signals (like you can do with pselect) you
>> may sit for a long time in select() until the timer expires.
>>     
>
> Hm. Does the guest timer affect host userspace in all configurations? The
> original trigger for my SIGIO patch was that opening a VNC connection
> would take up to a second, until the qemu RTC timer fired.
>
>   

Right, if the guest is using the lapic or pit timers, there is no 
periodic signal to userspace.

>   
>> Consider a 100Hz Linux guest running 'ls -lR' out of a cold cache:
>> instead of 1-2 ms disk latencies you'll see 10 ms latencies, killing
>> performance by a factor of 5.
>>     
>
> I guess this works out even worse for a dyntick guest?
>
>
>   

Yes.  We can't depend on guest activity for correctness.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>>
>>> What about aio completions?  The only race-free way to handle both 
>>> posix aio completion and fd readiness is signals AFAIK.
>>
>> We poll aio completion after the select don't we?  Worst case 
>> scenario we miss a signal and wait to poll after the next select 
>> event.  That's going to occur very often because of the timer.
>
> if select() doesn't enable signals (like you can do with pselect) you 
> may sit for a long time in select() until the timer expires.
>
> Consider a 100Hz Linux guest running 'ls -lR' out of a cold cache: 
> instead of 1-2 ms disk latencies you'll see 10 ms latencies, killing 
> performance by a factor of 5.
>
> I see the following possible solutions:
>
> 1. Apply Anders' patch and keep I/O completions signal based.
>
> 2. Use signalfd() to convert aio completions to fd readiness, 
> emulating signalfd() using a thread which does sigwait()+write() (to a 
> pipe) on older hosts
>
> 3. Use a separate thread for aio completions
>
> 4. Use pselect(), live with the race on older hosts (it was introduced 
> in 2.6.16, which we barely support anyway), live with the signal 
> delivery inefficiency.
>
> When I started writing this email I was in favor of (1), but now with 
> the new signalfd emulation I'm leaning towards (2).  I still think (1) 
> should be merged, preferably to qemu upstream.

There is a 5th option.  Do away with the use of posix aio.  We get 
absolutely no benefit from it because it's limited to a single thread.  
Fabrice has reverted a patch to change that in the past.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Carsten Otte]

Anthony Liguori wrote:
> There is a 5th option.  Do away with the use of posix aio.  We get 
> absolutely no benefit from it because it's limited to a single thread.  
> Fabrice has reverted a patch to change that in the past.
How about using linux aio for it? It seems much better, because it 
doesn't use userspace threads but has a direct in-kernel 
implementation. I've had good performance on zldisk with that, and 
it's stable.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Carsten Otte wrote:
> Anthony Liguori wrote:
>> There is a 5th option.  Do away with the use of posix aio.  We get 
>> absolutely no benefit from it because it's limited to a single 
>> thread.  Fabrice has reverted a patch to change that in the past.
> How about using linux aio for it? It seems much better, because it 
> doesn't use userspace threads but has a direct in-kernel 
> implementation. I've had good performance on zldisk with that, and 
> it's stable.

Linux-aio is nice except that you can't easily complete network and disk 
requests simulateneously (no IO_CMD_POLL yet).  This means you need an 
additional thread.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [patch 2/2] QEMU: decrease console "refresh rate" with -nographic

[Avi Kivity]

Anthony Liguori wrote:
> There is a 5th option.  Do away with the use of posix aio.  We get 
> absolutely no benefit from it because it's limited to a single thread.  
>   

Even one async request is much better than zero.

> Fabrice has reverted a patch to change that in the past.
>   

Perhaps he can be persuaded to unrevert it.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone