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

[Alon Levy]

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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel