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

[Jeremy White]

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(-)

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Attached Message Part
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120602/936929fe/attachment.ksh>