[Spice-devel] More on xspice + failure in net ping over vpn

Submitted by Jeremy White on May 4, 2012, 4:36 p.m.

Details

Message ID 4FA405A7.5060301@codeweavers.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jeremy White May 4, 2012, 4:36 p.m.
So I've probed more deeply after my naive misunderstanding
of the consequence of the EAGAIN.

I believe I am closer to the source of the problem; I also
believe the problem is more serious than I realized; it's likely to bite
anyone trying the xspice solution quite hard.

That is, in spiceqxl_main_loop.c in the qxl X driver, there
is a basic implementation of watch_add/watch_update_mask.
The watch stuff is vital to retrying sends when they don't
go out due to a blocking condition.

However, that implementation does not function fully.  It relies
on client activity on the X server to trigger a select polling
loop.  But when sending the large initial network ping packet,
we have no client activity, and we just hang.  The attached
patch 'improves' things, but I really have no sense as to whether
or not it would be a reliable solution.

I looked at trying to use the red_worker watch add stuff instead,
and I see that gets hairy.  You need the watches before you
have a channel; the red_worker stuff relies on having a channel
to hang it's stuff off of.

But I have to confess it feels right - we've got our own thread;
we created the socket, and do all the listens/accept - why don't we
do our own watching?

Cheers,

Jeremy

Patch hide | download patch | download mbox

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index 1718861..b06d5eb 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -317,7 +317,7 @@  static void select_and_check_watches(void)
 
 static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
 {
-    if (!nfds) {
+    if (!nfds && ring_is_empty(&watches)) {
         return;
     }
     select_and_check_watches();

Comments

On Fri, May 04, 2012 at 11:36:55AM -0500, Jeremy White wrote:
> So I've probed more deeply after my naive misunderstanding
> of the consequence of the EAGAIN.
> 
> I believe I am closer to the source of the problem; I also
> believe the problem is more serious than I realized; it's likely to bite
> anyone trying the xspice solution quite hard.
> 
> That is, in spiceqxl_main_loop.c in the qxl X driver, there
> is a basic implementation of watch_add/watch_update_mask.
> The watch stuff is vital to retrying sends when they don't
> go out due to a blocking condition.
> 
> However, that implementation does not function fully.  It relies
> on client activity on the X server to trigger a select polling
> loop.  But when sending the large initial network ping packet,
> we have no client activity, and we just hang.  The attached
> patch 'improves' things, but I really have no sense as to whether
> or not it would be a reliable solution.

OK, thanks for tracking this down.

> 
> I looked at trying to use the red_worker watch add stuff instead,
> and I see that gets hairy.  You need the watches before you
> have a channel; the red_worker stuff relies on having a channel
> to hang it's stuff off of.
> 
> But I have to confess it feels right - we've got our own thread;
> we created the socket, and do all the listens/accept - why don't we
> do our own watching?
> 

There is a thread for the display+cursor channel, but the rest are using
the main (and only) X server thread. Specifically the main channel,
which sends the ping message, is in that thread. I don't understand the
issue - is the select not being reached, or is it missing the spice fds?

> Cheers,
> 
> Jeremy

> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index 1718861..b06d5eb 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
> @@ -317,7 +317,7 @@ static void select_and_check_watches(void)
>  
>  static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
>  {
> -    if (!nfds) {
> +    if (!nfds && ring_is_empty(&watches)) {
>          return;
>      }
>      select_and_check_watches();

> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> There is a thread for the display+cursor channel, but the rest are using
> the main (and only) X server thread. Specifically the main channel,
> which sends the ping message, is in that thread. I don't understand the
> issue - is the select not being reached, or is it missing the spice fds?

Ah, okay, I hadn't figured out that the main thread is the X server
thread.  I also struggled finding documentation from X on
RegisterBlockAndWakeupHandlers.  It looks like the primary way our code
gets activated is via xspice_wakeup_handler, where we call
select_and_check_watches().  But only if nfds is > 0.

So I'm not sure if there is a way to get X to also watch our fds; that
would be best, right?  My quick googling turned up
xf86AddEnabledDevice() and/or xf86InstallSIGIOHandler(), but here my
ignorance really shines.

If not, does xspice_wakeup_handler get called with nfds == 0 reliably
enough that my patch would be 'good enough'?

Cheers,

Jeremy