From: Craig Gowing Date: Thu, 27 Sep 2018 08:21:27 +0000 (+0000) Subject: Bug 4885: Excessive memory usage when running out of descriptors (#291) X-Git-Tag: M-staged-PR293~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f3b976f73b82841915ecd3e6d35ebc9048be4adb;p=thirdparty%2Fsquid.git Bug 4885: Excessive memory usage when running out of descriptors (#291) TcpAcceptor now stops listening when it cannot accept due to FD limits. We also no longer defer/queue the same limited TcpAcceptor multiple times. These changes prevent unbounded memory growth and improve performance of Squids running out of file descriptors. They should have no impact on other Squids. --- diff --git a/src/comm/AcceptLimiter.cc b/src/comm/AcceptLimiter.cc index 26412fc1da..9e0845e293 100644 --- a/src/comm/AcceptLimiter.cc +++ b/src/comm/AcceptLimiter.cc @@ -24,42 +24,33 @@ Comm::AcceptLimiter::Instance() void Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd) { - ++ (afd->isLimited); - debugs(5, 5, afd->conn << " x" << afd->isLimited); + debugs(5, 5, afd->conn << "; already queued: " << deferred_.size()); deferred_.push_back(afd); } void Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd) { - uint64_t abandonedClients = 0; - for (unsigned int i = 0; i < deferred_.size() && afd->isLimited > 0; ++i) { - if (deferred_[i] == afd) { - -- deferred_[i]->isLimited; - deferred_[i] = NULL; // fast. kick() will skip empty entries later. - debugs(5, 5, afd->conn << " x" << afd->isLimited); - ++abandonedClients; + for (auto it = deferred_.begin(); it != deferred_.end(); ++it) { + if (*it == afd) { + *it = nullptr; // fast. kick() will skip empty entries later. + debugs(5,4, "Abandoned client TCP SYN by closing socket: " << afd->conn); + return; } } - debugs(5,4, "Abandoned " << abandonedClients << " client TCP SYN by closing socket: " << afd->conn); + debugs(5,4, "Not found " << afd->conn << " in queue, size: " << deferred_.size()); } void Comm::AcceptLimiter::kick() { - // TODO: this could be optimized further with an iterator to search - // looking for first non-NULL, followed by dumping the first N - // with only one shift()/pop_front operation - // OR, by reimplementing as a list instead of Vector. - debugs(5, 5, "size=" << deferred_.size()); - while (deferred_.size() > 0 && fdNFree() >= RESERVED_FD) { + while (deferred_.size() > 0 && Comm::TcpAcceptor::okToAccept()) { /* NP: shift() is equivalent to pop_front(). Giving us a FIFO queue. */ TcpAcceptor::Pointer temp = deferred_.front(); deferred_.erase(deferred_.begin()); if (temp.valid()) { debugs(5, 5, "doing one."); - -- temp->isLimited; temp->acceptNext(); break; } diff --git a/src/comm/AcceptLimiter.h b/src/comm/AcceptLimiter.h index f0b9e9eca9..b94ae308ec 100644 --- a/src/comm/AcceptLimiter.h +++ b/src/comm/AcceptLimiter.h @@ -11,7 +11,7 @@ #include "comm/TcpAcceptor.h" -#include +#include namespace Comm { @@ -26,16 +26,6 @@ namespace Comm * removeDead - used only by Comm layer ConnAcceptor to remove themselves when dying. * kick - used by Comm layer when FD are closed. */ -/* TODO this algorithm can be optimized further: - * - * 1) reduce overheads by only pushing one entry per port to the list? - * use TcpAcceptor::isLimited as a flag whether to re-list when kick()'ing - * or to NULL an entry while scanning the list for empty spaces. - * Side effect: TcpAcceptor->kick() becomes allowed to pull off multiple accept()'s in bunches - * - * 2) re-implement as a std::queue instead of std::vector - * storing head/tail pointers for fast push/pop and avoiding the whole shift() overhead - */ class AcceptLimiter { @@ -56,7 +46,7 @@ private: static AcceptLimiter Instance_; /** FIFO queue */ - std::vector deferred_; + std::deque deferred_; }; }; // namepace Comm diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc index ca009ff6d3..7309505413 100644 --- a/src/comm/TcpAcceptor.cc +++ b/src/comm/TcpAcceptor.cc @@ -43,7 +43,6 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, TcpAcceptor); Comm::TcpAcceptor::TcpAcceptor(const Comm::ConnectionPointer &newConn, const char *, const Subscription::Pointer &aSub) : AsyncJob("Comm::TcpAcceptor"), errcode(0), - isLimited(0), theCallSub(aSub), conn(newConn), listenPort_() @@ -52,7 +51,6 @@ Comm::TcpAcceptor::TcpAcceptor(const Comm::ConnectionPointer &newConn, const cha Comm::TcpAcceptor::TcpAcceptor(const AnyP::PortCfgPointer &p, const char *, const Subscription::Pointer &aSub) : AsyncJob("Comm::TcpAcceptor"), errcode(0), - isLimited(0), theCallSub(aSub), conn(p->listenConn), listenPort_(p) @@ -233,7 +231,6 @@ Comm::TcpAcceptor::doAccept(int fd, void *data) } else { afd->acceptNext(); } - SetSelect(fd, COMM_SELECT_READ, Comm::TcpAcceptor::doAccept, afd, 0); } catch (const std::exception &e) { fatalf("FATAL: error while accepting new client connection: %s\n", e.what()); @@ -310,6 +307,7 @@ Comm::TcpAcceptor::acceptOne() " accepted new connection " << newConnDetails << " handler Subscription: " << theCallSub); notify(flag, newConnDetails); + SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0); } void diff --git a/src/comm/TcpAcceptor.h b/src/comm/TcpAcceptor.h index 643773136e..0c77b0df4f 100644 --- a/src/comm/TcpAcceptor.h +++ b/src/comm/TcpAcceptor.h @@ -76,9 +76,12 @@ public: /// errno code of the last accept() or listen() action if one occurred. int errcode; + /// Method to test if there are enough file descriptors to open a new client connection + /// if not the accept() will be postponed + static bool okToAccept(); + protected: friend class AcceptLimiter; - int32_t isLimited; ///< whether this socket is delayed and on the AcceptLimiter queue. private: Subscription::Pointer theCallSub; ///< used to generate AsyncCalls handling our events. @@ -93,10 +96,6 @@ private: /// listen socket closure handler AsyncCall::Pointer closer_; - /// Method to test if there are enough file descriptors to open a new client connection - /// if not the accept() will be postponed - static bool okToAccept(); - /// Method callback for whenever an FD is ready to accept a client connection. static void doAccept(int fd, void *data);