> On Sep 18, 2016, at 08:51, Keith Packard <kei...@keithp.com> wrote: > > Jeremy Huddleston Sequoia <jerem...@apple.com> writes: > >> Use any_output_pending() instead. > > These aren't equivalent -- NewOutputPending is set when there is output > pending and we haven't detected that all of the clients with output > are blocked.
The comment says that NewOutputPending is set when there is output that hasn't been attempted yet, which is slightly different. The usage doesn't match the name/comment, so it is a tad confusing trying to see where it should or should not be set. > There's a patch for a bug in FlushAllOutput to set NewOutputPending when > we skip a client that has pending input. I attached that to my previous > message in this thread. > > I think an alternative would be to remove write blocked clients from the > output pending list and re-add them when they became writable > again. Then we could use any_output_pending() instead of NewOutputPending. Yeah, I was not liking the behavior of "something is ready, so try everything" that NewOutputPending is part of. I like the idea of only iterating over a list of clients that are expected to be unblocked. --- > On Sep 18, 2016, at 08:42, Keith Packard <kei...@keithp.com> wrote: > > Jeremy Huddleston Sequoia <jerem...@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; > } > } > } The issue remains with just these two changes applied. In my particular case, !client_is_ready(client), so we're not hitting that second bug, but yes, it would definitely be an issue. IMO, it would be best to not have a NewOutputPending boolean at all and base it solely on the list of pending clients (with the optimization of removing EWOULDBLOCK clients until they are ready for data).
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