Re: [PATCH 2/9] server: pending asyncs should only affect FD_READ/FD_WRITE/FD_CLOSE messages
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
--- server/async.c | 8 ++++++++ server/file.h | 1 + server/sock.c | 26 +++++++++++--------------- 3 files changed, 20 insertions(+), 15 deletions(-)
This doesn't look right. You shouldn't ignore POLLIN/POLLOUT, they shouldn't have been selected in the first place. If you do get them you have to do something with them and unselect them. -- Alexandre Julliard julliard(a)winehq.org
On Wed, Mar 24, 2010 at 2:28 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
--- server/async.c | 8 ++++++++ server/file.h | 1 + server/sock.c | 26 +++++++++++--------------- 3 files changed, 20 insertions(+), 15 deletions(-)
This doesn't look right. You shouldn't ignore POLLIN/POLLOUT, they shouldn't have been selected in the first place. If you do get them you have to do something with them and unselect them.
-- Alexandre Julliard julliard(a)winehq.org
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). It's basically the more correct version of not signaling events when we have asyncs pending. Mike.
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
On Wed, Mar 24, 2010 at 2:50 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> 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.
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
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?
I think there's more to it than merging them. If you have to ignore events you are doing something wrong, because you shouldn't get the events in the first place. -- Alexandre Julliard julliard(a)winehq.org
On Wed, Mar 24, 2010 at 3:52 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> writes:
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?
I think there's more to it than merging them. If you have to ignore events you are doing something wrong, because you shouldn't get the events in the first place.
Well we wouldn't be ignoring them, just not using them for FD_READ/FD_WRITE. We need POLLIN/POLLOUT when we have pending asyncs, but we shouldn't set FD_READ/FD_WRITE every time we get a POLLIN/POLLOUT since these events were needed by the overlapped operations. Mike.
participants (2)
-
Alexandre Julliard -
Mike Kaplinskiy