[PATCH 1/2] server: Only remove non-listening fd from poll
This makes sure we poll listening non-connection file descriptors even if the event is POLLERR or POLLHUP. Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/sock.c b/server/sock.c index befa9117c13..ce2f390ec05 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1024,7 +1024,7 @@ static void sock_poll_event( struct fd *fd, int event ) fprintf(stderr, "socket %p select event: %x\n", sock, event); /* we may change event later, remove from loop here */ - if (event & (POLLERR|POLLHUP)) set_fd_events( sock->fd, -1 ); + if (event & (POLLERR|POLLHUP) && sock->state != SOCK_LISTENING) set_fd_events( sock->fd, -1 ); switch (sock->state) { -- 2.32.0
Make sure we're not going to poll on connectionless file descriptors which have received shutdown signals on their read and write ends. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51319 Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/sock.c b/server/sock.c index ce2f390ec05..ecf10441c2b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1197,6 +1197,10 @@ static int sock_get_poll_events( struct fd *fd ) { ev |= POLLOUT; } + if (sock->rd_shutdown && sock->wr_shutdown && ev == 0) + { + ev = -1; + } break; } -- 2.32.0
On 7/2/21 1:06 PM, David Koolhoven wrote:
Make sure we're not going to poll on connectionless file descriptors which have received shutdown signals on their read and write ends.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51319 Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/server/sock.c b/server/sock.c index ce2f390ec05..ecf10441c2b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1197,6 +1197,10 @@ static int sock_get_poll_events( struct fd *fd ) { ev |= POLLOUT; } + if (sock->rd_shutdown && sock->wr_shutdown && ev == 0) + { + ev = -1; + }
break; }
This seems reasonable to me, but I feel like it deserves a comment—in the code, rather than in the commit message—that describes why we need to do this. See the comment for the SOCK_UNCONNECTED case above.
Dne 05. 07. 21 v 21:54 Zebediah Figura (she/her) napsal(a):
On 7/2/21 1:06 PM, David Koolhoven wrote:
Make sure we're not going to poll on connectionless file descriptors which have received shutdown signals on their read and write ends.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51319 Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/server/sock.c b/server/sock.c index ce2f390ec05..ecf10441c2b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1197,6 +1197,10 @@ static int sock_get_poll_events( struct fd *fd ) { ev |= POLLOUT; } + if (sock->rd_shutdown && sock->wr_shutdown && ev == 0) + { + ev = -1; + } break; }
This seems reasonable to me, but I feel like it deserves a comment—in the code, rather than in the commit message—that describes why we need to do this. See the comment for the SOCK_UNCONNECTED case above.
Hi, I'm facing a regression of a networking application with the same symptoms (wineserver at 100%). There is a little progress only if both these patches are applied but 100% peaks can still be observed at times (which are not with Wine 6.0.1) and the application still suffers from some errors. It seems to me that there is a deeper problem.
On 7/2/21 1:06 PM, David Koolhoven wrote:
This makes sure we poll listening non-connection file descriptors even if the event is POLLERR or POLLHUP.
Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/sock.c b/server/sock.c index befa9117c13..ce2f390ec05 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1024,7 +1024,7 @@ static void sock_poll_event( struct fd *fd, int event ) fprintf(stderr, "socket %p select event: %x\n", sock, event);
/* we may change event later, remove from loop here */ - if (event & (POLLERR|POLLHUP)) set_fd_events( sock->fd, -1 ); + if (event & (POLLERR|POLLHUP) && sock->state != SOCK_LISTENING) set_fd_events( sock->fd, -1 );
switch (sock->state) {
This doesn't look right. Why do we need to do this?
On 7/2/21 9:33 PM, Zebediah Figura (she/her) wrote:
On 7/2/21 1:06 PM, David Koolhoven wrote:
This makes sure we poll listening non-connection file descriptors even if the event is POLLERR or POLLHUP.
Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/sock.c b/server/sock.c index befa9117c13..ce2f390ec05 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1024,7 +1024,7 @@ static void sock_poll_event( struct fd *fd, int event ) fprintf(stderr, "socket %p select event: %x\n", sock, event); /* we may change event later, remove from loop here */ - if (event & (POLLERR|POLLHUP)) set_fd_events( sock->fd, -1 ); + if (event & (POLLERR|POLLHUP) && sock->state != SOCK_LISTENING) set_fd_events( sock->fd, -1 ); switch (sock->state) {
This doesn't look right. Why do we need to do this?
I only added this because there was no code path with sock->state == SOCK_LISTENING that didn't already lead to a set_fd_events call, which overrides this one, so calling it here is useless. It doesn't fix anything and might add more code complexity than optimizing away one epoll_ctl call is worth.
On 7/2/21 4:59 PM, Torge Matthies wrote:
On 7/2/21 9:33 PM, Zebediah Figura (she/her) wrote:
On 7/2/21 1:06 PM, David Koolhoven wrote:
This makes sure we poll listening non-connection file descriptors even if the event is POLLERR or POLLHUP.
Signed-off-by: David Koolhoven <david(a)koolhoven-home.net> --- server/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/sock.c b/server/sock.c index befa9117c13..ce2f390ec05 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1024,7 +1024,7 @@ static void sock_poll_event( struct fd *fd, int event ) fprintf(stderr, "socket %p select event: %x\n", sock, event); /* we may change event later, remove from loop here */ - if (event & (POLLERR|POLLHUP)) set_fd_events( sock->fd, -1 ); + if (event & (POLLERR|POLLHUP) && sock->state != SOCK_LISTENING) set_fd_events( sock->fd, -1 ); switch (sock->state) {
This doesn't look right. Why do we need to do this?
I only added this because there was no code path with sock->state == SOCK_LISTENING that didn't already lead to a set_fd_events call, which overrides this one, so calling it here is useless. It doesn't fix anything and might add more code complexity than optimizing away one epoll_ctl call is worth.
Personally I'm inclined to say that this code could use less code complexity rather than more (and I doubt the optimization makes a difference anyway.)
participants (4)
-
David Koolhoven -
Roman Pišl -
Torge Matthies -
Zebediah Figura (she/her)