Re: [PATCH 2/9] server: pending asyncs should only affect FD_READ/FD_WRITE/FD_CLOSE messages

[Mike Kaplinskiy]

On Wed, Mar 24, 2010 at 2:50 PM, Alexandre Julliard  wrote:
> Mike Kaplinskiy  writes:
> > POLLIN/POLLOUT are selected if we have pending asyncs (as they should
> > be), but we shouldn't add these as pending events since the associated
> > overlapped operations aren't done yet. We can't say for sure if we
> > should notify about these events or not until the async is done and
> > reselects the socket (where we would get another event if need be).
>
> Until the async is done POLLIN/POLLOUT must not be selected, otherwise
> you'll get a busy wait.

We have to select POLLIN/POLLOUT if we have waiting (STATUS_PENDING)
asyncs. Let's say we start an 2 async reads on a socket. This will
force us to POLLIN since there is no data yet. When the data comes, we
will set sock->pmask |= FD_READ to signify there is data available. So
here are two scenarios where we do the wrong thing - if the first
async read reads all of the available data, we falsely notify of
available data, and if it doesn't read everything and returns (as it
should), we will notify of available data even though there is an
async read pending (shouldn't do that).

Now the above doesn't take into account the check we have now in
sock_wake_up, but this check is wrong. If we simply change the current
blocking value (or really anything that causes an _enable_events call)
we flush the events and now we have false events popping up for no
apparent reason.

Basically the idea behind this patch is - in normal socket operation,
even if we ask for POLLIN/POLLOUT it may not because we're waiting to
signal FD_READ/FD_WRITE. There are other reasons we might want to POLL
and we shouldn't notify events for every one of those reasons, since
other things waiting on the poll may make the events stale.

Hope this makes the rationale a bit clearer. Although admittedly the
patch does allow for some busy waiting since
if (mask & FD_READ  || async_waiting( sock->read_q )) ev |= POLLIN | POLLPRI;
should be more like
if ( (mask & FD_READ && !async_busy( sock->read_q ))  ||
async_waiting( sock->read_q )) ev |= POLLIN | POLLPRI;
which probably should've went into #3.

Should I merge 2&3 and resend?

Thanks,
Mike.