[PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger

[Matthieu Herrb]

On Sun, Sep 18, 2016 at 08:42:00AM -0700, Keith Packard wrote:
> Jeremy Huddleston Sequoia <jeremyhu at apple.com> writes:
> 
> > On encountering an EAGAIN, FlushClient() re-adds the client to the
> > pending list and returns, but FlushClient() doesn't get called again.
> > We would expect it to be called via FlushAllOutput() via
> > WaitForSomething(), but that only happens when NewOutputPending is
> > set.  FlushAllOutput() also early-exits if NewOutputPending is not
> > set.
> >
> > The only place that is setting NewOutputPending is ClientReady().  The
> > problem there is that ClientReady() is called based on an edge trigger
> > and not a level trigger.
> 
> Hrm. I think this is a problem with our emulation of edge triggers as we
> don't tell the ospoll layer when we've detected that the client is not
> writable.
> 
> This should fix that; it makes the edge re-trigger whenever we start
> listening again:
> 
> diff --git a/os/ospoll.c b/os/ospoll.c
> index b00d422..8c99501 100644
> --- a/os/ospoll.c
> +++ b/os/ospoll.c
> @@ -352,10 +352,14 @@ ospoll_listen(struct ospoll *ospoll, int fd, int xevents)
>          epoll_mod(ospoll, osfd);
>  #endif
>  #if POLL
> -        if (xevents & X_NOTIFY_READ)
> +        if (xevents & X_NOTIFY_READ) {
>              ospoll->fds[pos].events |= POLLIN;
> -        if (xevents & X_NOTIFY_WRITE)
> +            ospoll->fds[pos].revents &= ~POLLIN;
> +        }
> +        if (xevents & X_NOTIFY_WRITE) {
>              ospoll->fds[pos].events |= POLLOUT;
> +            ospoll->fds[pos].revents &= ~POLLOUT;
> +        }
>  #endif
>      }
>  }
> 
> I did find a bug with the NewOutputPending flag. We don't actually flush
> the client's buffer if there are still requests queued. This is a change
> From the previous behavior.
> 
> diff --git a/os/io.c b/os/io.c
> index ff0ec3d..c6db028 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -615,6 +615,8 @@ FlushAllOutput(void)
>          if (!client_is_ready(client)) {
>              oc = (OsCommPtr) client->osPrivate;
>              (void) FlushClient(client, oc, (char *) NULL, 0);
> +        } else {
> +            NewOutputPending = TRUE;
>          }
>      }
>  }
> 
> -- 
> -keith


Hi

this doesn't fix an issue I'm seeing on OpenBSD with xterm not beeing
able to start (it makes the X server spin at 100% CPU in
WaitForSomething()), while Jeremy's patches do fix the issue for me.

Some other applications like xlogo, emacs 24 , or the XFCE4 base
desktop environment don't trigger it, but some other big applications
like inkscape trigger the same issue.

I've not been able to really understand the issue yet, but that's a
data point. (And I'm glad to see that it doesn't seem to be an OpenBSD
specific issue :)
-- 
Matthieu Herrb
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160918/84ceaa2b/attachment.sig>