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

[Jeremy Huddleston Sequoia]

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4465 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160918/f9b3461d/attachment.bin>