Re: [PATCH 2/2] os/connection: Remove NewOutputPending

[Jeremy Huddleston Sequoia]

> On Sep 18, 2016, at 08:51, Keith Packard  wrote:
> 
> Jeremy Huddleston Sequoia  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  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;
> }
> }
> }

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

Re: [PATCH 2/2] os/connection: Remove NewOutputPending

[Keith Packard]

Jeremy Huddleston Sequoia  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.

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.

-- 
-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