From: Amos Jeffries Date: Fri, 17 Jun 2011 02:14:01 +0000 (+1200) Subject: Upgrade ICAP persistent connection handling X-Git-Tag: take08~55^2~130 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f915be8d04d0ea6009e388b2b4a9f49be34e3ba3;p=thirdparty%2Fsquid.git Upgrade ICAP persistent connection handling ICAP services use a "service" model of pconn different from the "TCP destination" model which PconnPool objects are designed for. This patch alters Adaptation::Icap::ServiceRep to use the simpler IdleConnList object for pconn storage. IdleConnList stores a "set of idle connections" more compatible with the ICAP model. In order to implement ICAP max-connections feature the closeN() operation is added to IdleConnList. The result is removal of the complex hash and management operations on push/pop of the idle conn set. The only expected behaviour change is more frequent re-use of idle connections on services with multiple IP addresses. Speed gains are minimal, but positive. --- diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index b5d17addb4..1310d9dec9 100644 --- a/src/adaptation/icap/ServiceRep.cc +++ b/src/adaptation/icap/ServiceRep.cc @@ -24,7 +24,7 @@ Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg): theBusyConns(0), theAllWaiters(0), connOverloadReported(false), - theIdleConns("ICAP Service"), + theIdleConns("ICAP Service",NULL), isSuspended(0), notifying(false), updateScheduled(false), wasAnnouncedUp(true), // do not announce an "up" service at startup @@ -85,8 +85,26 @@ int Adaptation::Icap::ServiceRep::getConnection(bool retriableXact, bool &reused { Ip::Address client_addr; - int connection = theIdleConns.pop(cfg().host.termedBuf(), cfg().port, NULL, client_addr, - retriableXact); + int connection = -1; + + /* 2011-06-17: rousskov: + * There are two things that happen at the same time in pop(). Both are important. + * 1) Ensure that we can use a pconn for this transaction. + * 2) Ensure that the number of idle pconns does not grow without bounds. + * + * Both happen in the beginning of the transaction. Both are dictated by real-world problems. + * retriable means you can repeat the request if you suspect the first try failed due to a pconn race. + * HTTP and ICAP rules prohibit the use of pconns for non-retriable requests. + * + * If there are zero idle connections, (2) is irrelevant. (2) is only relevant when there are many + * idle connections and we should not open more connections without closing some idle ones, + * or instead of just opening a new connection and leaving idle connections as is. + * In other words, (2) tells us to close one FD for each new one we open due to retriable. + */ + if (retriableXact) + connection = theIdleConns.findUseableFD(); + else + theIdleConns.closeN(1); reused = connection >= 0; // reused a persistent connection @@ -116,7 +134,7 @@ void Adaptation::Icap::ServiceRep::putConnection(int fd, bool isReusable, const debugs(93, 3, HERE << "pushing pconn" << comment); commSetTimeout(fd, -1, NULL, NULL); Ip::Address anyAddr; - theIdleConns.push(fd, cfg().host.termedBuf(), cfg().port, NULL, anyAddr); + theIdleConns.push(fd); } else { debugs(93, 3, HERE << "closing pconn" << comment); // comm_close will clear timeout @@ -133,7 +151,7 @@ void Adaptation::Icap::ServiceRep::putConnection(int fd, bool isReusable, const void Adaptation::Icap::ServiceRep::noteConnectionUse(int fd) { Must(fd >= 0); - fd_table[fd].noteUse(&theIdleConns); + fd_table[fd].noteUse(NULL); // pconn re-use but not via PconnPool API } void Adaptation::Icap::ServiceRep::setMaxConnections() @@ -554,8 +572,7 @@ void Adaptation::Icap::ServiceRep::handleNewOptions(Adaptation::Icap::Options *n if (excess && theIdleConns.count() > 0) { const int n = min(excess, theIdleConns.count()); debugs(93,5, HERE << "closing " << n << " pconns to relief debt"); - Ip::Address anyAddr; - theIdleConns.closeN(n, cfg().host.termedBuf(), cfg().port, NULL, anyAddr); + theIdleConns.closeN(n); } scheduleNotification(); diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h index 27caf2f24b..a1ca2d1025 100644 --- a/src/adaptation/icap/ServiceRep.h +++ b/src/adaptation/icap/ServiceRep.h @@ -160,7 +160,7 @@ private: int theMaxConnections; ///< the maximum allowed connections to the service // TODO: use a better type like the FadingCounter for connOverloadReported mutable bool connOverloadReported; ///< whether we reported exceeding theMaxConnections - PconnPool theIdleConns; ///< idle persistent connection pool + IdleConnList theIdleConns; ///< idle persistent connection pool FadingCounter theSessionFailures; const char *isSuspended; // also stores suspension reason for debugging diff --git a/src/pconn.cc b/src/pconn.cc index be9fe30c3c..02811e6d9d 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -57,8 +57,8 @@ IdleConnList::IdleConnList(const char *key, PconnPool *thePool) : parent(thePool IdleConnList::~IdleConnList() { - - parent->unlinkList(this); + if (parent) + parent->unlinkList(this); if (nfds_alloc == PCONN_FDS_SZ) pconn_fds_pool->freeOne(fds); @@ -97,7 +97,53 @@ IdleConnList::removeFD(int fd) if (parent) parent->noteConnectionRemoved(); - if (--nfds == 0) { + if (parent && --nfds == 0) { + debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash)); + delete this; + } +} + +// almost a duplicate of removeFD. But drops multiple entries. +void +IdleConnList::closeN(size_t n) +{ + if (n < 1) { + debugs(48, 2, HERE << "Nothing to do."); + return; + } else if (n < (size_t)count()) { + debugs(48, 2, HERE << "Closing all entries."); + while (nfds >= 0) { + int fd = fds[--nfds]; + fds[nfds] = -1; + clearHandlers(fd); + comm_close(fd); + if (parent) + parent->noteConnectionRemoved(); + } + } else { + debugs(48, 2, HERE << "Closing " << n << " of " << nfds << " entries."); + + size_t index = 0; + // ensure the first N entries are closed + while (index < n) { + int fd = fds[--nfds]; + fds[nfds] = -1; + clearHandlers(fd); + comm_close(fd); + if (parent) + parent->noteConnectionRemoved(); + } + // shuffle the list N down. + for (;index < (size_t)nfds; index++) { + fds[index - n] = fds[index]; + } + // ensure the last N entries are unset + while (index < ((size_t)nfds) + n) { + fds[index] = -1; + } + } + + if (parent && nfds == 0) { debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash)); delete this; } diff --git a/src/pconn.h b/src/pconn.h index 6eada4fdbd..869dd3b63a 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -35,6 +35,7 @@ public: int findFDIndex(int fd); ///< search from the end of array void removeFD(int fd); + void closeN(size_t count); void push(int fd); int findUseableFD(); ///< find first from the end not pending read fd. void clearHandlers(int fd);