Re: [PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
> On Sep 18, 2016, at 09:58, Keith Packard wrote: > > Matthieu Herrb writes: > >> 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. > > I think Jeremy's patch will cause the server to spin while any client is > not writable, which isn't good. I don't believe that is the case because ClientReady does: ospoll_mute(server_poll, fd, X_NOTIFY_WRITE); If I read the flow right, the level trigger causes WaitForSomething to spin up only when a client is ready to receive data. ClientReady then mutes the trigger. FlushClient then possibly re-enables it if we EAGAIN. If/when the client is ready again (level trigger), ClientReady will fire again. If the client is not ready to receive data, ClientReady won't fire. My patch does indeed cause us to needlessly FlushAllOutput() if something else (eg: a new connection, new request) causes WaitForSomething() to spin up, but that's no different than someone currently just setting NewOutputPending=YES even though there is no pending output. >> 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 :) > > I'd bet anyone using poll(2) instead of epoll(2) will see the same > problems. It should be possible to reproduce this on Linux by using the > poll path instead of epoll. smime.p7s Description: S/MIME cryptographic signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
Matthieu Herrb writes: > 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. I think Jeremy's patch will cause the server to spin while any client is not writable, which isn't good. > 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 :) I'd bet anyone using poll(2) instead of epoll(2) will see the same problems. It should be possible to reproduce this on Linux by using the poll path instead of epoll. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
On Sun, Sep 18, 2016 at 08:42:00AM -0700, Keith Packard wrote:
> Jeremy Huddleston Sequoia 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
pgpmGGI83FWWV.pgp
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
Jeremy Huddleston Sequoia 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
signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
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. Signed-off-by: Jeremy Huddleston Sequoia CC: Keith Packard --- os/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/connection.c b/os/connection.c index 0d42184..4630438 100644 --- a/os/connection.c +++ b/os/connection.c @@ -749,7 +749,7 @@ AllocNewConnection(XtransConnInfo trans_conn, int fd, CARD32 conn_time) SetConnectionTranslation(fd, client->index); #endif ospoll_add(server_poll, fd, - ospoll_trigger_edge, + ospoll_trigger_level, ClientReady, client); set_poll_client(client); -- 2.10.0 (Apple Git-99) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
