From 1e6afd0dbc1c581435588e1e9bb419a035b81028 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Mon, 29 Oct 2018 16:30:30 -0400 Subject: [PATCH 1/5] Introduce and use constant SELECT_TIMEOUT_MILLISECONDS. --- src/net.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index b85a8c2c1d..0613e05998 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -71,6 +71,10 @@ enum BindFlags { BF_WHITELIST = (1U << 2), }; +// The set of sockets cannot be modified while waiting +// The sleep time needs to be small to avoid new sockets stalling +static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50; + const static std::string NET_MESSAGE_COMMAND_OTHER = "*other*"; static const uint64_t RANDOMIZER_ID_NETGROUP = 0x6c0edd8036ef4036ULL; // SHA256("netgroup")[0:8] @@ -1264,7 +1268,7 @@ void CConnman::SocketHandler() // struct timeval timeout; timeout.tv_sec = 0; - timeout.tv_usec = 50000; // frequency to poll pnode->vSend + timeout.tv_usec = SELECT_TIMEOUT_MILLISECONDS * 1000; // frequency to poll pnode->vSend fd_set fdsetRecv; fd_set fdsetSend; @@ -1337,7 +1341,7 @@ void CConnman::SocketHandler() } FD_ZERO(&fdsetSend); FD_ZERO(&fdsetError); - if (!interruptNet.sleep_for(std::chrono::milliseconds(timeout.tv_usec/1000))) + if (!interruptNet.sleep_for(std::chrono::milliseconds(SELECT_TIMEOUT_MILLISECONDS))) return; } From 7e403c0ae705455aa66f7df9a9a99f462fd4e9a8 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Sep 2018 15:32:07 -0400 Subject: [PATCH 2/5] Move GenerateSelectSet logic to private method. This separates the socket event collection logic from the logic deciding which events we're interested in at all. --- src/net.cpp | 87 ++++++++++++++++++++++++++++++++--------------------- src/net.h | 1 + 2 files changed, 53 insertions(+), 35 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0613e05998..9d837192af 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1261,28 +1261,10 @@ void CConnman::InactivityCheck(CNode *pnode) } } -void CConnman::SocketHandler() +bool CConnman::GenerateSelectSet(std::set &recv_set, std::set &send_set, std::set &error_set) { - // - // Find which sockets have data to receive - // - struct timeval timeout; - timeout.tv_sec = 0; - timeout.tv_usec = SELECT_TIMEOUT_MILLISECONDS * 1000; // frequency to poll pnode->vSend - - fd_set fdsetRecv; - fd_set fdsetSend; - fd_set fdsetError; - FD_ZERO(&fdsetRecv); - FD_ZERO(&fdsetSend); - FD_ZERO(&fdsetError); - SOCKET hSocketMax = 0; - bool have_fds = false; - for (const ListenSocket& hListenSocket : vhListenSocket) { - FD_SET(hListenSocket.socket, &fdsetRecv); - hSocketMax = std::max(hSocketMax, hListenSocket.socket); - have_fds = true; + recv_set.insert(hListenSocket.socket); } { @@ -1311,34 +1293,69 @@ void CConnman::SocketHandler() if (pnode->hSocket == INVALID_SOCKET) continue; - FD_SET(pnode->hSocket, &fdsetError); - hSocketMax = std::max(hSocketMax, pnode->hSocket); - have_fds = true; - + error_set.insert(pnode->hSocket); if (select_send) { - FD_SET(pnode->hSocket, &fdsetSend); + send_set.insert(pnode->hSocket); continue; } if (select_recv) { - FD_SET(pnode->hSocket, &fdsetRecv); + recv_set.insert(pnode->hSocket); } } } - int nSelect = select(have_fds ? hSocketMax + 1 : 0, - &fdsetRecv, &fdsetSend, &fdsetError, &timeout); + return !recv_set.empty() || !send_set.empty() || !error_set.empty(); +} + +void CConnman::SocketHandler() +{ + std::set recv_select_set, send_select_set, error_select_set; + if (!GenerateSelectSet(recv_select_set, send_select_set, error_select_set)) { + interruptNet.sleep_for(std::chrono::milliseconds(SELECT_TIMEOUT_MILLISECONDS)); + return; + } + + // + // Find which sockets have data to receive + // + struct timeval timeout; + timeout.tv_sec = 0; + timeout.tv_usec = SELECT_TIMEOUT_MILLISECONDS * 1000; // frequency to poll pnode->vSend + + fd_set fdsetRecv; + fd_set fdsetSend; + fd_set fdsetError; + FD_ZERO(&fdsetRecv); + FD_ZERO(&fdsetSend); + FD_ZERO(&fdsetError); + SOCKET hSocketMax = 0; + + for (SOCKET hSocket : recv_select_set) { + FD_SET(hSocket, &fdsetRecv); + hSocketMax = std::max(hSocketMax, hSocket); + } + + for (SOCKET hSocket : send_select_set) { + FD_SET(hSocket, &fdsetSend); + hSocketMax = std::max(hSocketMax, hSocket); + } + + for (SOCKET hSocket : error_select_set) { + FD_SET(hSocket, &fdsetError); + hSocketMax = std::max(hSocketMax, hSocket); + } + + int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, &fdsetError, &timeout); + if (interruptNet) return; if (nSelect == SOCKET_ERROR) { - if (have_fds) - { - int nErr = WSAGetLastError(); - LogPrintf("socket select error %s\n", NetworkErrorString(nErr)); - for (unsigned int i = 0; i <= hSocketMax; i++) - FD_SET(i, &fdsetRecv); - } + int nErr = WSAGetLastError(); + LogPrintf("socket select error %s\n", NetworkErrorString(nErr)); + for (unsigned int i = 0; i <= hSocketMax; i++) + FD_SET(i, &fdsetRecv); FD_ZERO(&fdsetSend); FD_ZERO(&fdsetError); if (!interruptNet.sleep_for(std::chrono::milliseconds(SELECT_TIMEOUT_MILLISECONDS))) diff --git a/src/net.h b/src/net.h index a059d32c89..27121ab213 100644 --- a/src/net.h +++ b/src/net.h @@ -342,6 +342,7 @@ private: void DisconnectNodes(); void NotifyNumConnectionsChanged(); void InactivityCheck(CNode *pnode); + bool GenerateSelectSet(std::set &recv_set, std::set &send_set, std::set &error_set); void SocketHandler(); void ThreadSocketHandler(); void ThreadDNSAddressSeed(); From 28211a4bc9c65859b641b81a0541726a0e01988f Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Wed, 26 Sep 2018 21:51:46 -0400 Subject: [PATCH 3/5] Move SocketEvents logic to private method. This separates the select() logic from the socket handling logic, setting up for a switch to poll(). --- src/net.cpp | 36 +++++++++++++++++++++++++++++++----- src/net.h | 1 + 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9d837192af..2664828034 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1307,7 +1307,7 @@ bool CConnman::GenerateSelectSet(std::set &recv_set, std::set &s return !recv_set.empty() || !send_set.empty() || !error_set.empty(); } -void CConnman::SocketHandler() +void CConnman::SocketEvents(std::set &recv_set, std::set &send_set, std::set &error_set) { std::set recv_select_set, send_select_set, error_select_set; if (!GenerateSelectSet(recv_select_set, send_select_set, error_select_set)) { @@ -1362,12 +1362,38 @@ void CConnman::SocketHandler() return; } + for (SOCKET hSocket : recv_select_set) { + if (FD_ISSET(hSocket, &fdsetRecv)) { + recv_set.insert(hSocket); + } + } + + for (SOCKET hSocket : send_select_set) { + if (FD_ISSET(hSocket, &fdsetSend)) { + send_set.insert(hSocket); + } + } + + for (SOCKET hSocket : error_select_set) { + if (FD_ISSET(hSocket, &fdsetError)) { + error_set.insert(hSocket); + } + } +} + +void CConnman::SocketHandler() +{ + std::set recv_set, send_set, error_set; + SocketEvents(recv_set, send_set, error_set); + + if (interruptNet) return; + // // Accept new connections // for (const ListenSocket& hListenSocket : vhListenSocket) { - if (hListenSocket.socket != INVALID_SOCKET && FD_ISSET(hListenSocket.socket, &fdsetRecv)) + if (hListenSocket.socket != INVALID_SOCKET && recv_set.count(hListenSocket.socket) > 0) { AcceptConnection(hListenSocket); } @@ -1398,9 +1424,9 @@ void CConnman::SocketHandler() LOCK(pnode->cs_hSocket); if (pnode->hSocket == INVALID_SOCKET) continue; - recvSet = FD_ISSET(pnode->hSocket, &fdsetRecv); - sendSet = FD_ISSET(pnode->hSocket, &fdsetSend); - errorSet = FD_ISSET(pnode->hSocket, &fdsetError); + recvSet = recv_set.count(pnode->hSocket) > 0; + sendSet = send_set.count(pnode->hSocket) > 0; + errorSet = error_set.count(pnode->hSocket) > 0; } if (recvSet || errorSet) { diff --git a/src/net.h b/src/net.h index 27121ab213..eff7198111 100644 --- a/src/net.h +++ b/src/net.h @@ -343,6 +343,7 @@ private: void NotifyNumConnectionsChanged(); void InactivityCheck(CNode *pnode); bool GenerateSelectSet(std::set &recv_set, std::set &send_set, std::set &error_set); + void SocketEvents(std::set &recv_set, std::set &send_set, std::set &error_set); void SocketHandler(); void ThreadSocketHandler(); void ThreadDNSAddressSeed(); From 11cc491a288a73e911be24a285e12abd57df7d04 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Wed, 26 Sep 2018 21:54:52 -0400 Subject: [PATCH 4/5] Implement poll() on systems which support it properly. This eliminates the restriction on maximum socket descriptor number. --- src/compat.h | 9 ++++++++- src/net.cpp | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ src/netbase.cpp | 21 ++++++++++++++++++++- 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/compat.h b/src/compat.h index 049579c365..7b164d5630 100644 --- a/src/compat.h +++ b/src/compat.h @@ -92,8 +92,15 @@ typedef void* sockopt_arg_type; typedef char* sockopt_arg_type; #endif +// Note these both should work with the current usage of poll, but best to be safe +// WIN32 poll is broken https://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/ +// __APPLE__ poll is broke https://github.com/bitcoin/bitcoin/pull/14336#issuecomment-437384408 +#if defined(__linux__) +#define USE_POLL +#endif + bool static inline IsSelectableSocket(const SOCKET& s) { -#ifdef WIN32 +#if defined(USE_POLL) || defined(WIN32) return true; #else return (s < FD_SETSIZE); diff --git a/src/net.cpp b/src/net.cpp index 2664828034..7b8b6e5ea2 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -26,6 +26,10 @@ #include #endif +#ifdef USE_POLL +#include +#endif + #ifdef USE_UPNP #include #include @@ -33,6 +37,7 @@ #include #endif +#include #include @@ -1307,6 +1312,49 @@ bool CConnman::GenerateSelectSet(std::set &recv_set, std::set &s return !recv_set.empty() || !send_set.empty() || !error_set.empty(); } +#ifdef USE_POLL +void CConnman::SocketEvents(std::set &recv_set, std::set &send_set, std::set &error_set) +{ + std::set recv_select_set, send_select_set, error_select_set; + if (!GenerateSelectSet(recv_select_set, send_select_set, error_select_set)) { + interruptNet.sleep_for(std::chrono::milliseconds(SELECT_TIMEOUT_MILLISECONDS)); + return; + } + + std::unordered_map pollfds; + for (SOCKET socket_id : recv_select_set) { + pollfds[socket_id].fd = socket_id; + pollfds[socket_id].events |= POLLIN; + } + + for (SOCKET socket_id : send_select_set) { + pollfds[socket_id].fd = socket_id; + pollfds[socket_id].events |= POLLOUT; + } + + for (SOCKET socket_id : error_select_set) { + pollfds[socket_id].fd = socket_id; + // These flags are ignored, but we set them for clarity + pollfds[socket_id].events |= POLLERR|POLLHUP; + } + + std::vector vpollfds; + vpollfds.reserve(pollfds.size()); + for (auto it : pollfds) { + vpollfds.push_back(std::move(it.second)); + } + + if (poll(vpollfds.data(), vpollfds.size(), SELECT_TIMEOUT_MILLISECONDS) < 0) return; + + if (interruptNet) return; + + for (struct pollfd pollfd_entry : vpollfds) { + if (pollfd_entry.revents & POLLIN) recv_set.insert(pollfd_entry.fd); + if (pollfd_entry.revents & POLLOUT) send_set.insert(pollfd_entry.fd); + if (pollfd_entry.revents & (POLLERR|POLLHUP)) error_set.insert(pollfd_entry.fd); + } +} +#else void CConnman::SocketEvents(std::set &recv_set, std::set &send_set, std::set &error_set) { std::set recv_select_set, send_select_set, error_select_set; @@ -1380,6 +1428,7 @@ void CConnman::SocketEvents(std::set &recv_set, std::set &send_s } } } +#endif void CConnman::SocketHandler() { diff --git a/src/netbase.cpp b/src/netbase.cpp index 1c043fc981..355e21d4e6 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -21,6 +21,10 @@ #include #endif +#ifdef USE_POLL +#include +#endif + #if !defined(MSG_NOSIGNAL) #define MSG_NOSIGNAL 0 #endif @@ -264,11 +268,19 @@ static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, int timeout, c if (!IsSelectableSocket(hSocket)) { return IntrRecvError::NetworkError; } - struct timeval tval = MillisToTimeval(std::min(endTime - curTime, maxWait)); + int timeout_ms = std::min(endTime - curTime, maxWait); +#ifdef USE_POLL + struct pollfd pollfd = {}; + pollfd.fd = hSocket; + pollfd.events = POLLIN | POLLOUT; + int nRet = poll(&pollfd, 1, timeout_ms); +#else + struct timeval tval = MillisToTimeval(timeout_ms); fd_set fdset; FD_ZERO(&fdset); FD_SET(hSocket, &fdset); int nRet = select(hSocket + 1, &fdset, nullptr, nullptr, &tval); +#endif if (nRet == SOCKET_ERROR) { return IntrRecvError::NetworkError; } @@ -499,11 +511,18 @@ bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, i // WSAEINVAL is here because some legacy version of winsock uses it if (nErr == WSAEINPROGRESS || nErr == WSAEWOULDBLOCK || nErr == WSAEINVAL) { +#ifdef USE_POLL + struct pollfd pollfd = {}; + pollfd.fd = hSocket; + pollfd.events = POLLIN | POLLOUT; + int nRet = poll(&pollfd, 1, nTimeout); +#else struct timeval timeout = MillisToTimeval(nTimeout); fd_set fdset; FD_ZERO(&fdset); FD_SET(hSocket, &fdset); int nRet = select(hSocket + 1, nullptr, &fdset, nullptr, &timeout); +#endif if (nRet == 0) { LogPrint(BCLog::NET, "connection to %s timeout\n", addrConnect.ToString()); From 4927bf2f257ac53569978980eaf1f61c2c6b04cc Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 13 Nov 2018 14:09:05 -0500 Subject: [PATCH 5/5] Increase maxconnections limit when using poll. --- src/init.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 31212a355b..6a49fdbc62 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -952,8 +952,13 @@ bool AppInitParameterInteraction() // Trim requested connection counts, to fit into system limitations // in std::min(...) to work around FreeBSD compilation issue described in #2695 - nMaxConnections = std::max(std::min(nMaxConnections, FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS), 0); nFD = RaiseFileDescriptorLimit(nMaxConnections + MIN_CORE_FILEDESCRIPTORS + MAX_ADDNODE_CONNECTIONS); +#ifdef USE_POLL + int fd_max = nFD; +#else + int fd_max = FD_SETSIZE; +#endif + nMaxConnections = std::max(std::min(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS), 0); if (nFD < MIN_CORE_FILEDESCRIPTORS) return InitError(_("Not enough file descriptors available.")); nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS, nMaxConnections);