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

Submitted by Jeremy White on June 20, 2012, 1:59 p.m.

Details

Message ID 4FE1D738.20709@codeweavers.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jeremy White June 20, 2012, 1:59 p.m.
> 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.

Keith very kindly put together a patch to add write block handlers for us:
  http://lists.x.org/archives/xorg-devel/2012-June/031783.html

It not only lets us handler write blocks properly, but it also lets us
remove our select and tidy our code a bit (or at least I think it does :-/).

I've attached a patch, for discussion only (needs configure checks etc),
to see if this all makes sense to everyone else as well.

Other eyeballs appreciated.

Cheers,

Jeremy

Patch hide | download patch | download mbox

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index e57fb91..2007970 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -220,19 +220,37 @@  static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
     ring_item_init(&watch->link);
     ring_add(&watches, &watch->link);
     watch_count++;
+
+    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
+        AddWriteSocket(watch->fd);
+    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
+        AddGeneralSocket(watch->fd);
+
     return watch;
 }
 
 static void watch_update_mask(SpiceWatch *watch, int event_mask)
 {
     DPRINTF(0, "fd %d to %d", watch->fd, event_mask);
+    if ((watch->event_mask & SPICE_WATCH_EVENT_WRITE) && !(event_mask & SPICE_WATCH_EVENT_WRITE))
+        RemoveWriteSocket(watch->fd);
+    if ((watch->event_mask & SPICE_WATCH_EVENT_READ) && !(event_mask & SPICE_WATCH_EVENT_READ))
+        RemoveGeneralSocket(watch->fd);
     watch->event_mask = event_mask;
+    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
+        AddWriteSocket(watch->fd);
+    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
+        AddGeneralSocket(watch->fd);
 }
 
 static void watch_remove(SpiceWatch *watch)
 {
     DPRINTF(0, "remove %p (fd %d)", watch, watch->fd);
     watch->remove = TRUE;
+    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
+        RemoveWriteSocket(watch->fd);
+    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
+        RemoveGeneralSocket(watch->fd);
     watch_count--;
 }
 
@@ -241,70 +259,21 @@  static void channel_event(int event, SpiceChannelEventInfo *info)
     NOT_IMPLEMENTED
 }
 
-static int set_watch_fds(fd_set *rfds, fd_set *wfds)
+static void check_watches(int retval)
 {
     SpiceWatch *watch;
     RingItem *link;
     RingItem *next;
-    int max_fd = -1;
-
-    RING_FOREACH_SAFE(link, next, &watches) {
-        watch = (SpiceWatch*)link;
-        if (watch->event_mask & SPICE_WATCH_EVENT_READ) {
-            FD_SET(watch->fd, rfds);
-            max_fd = watch->fd > max_fd ? watch->fd : max_fd;
-        }
-        if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
-            FD_SET(watch->fd, wfds);
-            max_fd = watch->fd > max_fd ? watch->fd : max_fd;
-        }
-    }
-    return max_fd;
-}
 
-/*
- * called just before the X server goes into select()
- * 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 void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
-{
-    /* set all our fd's */
-    set_watch_fds((fd_set*)readmask, (fd_set*)readmask);
-}
-
-/*
- * xserver only calles wakeup_handler with the read fd_set, so we
- * must either patch it or do a polling select ourselves, this is the
- * later approach. Since we are already doing a polling select, we
- * already select on all (we could avoid selecting on the read since
- * that *is* actually taken care of by the wakeup handler).
- */
-static void select_and_check_watches(void)
-{
-    fd_set rfds, wfds;
-    int max_fd = -1;
-    SpiceWatch *watch;
-    RingItem *link;
-    RingItem *next;
-    struct timeval timeout;
-    int retval;
-
-    FD_ZERO(&rfds);
-    FD_ZERO(&wfds);
-    max_fd = set_watch_fds(&rfds, &wfds);
-    watch = (SpiceWatch*)watches.next;
-    timeout.tv_sec = timeout.tv_usec = 0;
-    retval = select(max_fd + 1, &rfds, &wfds, NULL, &timeout);
     if (retval) {
         RING_FOREACH_SAFE(link, next, &watches) {
             watch = (SpiceWatch*)link;
             if ((watch->event_mask & SPICE_WATCH_EVENT_READ)
-                 && FD_ISSET(watch->fd, &rfds)) {
+                 && CheckReadSocket(watch->fd)) {
                 watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
             }
             if (!watch->remove && (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
-                 && FD_ISSET(watch->fd, &wfds)) {
+                 && CheckWriteSocket(watch->fd)) {
                 watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
             }
             if (watch->remove) {
@@ -315,27 +284,15 @@  static void select_and_check_watches(void)
     }
 }
 
-static int no_write_watches(Ring *w)
+static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
 {
-    SpiceWatch *watch;
-    RingItem *link;
-    RingItem *next;
-
-    RING_FOREACH_SAFE(link, next, w) {
-        watch = (SpiceWatch*)link;
-        if (!watch->remove && (watch->event_mask & SPICE_WATCH_EVENT_WRITE))
-            return 0;
-    }
-
-    return 1;
+    /* We need a non-screen wakeup handler, and if you want one of those, you
+       have to provide a block handler as well... */
 }
 
 static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
 {
-    if (!nfds && no_write_watches(&watches)) {
-        return;
-    }
-    select_and_check_watches();
+    check_watches(nfds);
 }
 
 SpiceCoreInterface *basic_event_loop_init(void)

Comments

On Wed, Jun 20, 2012 at 08:59:20AM -0500, Jeremy White wrote:
> > 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.
> 
> Keith very kindly put together a patch to add write block handlers for us:
>   http://lists.x.org/archives/xorg-devel/2012-June/031783.html
> 
> It not only lets us handler write blocks properly, but it also lets us
> remove our select and tidy our code a bit (or at least I think it does :-/).
> 
> I've attached a patch, for discussion only (needs configure checks etc),
> to see if this all makes sense to everyone else as well.
> 
> Other eyeballs appreciated.

Excellent! Patch looks good at first pass. I'll try to test it later.

> 
> Cheers,
> 
> Jeremy

> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index e57fb91..2007970 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
> @@ -220,19 +220,37 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
>      ring_item_init(&watch->link);
>      ring_add(&watches, &watch->link);
>      watch_count++;
> +
> +    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
> +        AddWriteSocket(watch->fd);
> +    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
> +        AddGeneralSocket(watch->fd);
> +
>      return watch;
>  }
>  
>  static void watch_update_mask(SpiceWatch *watch, int event_mask)
>  {
>      DPRINTF(0, "fd %d to %d", watch->fd, event_mask);
> +    if ((watch->event_mask & SPICE_WATCH_EVENT_WRITE) && !(event_mask & SPICE_WATCH_EVENT_WRITE))
> +        RemoveWriteSocket(watch->fd);
> +    if ((watch->event_mask & SPICE_WATCH_EVENT_READ) && !(event_mask & SPICE_WATCH_EVENT_READ))
> +        RemoveGeneralSocket(watch->fd);
>      watch->event_mask = event_mask;
> +    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
> +        AddWriteSocket(watch->fd);
> +    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
> +        AddGeneralSocket(watch->fd);
>  }
>  
>  static void watch_remove(SpiceWatch *watch)
>  {
>      DPRINTF(0, "remove %p (fd %d)", watch, watch->fd);
>      watch->remove = TRUE;
> +    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
> +        RemoveWriteSocket(watch->fd);
> +    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
> +        RemoveGeneralSocket(watch->fd);
>      watch_count--;
>  }
>  
> @@ -241,70 +259,21 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
>      NOT_IMPLEMENTED
>  }
>  
> -static int set_watch_fds(fd_set *rfds, fd_set *wfds)
> +static void check_watches(int retval)
>  {
>      SpiceWatch *watch;
>      RingItem *link;
>      RingItem *next;
> -    int max_fd = -1;
> -
> -    RING_FOREACH_SAFE(link, next, &watches) {
> -        watch = (SpiceWatch*)link;
> -        if (watch->event_mask & SPICE_WATCH_EVENT_READ) {
> -            FD_SET(watch->fd, rfds);
> -            max_fd = watch->fd > max_fd ? watch->fd : max_fd;
> -        }
> -        if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
> -            FD_SET(watch->fd, wfds);
> -            max_fd = watch->fd > max_fd ? watch->fd : max_fd;
> -        }
> -    }
> -    return max_fd;
> -}
>  
> -/*
> - * called just before the X server goes into select()
> - * 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 void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
> -{
> -    /* set all our fd's */
> -    set_watch_fds((fd_set*)readmask, (fd_set*)readmask);
> -}
> -
> -/*
> - * xserver only calles wakeup_handler with the read fd_set, so we
> - * must either patch it or do a polling select ourselves, this is the
> - * later approach. Since we are already doing a polling select, we
> - * already select on all (we could avoid selecting on the read since
> - * that *is* actually taken care of by the wakeup handler).
> - */
> -static void select_and_check_watches(void)
> -{
> -    fd_set rfds, wfds;
> -    int max_fd = -1;
> -    SpiceWatch *watch;
> -    RingItem *link;
> -    RingItem *next;
> -    struct timeval timeout;
> -    int retval;
> -
> -    FD_ZERO(&rfds);
> -    FD_ZERO(&wfds);
> -    max_fd = set_watch_fds(&rfds, &wfds);
> -    watch = (SpiceWatch*)watches.next;
> -    timeout.tv_sec = timeout.tv_usec = 0;
> -    retval = select(max_fd + 1, &rfds, &wfds, NULL, &timeout);
>      if (retval) {
>          RING_FOREACH_SAFE(link, next, &watches) {
>              watch = (SpiceWatch*)link;
>              if ((watch->event_mask & SPICE_WATCH_EVENT_READ)
> -                 && FD_ISSET(watch->fd, &rfds)) {
> +                 && CheckReadSocket(watch->fd)) {
>                  watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
>              }
>              if (!watch->remove && (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
> -                 && FD_ISSET(watch->fd, &wfds)) {
> +                 && CheckWriteSocket(watch->fd)) {
>                  watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
>              }
>              if (watch->remove) {
> @@ -315,27 +284,15 @@ static void select_and_check_watches(void)
>      }
>  }
>  
> -static int no_write_watches(Ring *w)
> +static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readmask)
>  {
> -    SpiceWatch *watch;
> -    RingItem *link;
> -    RingItem *next;
> -
> -    RING_FOREACH_SAFE(link, next, w) {
> -        watch = (SpiceWatch*)link;
> -        if (!watch->remove && (watch->event_mask & SPICE_WATCH_EVENT_WRITE))
> -            return 0;
> -    }
> -
> -    return 1;
> +    /* We need a non-screen wakeup handler, and if you want one of those, you
> +       have to provide a block handler as well... */
>  }
>  
>  static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
>  {
> -    if (!nfds && no_write_watches(&watches)) {
> -        return;
> -    }
> -    select_and_check_watches();
> +    check_watches(nfds);
>  }
>  
>  SpiceCoreInterface *basic_event_loop_init(void)

> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel