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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel