[Spice-devel] Further thoughts on the write polling in the xf86_spice_xql driver

Submitted by Jeremy White on June 2, 2012, 6:24 p.m.

Details

Message ID 4FCA5A5D.1030400@codeweavers.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jeremy White June 2, 2012, 6:24 p.m.
I spent a fair amount of energy trying to understand the issue with EAGAIN
and the xspice server.  I've sent in one patch which I believe to be
correct.

I had some further thoughts I wanted to share as well.

To reprise, the issue occurs when we send the large ping test packet from the server; if
the network interface returns EAGAIN, we break out and return to the X server.  We
add the socket to our list of 'write watches', and hope to retry the write again
once we detect that the socket is ready for writing.

However, while the X server main loop has a facility that lets us add fds to it's
select read mask, it has no such facility to let us add fds to it's *write* select mask.

So, without patching X, we have no clean way to add our socket to the list to
be checked as being writable.  Given that, we have to fall back on polling.  However,
in our current state, we don't actually do the poll unless there is a readable
socket.  The patch I've already sent just makes us actually
perform the polling action.

I made an argument to myself that another potentially useful design change was to use
the facility the X server does provide - namely the timeout in the pre block call - to prevent
the X server from doing a long select on the read sockets.  Essentially, since
the X server doesn't select on the write fds, we should have it spin loop, while
we check the write fds.

A patch that implements that strategy is attached below.

However, as I played with this patch, I came to feel that, while logically correct,
it had little practical value, and the resulting spin loop was actually an annoying
side effect.  It turns out that the X server pretty much always has a timer of some
sort going, so in my testing, it was never hanging on the select for more than 33 us.

Given that, I thought to omit this patch, and perhaps instead send a patch
with a comment touching on this.

Have I missed anything?

Cheers,

Jeremy


---
 src/spiceqxl_main_loop.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index b06d5eb..3154384 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -241,12 +241,14 @@  static void channel_event(int event, SpiceChannelEventInfo *info)
     NOT_IMPLEMENTED
 }
 
-static int set_watch_fds(fd_set *rfds, fd_set *wfds)
+static int set_watch_fds(fd_set *rfds, fd_set *wfds, int *wcount)
 {
     SpiceWatch *watch;
     RingItem *link;
     RingItem *next;
     int max_fd = -1;
+    if (wcount)
+        *wcount = 0;
 
     RING_FOREACH_SAFE(link, next, &watches) {
         watch = (SpiceWatch*)link;
@@ -257,6 +259,8 @@  static int set_watch_fds(fd_set *rfds, fd_set *wfds)
         if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
             FD_SET(watch->fd, wfds);
             max_fd = watch->fd > max_fd ? watch->fd : max_fd;
+            if (wcount)
+                (*wcount)++;
         }
     }
     return max_fd;
@@ -267,10 +271,21 @@  static int set_watch_fds(fd_set *rfds, fd_set *wfds)
  * readmask is just an fdset on linux, but something totally different on windows (etc).
  * DIX has a comment about it using a special type to hide this (so we break that here)
  */
+static struct timeval write_timeout;
 static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
 {
+    int wcount;
     /* set all our fd's */
-    set_watch_fds((fd_set*)readmask, (fd_set*)readmask);
+    set_watch_fds((fd_set*)readmask, (fd_set*)readmask, &wcount);
+
+    /* If we have any write watches, we have to force the X server into a polling
+       select loop */
+    if (wcount > 0)
+    {
+        if (*timeout == NULL)
+            *timeout = &write_timeout;
+        (*timeout)->tv_sec = (*timeout)->tv_usec = 0;
+    }
 }
 
 /*
@@ -292,7 +307,7 @@  static void select_and_check_watches(void)
 
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
-    max_fd = set_watch_fds(&rfds, &wfds);
+    max_fd = set_watch_fds(&rfds, &wfds, NULL);
     watch = (SpiceWatch*)watches.next;
     timeout.tv_sec = timeout.tv_usec = 0;
     retval = select(max_fd + 1, &rfds, &wfds, NULL, &timeout);

Comments

On Sat, Jun 02, 2012 at 01:24:29PM -0500, Jeremy White wrote:
> I spent a fair amount of energy trying to understand the issue with EAGAIN
> and the xspice server.  I've sent in one patch which I believe to be
> correct.
> 
> I had some further thoughts I wanted to share as well.
> 
> To reprise, the issue occurs when we send the large ping test packet from the server; if
> the network interface returns EAGAIN, we break out and return to the X server.  We
> add the socket to our list of 'write watches', and hope to retry the write again
> once we detect that the socket is ready for writing.
> 
> However, while the X server main loop has a facility that lets us add fds to it's
> select read mask, it has no such facility to let us add fds to it's *write* select mask.

Right. This is the problem. I think the right solution is to patch the X
server.

> 
> So, without patching X, we have no clean way to add our socket to the list to
> be checked as being writable.  Given that, we have to fall back on polling.  However,
> in our current state, we don't actually do the poll unless there is a readable
> socket.  The patch I've already sent just makes us actually
> perform the polling action.
> 
> I made an argument to myself that another potentially useful design change was to use
> the facility the X server does provide - namely the timeout in the pre block call - to prevent
> the X server from doing a long select on the read sockets.  Essentially, since
> the X server doesn't select on the write fds, we should have it spin loop, while
> we check the write fds.
> 
> A patch that implements that strategy is attached below.
> 
> However, as I played with this patch, I came to feel that, while logically correct,
> it had little practical value, and the resulting spin loop was actually an annoying
> side effect.  It turns out that the X server pretty much always has a timer of some
> sort going, so in my testing, it was never hanging on the select for more than 33 us.
> 
> Given that, I thought to omit this patch, and perhaps instead send a patch
> with a comment touching on this.
> 
> Have I missed anything?

I don't think so. I think fixing BlockHandler in X to include a
pWritemask is the right way (and/or WakeupHandler, not sure).

> 
> Cheers,
> 
> Jeremy
> 
> 
> ---
>  src/spiceqxl_main_loop.c |   21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index b06d5eb..3154384 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
> @@ -241,12 +241,14 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
>      NOT_IMPLEMENTED
>  }
>  
> -static int set_watch_fds(fd_set *rfds, fd_set *wfds)
> +static int set_watch_fds(fd_set *rfds, fd_set *wfds, int *wcount)
>  {
>      SpiceWatch *watch;
>      RingItem *link;
>      RingItem *next;
>      int max_fd = -1;
> +    if (wcount)
> +        *wcount = 0;
>  
>      RING_FOREACH_SAFE(link, next, &watches) {
>          watch = (SpiceWatch*)link;
> @@ -257,6 +259,8 @@ static int set_watch_fds(fd_set *rfds, fd_set *wfds)
>          if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
>              FD_SET(watch->fd, wfds);
>              max_fd = watch->fd > max_fd ? watch->fd : max_fd;
> +            if (wcount)
> +                (*wcount)++;
>          }
>      }
>      return max_fd;
> @@ -267,10 +271,21 @@ static int set_watch_fds(fd_set *rfds, fd_set *wfds)
>   * readmask is just an fdset on linux, but something totally different on windows (etc).
>   * DIX has a comment about it using a special type to hide this (so we break that here)
>   */
> +static struct timeval write_timeout;
>  static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
>  {
> +    int wcount;
>      /* set all our fd's */
> -    set_watch_fds((fd_set*)readmask, (fd_set*)readmask);
> +    set_watch_fds((fd_set*)readmask, (fd_set*)readmask, &wcount);
> +
> +    /* If we have any write watches, we have to force the X server into a polling
> +       select loop */
> +    if (wcount > 0)
> +    {
> +        if (*timeout == NULL)
> +            *timeout = &write_timeout;
> +        (*timeout)->tv_sec = (*timeout)->tv_usec = 0;
> +    }
>  }
>  
>  /*
> @@ -292,7 +307,7 @@ static void select_and_check_watches(void)
>  
>      FD_ZERO(&rfds);
>      FD_ZERO(&wfds);
> -    max_fd = set_watch_fds(&rfds, &wfds);
> +    max_fd = set_watch_fds(&rfds, &wfds, NULL);
>      watch = (SpiceWatch*)watches.next;
>      timeout.tv_sec = timeout.tv_usec = 0;
>      retval = select(max_fd + 1, &rfds, &wfds, NULL, &timeout);
> -- 
> 1.7.9.5
> 

> ---
>  src/spiceqxl_main_loop.c |   21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index b06d5eb..3154384 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
> @@ -241,12 +241,14 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
>      NOT_IMPLEMENTED
>  }
>  
> -static int set_watch_fds(fd_set *rfds, fd_set *wfds)
> +static int set_watch_fds(fd_set *rfds, fd_set *wfds, int *wcount)
>  {
>      SpiceWatch *watch;
>      RingItem *link;
>      RingItem *next;
>      int max_fd = -1;
> +    if (wcount)
> +        *wcount = 0;
>  
>      RING_FOREACH_SAFE(link, next, &watches) {
>          watch = (SpiceWatch*)link;
> @@ -257,6 +259,8 @@ static int set_watch_fds(fd_set *rfds, fd_set *wfds)
>          if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
>              FD_SET(watch->fd, wfds);
>              max_fd = watch->fd > max_fd ? watch->fd : max_fd;
> +            if (wcount)
> +                (*wcount)++;
>          }
>      }
>      return max_fd;
> @@ -267,10 +271,21 @@ static int set_watch_fds(fd_set *rfds, fd_set *wfds)
>   * readmask is just an fdset on linux, but something totally different on windows (etc).
>   * DIX has a comment about it using a special type to hide this (so we break that here)
>   */
> +static struct timeval write_timeout;
>  static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
>  {
> +    int wcount;
>      /* set all our fd's */
> -    set_watch_fds((fd_set*)readmask, (fd_set*)readmask);
> +    set_watch_fds((fd_set*)readmask, (fd_set*)readmask, &wcount);
> +
> +    /* If we have any write watches, we have to force the X server into a polling
> +       select loop */
> +    if (wcount > 0)
> +    {
> +        if (*timeout == NULL)
> +            *timeout = &write_timeout;
> +        (*timeout)->tv_sec = (*timeout)->tv_usec = 0;
> +    }
>  }
>  
>  /*
> @@ -292,7 +307,7 @@ static void select_and_check_watches(void)
>  
>      FD_ZERO(&rfds);
>      FD_ZERO(&wfds);
> -    max_fd = set_watch_fds(&rfds, &wfds);
> +    max_fd = set_watch_fds(&rfds, &wfds, NULL);
>      watch = (SpiceWatch*)watches.next;
>      timeout.tv_sec = timeout.tv_usec = 0;
>      retval = select(max_fd + 1, &rfds, &wfds, NULL, &timeout);
> -- 
> 1.7.9.5
> 

> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> However, while the X server main loop has a facility that lets us add fds to it's
>> select read mask, it has no such facility to let us add fds to it's *write* select mask.
> 
> Right. This is the problem. I think the right solution is to patch the X
> server.

Okay; I unimaginatively decided that would be Hard.  Wouldn't that break
the ABI, and therefore be rejected unilaterally?  I'm willing to try to
craft a patch and submit it; but maybe I just need encouragement <grin>.

Cheers,

Jeremy
On Mon, Jun 04, 2012 at 08:42:17AM -0500, Jeremy White wrote:
> >> However, while the X server main loop has a facility that lets us add fds to it's
> >> select read mask, it has no such facility to let us add fds to it's *write* select mask.
> > 
> > Right. This is the problem. I think the right solution is to patch the X
> > server.
> 
> Okay; I unimaginatively decided that would be Hard.  Wouldn't that break
> the ABI, and therefore be rejected unilaterally?  I'm willing to try to
> craft a patch and submit it; but maybe I just need encouragement <grin>.

I started looking at this, talked to Dave Airlie and he suggested to ask
on xorg-devel either Adam Jackson or Keith Packard about one of two
suggested solutions I fielded, expand BlockHandler with a pReadmask or
add a third callback (in addition to BlockHandler, WakeupHandler)
BlockWriteHandler. The later won't break the internal Xorg / module ABI,
but is a little ugly IMO. The former breaks it.

> 
> Cheers,
> 
> Jeremy
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> Okay; I unimaginatively decided that would be Hard.  Wouldn't that break
>> the ABI, and therefore be rejected unilaterally?  I'm willing to try to
>> craft a patch and submit it; but maybe I just need encouragement <grin>.
> 
> I started looking at this, talked to Dave Airlie and he suggested to ask
> on xorg-devel either Adam Jackson or Keith Packard about one of two
> suggested solutions I fielded, expand BlockHandler with a pReadmask or
> add a third callback (in addition to BlockHandler, WakeupHandler)
> BlockWriteHandler. The later won't break the internal Xorg / module ABI,
> but is a little ugly IMO. The former breaks it.

I've started a conversation on the xorg-devel mailing list[1], and I'll
follow up directly in irc with Adam and/or Keith if I don't get any answers.

Could I ask that my last patch be committed regardless?  You really
cannot use Xspice if the large ping results in an EAGAIN result; the
client just hangs.

Cheers,

Jeremy

[1]
http://lists.x.org/archives/xorg-devel/2012-June/031728.html