[PATCH 0/1] MR2687: ntdll: Retry send on ECONNREFUSED in try_send().
Fixes low FPS in Ghost Recon Breakpoint in dx11 mode. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 12 ++++++++++++ dlls/ws2_32/tests/sock.c | 25 ++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index ac351a17a70..4e706323a0a 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -984,6 +984,7 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async ) { union unix_sockaddr unix_addr; struct msghdr hdr; + int attempt = 0; ssize_t ret; memset( &hdr, 0, sizeof(hdr) ); @@ -1028,6 +1029,17 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async ) else if (errno != EINTR) { if (errno != EWOULDBLOCK) WARN( "sendmsg: %s\n", strerror( errno ) ); + + /* ECONNREFUSED may be returned if this is connected datagram socket and the system received + * ICMP "destination port unreachable" message from the peer. That is ignored + * on Windows. The first sendmsg() will clear the error in this case and the next + * call should succeed. */ + if (!attempt && errno == ECONNREFUSED) + { + ++attempt; + continue; + } + return sock_errno_to_status( errno ); } } diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 1125d4519d3..fe45f5bac81 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -2962,7 +2962,9 @@ static void test_UDP(void) /* peer 0 receives data from all other peers */ struct sock_info peer[NUM_UDP_PEERS]; char buf[16]; - int ss, i, n_recv, n_sent; + int ss, i, n_recv, n_sent, ret; + struct sockaddr_in addr; + int sock; memset (buf,0,sizeof(buf)); for ( i = NUM_UDP_PEERS - 1; i >= 0; i-- ) { @@ -3000,6 +3002,27 @@ static void test_UDP(void) ok ( n_recv == sizeof(buf), "UDP: recvfrom() received wrong amount of data or socket error: %d\n", n_recv ); ok ( memcmp ( &peer[0].peer.sin_port, buf, sizeof(peer[0].addr.sin_port) ) == 0, "UDP: port numbers do not match\n" ); } + + sock = socket( AF_INET, SOCK_DGRAM, IPPROTO_UDP); + ok( sock != INVALID_SOCKET, "got error %u.\n", WSAGetLastError() ); + + memset( &addr, 0, sizeof(addr) ); + addr.sin_family = AF_INET; + addr.sin_addr.s_addr = inet_addr("127.0.0.1"); + addr.sin_port = htons(23456); + + ret = connect( sock, (struct sockaddr *)&addr, sizeof(addr) ); + ok( !ret, "got error %u.\n", WSAGetLastError() ); + + /* Send to UDP socket succeeds even if the packets are not received and the network is replying with + * "destination port unreachable" ICMP messages. */ + for (i = 0; i < 10; ++i) + { + ret = send( sock, buf, sizeof(buf), 0 ); + ok( ret == sizeof(buf), "got %d, error %u.\n", ret, WSAGetLastError() ); + } + + closesocket(sock); } static void test_WSASocket(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2687
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c:
ok ( n_recv == sizeof(buf), "UDP: recvfrom() received wrong amount of data or socket error: %d\n", n_recv ); ok ( memcmp ( &peer[0].peer.sin_port, buf, sizeof(peer[0].addr.sin_port) ) == 0, "UDP: port numbers do not match\n" ); } + + sock = socket( AF_INET, SOCK_DGRAM, IPPROTO_UDP); + ok( sock != INVALID_SOCKET, "got error %u.\n", WSAGetLastError() ); + + memset( &addr, 0, sizeof(addr) ); + addr.sin_family = AF_INET; + addr.sin_addr.s_addr = inet_addr("127.0.0.1"); + addr.sin_port = htons(23456);
This is a bit risky since the port might be in use. Elsewhere we use the guaranteed-invalid port number 255. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32319
Sorry for the late review; I somehow managed to miss this patch. The fact that we're trying to send twice for all socket types makes me a bit nervous, but it's probably okay. I guess there's not a particularly easy way to limit this to UDP. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32320
On Tue May 9 21:20:08 2023 +0000, Zebediah Figura wrote:
Sorry for the late review; I somehow managed to miss this patch. The fact that we're trying to send twice for all socket types makes me a bit nervous, but it's probably okay. I guess there's not a particularly easy way to limit this to UDP. We'd also need to limit it to connected UDP then which both adds complications, if not an extra server call. But I hope we might rely on the sockets not giving ECONNREFUSED for send() in any other cases, error codes are documented and breaking those in Unix kernel will probably break a lot of things anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32373
On Tue May 9 19:40:49 2023 +0000, Zebediah Figura wrote:
This is a bit risky since the port might be in use. Elsewhere we use the guaranteed-invalid port number 255. Yeah, I worried that reserved port might behave differently WRT those ICMP replies and not reproduce the issue but it looks like it does.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32374
On Tue May 9 21:20:07 2023 +0000, Paul Gofman wrote:
We'd also need to limit it to connected UDP then which both adds complications, if not an extra server call. But I hope we might rely on the sockets not giving ECONNREFUSED for send() in any other cases, error codes are documented and breaking those in Unix kernel will probably break a lot of things anyway. I also can imagine that in theory something between the lines of commit 4e6a5d62ad2bbccafa751812ca31eb9ffa25727e may happen (while I am not immediately sure how to trigger that), but in that case calling send() once again won't hurt too probably, even if to get a more sensible error code.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2687#note_32375
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)