From: Amos Jeffries Date: Sat, 2 Oct 2010 15:28:20 +0000 (+1300) Subject: Transition pconn handling from FD to Comm::Connection X-Git-Tag: take08~55^2~124^2~52 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=642a305c2b7cce1a68f778ef7db9f5948489d3b6;p=thirdparty%2Fsquid.git Transition pconn handling from FD to Comm::Connection --- diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 7a286872eb..757e9b07b9 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -86,10 +86,9 @@ void Adaptation::Icap::Xaction::start() } // TODO: obey service-specific, OPTIONS-reported connection limit -void Adaptation::Icap::Xaction::openConnection() +void +Adaptation::Icap::Xaction::openConnection() { - Ip::Address client_addr; - Must(!haveConnection()); const Adaptation::Service &s = service(); @@ -106,16 +105,16 @@ void Adaptation::Icap::Xaction::openConnection() connection->remote.SetPort(s.cfg().port); // TODO: check whether NULL domain is appropriate here - connection->fd = icapPconnPool->pop(s.cfg().host.termedBuf(), s.cfg().port, NULL, client_addr, isRetriable); + icapPconnPool->pop(connection, NULL, isRetriable); if (connection->isOpen()) { - debugs(93,3, HERE << "reused pconn FD " << connection->fd); + debugs(93,3, HERE << "reused pconn " << connection); // fake the connect callback // TODO: can we sync call Adaptation::Icap::Xaction::noteCommConnected here instead? typedef CommCbMemFunT Dialer; CbcPointer self(this); Dialer dialer(self, &Adaptation::Icap::Xaction::noteCommConnected); - dialer.params.fd = connection->fd; + dialer.params.conn = connection; dialer.params.flag = COMM_OK; // fake other parameters by copying from the existing connection connector = asyncCall(93,3, "Adaptation::Icap::Xaction::noteCommConnected", dialer); @@ -164,19 +163,15 @@ void Adaptation::Icap::Xaction::closeConnection() } if (reuseConnection) { - Ip::Address client_addr; //status() adds leading spaces. debugs(93,3, HERE << "pushing pconn" << status()); - AsyncCall::Pointer call = NULL; - commSetTimeout(connection->fd, -1, call); - icapPconnPool->push(connection->fd, theService->cfg().host.termedBuf(), - theService->cfg().port, NULL, client_addr); + AsyncCall::Pointer nul; + commSetTimeout(connection->fd, -1, nul); + icapPconnPool->push(connection, NULL); disableRetries(); - connection->fd = -1; // prevent premature real closing. } else { //status() adds leading spaces. debugs(93,3, HERE << "closing pconn" << status()); - // comm_close will clear timeout connection->close(); } diff --git a/src/forward.cc b/src/forward.cc index 636f96c263..9e8f956db9 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -809,32 +809,23 @@ FwdState::connectStart() return; } -// TODO: now that we are dealing with actual IP->IP links. should we still anchor pconn on hostname? -// or on the remote IP+port? -// that could reduce the pconns per virtual server a fair amount -// but would prevent crossover between servers hosting the one domain -// this currently opens the possibility that conn will lie about where the FD goes. - + // Use pconn to avoid opening a new connection. const char *host; int port; if (serverDestinations[0]->getPeer()) { host = serverDestinations[0]->getPeer()->host; port = serverDestinations[0]->getPeer()->http_port; - serverDestinations[0]->fd = fwdPconnPool->pop(serverDestinations[0]->getPeer()->name, - serverDestinations[0]->getPeer()->http_port, - request->GetHost(), serverDestinations[0]->local, - checkRetriable()); } else { host = request->GetHost(); port = request->port; - serverDestinations[0]->fd = fwdPconnPool->pop(host, port, NULL, serverDestinations[0]->local, checkRetriable()); } serverDestinations[0]->remote.SetPort(port); + fwdPconnPool->pop(serverDestinations[0], host, checkRetriable()); // if we found an open persistent connection to use. use it. if (Comm::IsConnOpen(serverDestinations[0])) { serverConn = serverDestinations[0]; - debugs(17, 3, HERE << "reusing pconn FD " << serverConnection()->fd); + debugs(17, 3, HERE << "reusing pconn " << serverConnection()); n_tries++; if (!serverConnection()->getPeer()) @@ -1095,27 +1086,17 @@ FwdState::reforwardableStatus(http_status s) /** * Decide where details need to be gathered to correctly describe a persistent connection. * What is needed: - * - host name of server at other end of this link (either peer or requested host) - * - port to which we connected the other end of this link (for peer or request) - * - domain for which the connection is supposed to be used - * - address of the client for which we made the connection + * - the address/port details about this link + * - domain name of server at other end of this link (either peer or requested host) */ void -FwdState::pconnPush(Comm::ConnectionPointer &conn, const peer *_peer, const HttpRequest *req, const char *domain, Ip::Address &client_addr) +FwdState::pconnPush(Comm::ConnectionPointer &conn, const char *domain) { - if (_peer) { - fwdPconnPool->push(conn->fd, _peer->name, _peer->http_port, domain, client_addr); + if (conn->getPeer()) { + fwdPconnPool->push(conn, conn->getPeer()->name); } else { - /* small performance improvement, using NULL for domain instead of listing it twice */ - /* although this will leave a gap open for url-rewritten domains to share a link */ - fwdPconnPool->push(conn->fd, req->GetHost(), req->port, NULL, client_addr); + fwdPconnPool->push(conn, domain); } - - /* XXX: remove this when Comm::Connection are stored in the pool - * this only prevents the persistent FD being closed when the - * Comm::Connection currently using it is destroyed. - */ - conn->fd = -1; } void diff --git a/src/forward.h b/src/forward.h index c2cc7c9ce6..7cef702f79 100644 --- a/src/forward.h +++ b/src/forward.h @@ -37,7 +37,7 @@ public: bool checkRetry(); bool checkRetriable(); void dispatch(); - void pconnPush(Comm::ConnectionPointer & conn, const peer *_peer, const HttpRequest *req, const char *domain, Ip::Address &client_addr); + void pconnPush(Comm::ConnectionPointer & conn, const char *domain); bool dontRetry() { return flags.dont_retry; } diff --git a/src/http.cc b/src/http.cc index 3a1b8b11aa..08d676e848 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1431,7 +1431,7 @@ HttpStateData::processReplyBody() orig_request->pinnedConnection()->pinConnection(serverConnection->fd, orig_request, _peer, (request->flags.connection_auth != 0)); } else { - fwd->pconnPush(serverConnection, _peer, request, orig_request->GetHost(), client_addr); + fwd->pconnPush(serverConnection, request->GetHost()); } serverConnection = NULL; diff --git a/src/pconn.cc b/src/pconn.cc index 21b91e6121..e49136d568 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -42,40 +42,41 @@ #define PCONN_FDS_SZ 8 /* pconn set size, increase for better memcache hit rate */ -static MemAllocator *pconn_fds_pool = NULL; +//TODO: re-attach to MemPools. WAS: static MemAllocator *pconn_fds_pool = NULL; PconnModule * PconnModule::instance = NULL; CBDATA_CLASS_INIT(IdleConnList); /* ========== IdleConnList ============================================ */ -IdleConnList::IdleConnList(const char *key, PconnPool *thePool) : parent(thePool) +IdleConnList::IdleConnList(const char *key, PconnPool *thePool) : + nfds_alloc(PCONN_FDS_SZ), + nfds(0), + parent(thePool) { hash.key = xstrdup(key); - nfds_alloc = PCONN_FDS_SZ; - nfds = 0; - fds = (int *)pconn_fds_pool->alloc(); + theList = new Comm::ConnectionPointer[nfds_alloc]; +// TODO: re-attach to MemPools. WAS: fds = (int *)pconn_fds_pool->alloc(); } IdleConnList::~IdleConnList() { - parent->unlinkList(this); +/* TODO: re-attach to MemPools. if (nfds_alloc == PCONN_FDS_SZ) - pconn_fds_pool->freeOne(fds); + pconn_fds_pool->freeOne(theList); else - xfree(fds); +*/ + delete[] theList; xfree(hash.key); } int -IdleConnList::findFDIndex(int fd) +IdleConnList::findIndex(const Comm::ConnectionPointer &conn) { - int index; - - for (index = nfds - 1; index >= 0; --index) { - if (fds[index] == fd) + for (int index = nfds - 1; index >= 0; --index) { + if (theList[index]->fd == conn->fd) return index; } @@ -83,17 +84,17 @@ IdleConnList::findFDIndex(int fd) } bool -IdleConnList::removeFD(int fd) +IdleConnList::remove(const Comm::ConnectionPointer &conn) { - int index = findFDIndex(fd); + int index = findIndex(conn); if (index < 0) { - debugs(48, 2, "IdleConnList::removeFD: FD " << fd << " NOT FOUND!"); + debugs(48, 2, HERE << conn << " NOT FOUND!"); return false; } - debugs(48, 3, "IdleConnList::removeFD: found FD " << fd << " at index " << index); + debugs(48, 3, HERE << "found " << conn << " at index " << index); for (; index < nfds - 1; index++) - fds[index] = fds[index + 1]; + theList[index] = theList[index + 1]; if (--nfds == 0) { debugs(48, 3, "IdleConnList::removeFD: deleting " << hashKeyStr(&hash)); @@ -103,33 +104,34 @@ IdleConnList::removeFD(int fd) } void -IdleConnList::clearHandlers(int fd) +IdleConnList::clearHandlers(const Comm::ConnectionPointer &conn) { - comm_read_cancel(fd, IdleConnList::read, this); - commSetTimeout(fd, -1, NULL, NULL); + comm_read_cancel(conn->fd, IdleConnList::read, this); + commSetTimeout(conn->fd, -1, NULL, NULL); } void -IdleConnList::push(int fd) +IdleConnList::push(const Comm::ConnectionPointer &conn) { if (nfds == nfds_alloc) { debugs(48, 3, "IdleConnList::push: growing FD array"); nfds_alloc <<= 1; - int *old = fds; - fds = (int *)xmalloc(nfds_alloc * sizeof(int)); - xmemcpy(fds, old, nfds * sizeof(int)); + const Comm::ConnectionPointer *oldList = theList; + theList = new Comm::ConnectionPointer[nfds_alloc]; + for (int index = 0; index < nfds; index++) + theList[index] = oldList[index]; +/* TODO: re-attach to MemPools. if (nfds == PCONN_FDS_SZ) - pconn_fds_pool->freeOne(old); + pconn_fds_pool->freeOne(oldList); else - xfree(old); +*/ + delete[] oldList; } - fds[nfds++] = fd; - Comm::ConnectionPointer temp = new Comm::Connection; // XXX: transition. until pconn's converted to store Comm::Connection - temp->fd = fd; // assume control of the fd. - comm_read(temp, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::read, this); - commSetTimeout(temp->fd, Config.Timeout.pconn, IdleConnList::timeout, this); + theList[nfds++] = conn; + comm_read(conn, fakeReadBuf, sizeof(fakeReadBuf), IdleConnList::read, this); + commSetTimeout(conn->fd, Config.Timeout.pconn, IdleConnList::timeout, this); } /* @@ -140,24 +142,24 @@ IdleConnList::push(int fd) * of requests JUST as they timeout (say, it shuts down) we'll be wasting * quite a bit of CPU. Just keep it in mind. */ -int -IdleConnList::findUseableFD() +Comm::ConnectionPointer +IdleConnList::findUseable() { assert(nfds); for (int i=nfds-1; i>=0; i--) { - if (!comm_has_pending_read_callback(fds[i])) { - return fds[i]; + if (!comm_has_pending_read_callback(theList[i]->fd)) { + return theList[i]; } } - return -1; + return Comm::ConnectionPointer(); } void IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data) { - debugs(48, 3, "IdleConnList::read: " << len << " bytes from " << conn); + debugs(48, 3, HERE << len << " bytes from " << conn); if (flag == COMM_ERR_CLOSING) { /* Bail out early on COMM_ERR_CLOSING - close handlers will tidy up for us */ @@ -165,7 +167,8 @@ IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len, c } IdleConnList *list = (IdleConnList *) data; - if (list && list->removeFD(conn->fd)) { /* might delete list */ + /* might delete list */ + if (list && list->remove(conn)) { Comm::ConnectionPointer nonConst = conn; nonConst->close(); } @@ -176,35 +179,34 @@ IdleConnList::timeout(int fd, void *data) { debugs(48, 3, "IdleConnList::timeout: FD " << fd); IdleConnList *list = (IdleConnList *) data; - if (list->removeFD(fd)) /* might delete list */ - comm_close(fd); + Comm::ConnectionPointer temp = new Comm::Connection; // XXX: transition. make timeouts pass conn in + temp->fd = fd; + if (list->remove(temp)) { + temp->close(); + } else + temp->fd = -1; // XXX: transition. prevent temp erasure double-closing FD until timeout CB passess conn in. } /* ========== PconnPool PRIVATE FUNCTIONS ============================================ */ const char * -PconnPool::key(const char *host, u_short port, const char *domain, Ip::Address &client_address) +PconnPool::key(const Comm::ConnectionPointer &destLink, const char *domain) { LOCAL_ARRAY(char, buf, SQUIDHOSTNAMELEN * 3 + 10); - char ntoabuf[MAX_IPSTRLEN]; - - if (domain && !client_address.IsAnyAddr()) - snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s/%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN), domain); - else if (domain && client_address.IsAnyAddr()) - snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d/%s", host, (int) port, domain); - else if ((!domain) && !client_address.IsAnyAddr()) - snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d-%s", host, (int) port, client_address.NtoA(ntoabuf,MAX_IPSTRLEN)); - else - snprintf(buf, SQUIDHOSTNAMELEN * 3 + 10, "%s:%d", host, (int) port); - debugs(48,6,"PconnPool::key(" << (host?host:"(no host!)") << "," << port << "," << (domain?domain:"(no domain)") << "," << client_address << "is {" << buf << "}" ); + destLink->remote.ToURL(buf, SQUIDHOSTNAMELEN * 3 + 10); + if (domain) { + int used = strlen(buf); + snprintf(buf+used, SQUIDHOSTNAMELEN * 3 + 10-used, "/%s", domain); + } + + debugs(48,6,"PconnPool::key(" << destLink << ", " << (domain?domain:"[no domain]") << ") is {" << buf << "}" ); return buf; } void -PconnPool::dumpHist(StoreEntry * e) +PconnPool::dumpHist(StoreEntry * e) const { - int i; storeAppendPrintf(e, "%s persistent connection counts:\n" "\n" @@ -213,7 +215,7 @@ PconnPool::dumpHist(StoreEntry * e) "\t---- ---------\n", descr); - for (i = 0; i < PCONN_HIST_SZ; i++) { + for (int i = 0; i < PCONN_HIST_SZ; i++) { if (hist[i] == 0) continue; @@ -222,14 +224,13 @@ PconnPool::dumpHist(StoreEntry * e) } void -PconnPool::dumpHash(StoreEntry *e) +PconnPool::dumpHash(StoreEntry *e) const { - int i; - hash_link *walker = NULL; hash_table *hid = table; hash_first(hid); - for (i = 0, walker = hid->next; walker; walker = hash_next(hid)) { + int i = 0; + for (hash_link *walker = hid->next; walker; walker = hash_next(hid)) { storeAppendPrintf(e, "\t item %5d: %s\n", i++, (char *)(walker->key)); } } @@ -238,10 +239,9 @@ PconnPool::dumpHash(StoreEntry *e) PconnPool::PconnPool(const char *aDescr) : table(NULL), descr(aDescr) { - int i; table = hash_create((HASHCMP *) strcmp, 229, hash_string); - for (i = 0; i < PCONN_HIST_SZ; i++) + for (int i = 0; i < PCONN_HIST_SZ; i++) hist[i] = 0; PconnModule::GetInstance()->add(this); @@ -254,25 +254,22 @@ PconnPool::~PconnPool() } void -PconnPool::push(int fd, const char *host, u_short port, const char *domain, Ip::Address &client_address) +PconnPool::push(const Comm::ConnectionPointer &conn, const char *domain) { - IdleConnList *list; - const char *aKey; - LOCAL_ARRAY(char, desc, FD_DESC_SZ); - if (fdUsageHigh()) { debugs(48, 3, "PconnPool::push: Not many unused FDs"); - comm_close(fd); + Comm::ConnectionPointer nonConst = conn; + nonConst->close(); return; } else if (shutting_down) { - comm_close(fd); + Comm::ConnectionPointer nonConst = conn; + nonConst->close(); debugs(48, 3, "PconnPool::push: Squid is shutting down. Refusing to do anything"); return; } - aKey = key(host, port, domain, client_address); - - list = (IdleConnList *) hash_lookup(table, aKey); + const char *aKey = key(conn, domain); + IdleConnList *list = (IdleConnList *) hash_lookup(table, aKey); if (list == NULL) { list = new IdleConnList(aKey, this); @@ -282,48 +279,41 @@ PconnPool::push(int fd, const char *host, u_short port, const char *domain, Ip:: debugs(48, 3, "PconnPool::push: found IdleConnList for {" << hashKeyStr(&list->hash) << "}" ); } - list->push(fd); + list->push(conn); + assert(!comm_has_incomplete_write(conn->fd)); - assert(!comm_has_incomplete_write(fd)); - snprintf(desc, FD_DESC_SZ, "%s idle connection", host); - fd_note(fd, desc); - debugs(48, 3, "PconnPool::push: pushed FD " << fd << " for " << aKey); + LOCAL_ARRAY(char, desc, FD_DESC_SZ); + snprintf(desc, FD_DESC_SZ, "Idle: %s", aKey); + fd_note(conn->fd, desc); + debugs(48, 3, HERE << "pushed " << conn << " for " << aKey); } -/** - * Return a pconn fd for host:port if available and retriable. - * Otherwise, return -1. - * - * We close available persistent connection if the caller transaction is not - * retriable to avoid having a growing number of open connections when many - * transactions create persistent connections but are not retriable. - */ -int -PconnPool::pop(const char *host, u_short port, const char *domain, Ip::Address &client_address, bool isRetriable) +bool +PconnPool::pop(Comm::ConnectionPointer &destLink, const char *domain, bool isRetriable) { - const char * aKey = key(host, port, domain, client_address); + const char * aKey = key(destLink, domain); IdleConnList *list = (IdleConnList *)hash_lookup(table, aKey); if (list == NULL) { debugs(48, 3, "PconnPool::pop: lookup for key {" << aKey << "} failed."); - return -1; + return false; } else { debugs(48, 3, "PconnPool::pop: found " << hashKeyStr(&list->hash) << (isRetriable?"(to use)":"(to kill)") ); } - int fd = list->findUseableFD(); // search from the end. skip pending reads. + Comm::ConnectionPointer temp = list->findUseable(); // search from the end. skip pending reads. - if (fd >= 0) { - list->clearHandlers(fd); + if (Comm::IsConnOpen(temp)) { + list->clearHandlers(temp); /* might delete list */ - if (list->removeFD(fd) && !isRetriable) { - comm_close(fd); - return -1; - } + if (list->remove(temp) && !isRetriable) + temp->close(); + else + destLink = temp; } - return fd; + return true; } void @@ -350,7 +340,7 @@ PconnPool::count(int uses) PconnModule::PconnModule() : pools(NULL), poolCount(0) { pools = (PconnPool **) xcalloc(MAX_NUM_PCONN_POOLS, sizeof(*pools)); - pconn_fds_pool = memPoolCreate("pconn_fds", PCONN_FDS_SZ * sizeof(int)); +//TODO: re-link to MemPools. WAS: pconn_fds_pool = memPoolCreate("pconn_fds", PCONN_FDS_SZ * sizeof(int)); debugs(48, 0, "persistent connection module initialized"); registerWithCacheManager(); } @@ -375,8 +365,7 @@ PconnModule::registerWithCacheManager(void) void -PconnModule::add -(PconnPool *aPool) +PconnModule::add(PconnPool *aPool) { assert(poolCount < MAX_NUM_PCONN_POOLS); *(pools+poolCount) = aPool; @@ -386,9 +375,7 @@ PconnModule::add void PconnModule::dump(StoreEntry *e) { - int i; - - for (i = 0; i < poolCount; i++) { + for (int i = 0; i < poolCount; i++) { storeAppendPrintf(e, "\n Pool %d Stats\n", i); (*(pools+i))->dumpHist(e); storeAppendPrintf(e, "\n Pool %d Hash Table\n",i); diff --git a/src/pconn.h b/src/pconn.h index f52f3f5b5b..0186d31bdc 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -37,16 +37,18 @@ public: ~IdleConnList(); int numIdle() { return nfds; } - int findFDIndex(int fd); ///< search from the end of array + int findIndex(const Comm::ConnectionPointer &conn); ///< search from the end of array - /// If false the FD does not currently exist in the list. - /// We seem to have hit and lost a race condition. - /// Nevermind, but MUST NOT do anything with the raw FD. - bool removeFD(int fd); + /** + * \retval false The connection does not currently exist in the list. + * We seem to have hit and lost a race condition. + * Nevermind, but MUST NOT do anything with the raw FD. + */ + bool remove(const Comm::ConnectionPointer &conn); - void push(int fd); - int findUseableFD(); ///< find first from the end not pending read fd. - void clearHandlers(int fd); + void push(const Comm::ConnectionPointer &conn); + Comm::ConnectionPointer findUseable(); ///< find first from the end not pending read fd. + void clearHandlers(const Comm::ConnectionPointer &conn); private: static IOCB read; @@ -56,11 +58,11 @@ public: hash_link hash; /** must be first */ private: - int *fds; + Comm::ConnectionPointer *theList; int nfds_alloc; int nfds; PconnPool *parent; - char fakeReadBuf[4096]; + char fakeReadBuf[4096]; // TODO: kill magic number. CBDATA_CLASS2(IdleConnList); }; @@ -85,21 +87,29 @@ public: ~PconnPool(); void moduleInit(); - void push(int fd, const char *host, u_short port, const char *domain, Ip::Address &client_address); - int pop(const char *host, u_short port, const char *domain, Ip::Address &client_address, bool retriable); + void push(const Comm::ConnectionPointer &serverConn, const char *domain); + + /** + * Updates destLink to point at an existing open connection if available and retriable. + * Otherwise, return false. + * + * We close available persistent connection if the caller transaction is not + * retriable to avoid having a growing number of open connections when many + * transactions create persistent connections but are not retriable. + */ + bool pop(Comm::ConnectionPointer &serverConn, const char *domain, bool retriable); void count(int uses); - void dumpHist(StoreEntry *e); - void dumpHash(StoreEntry *e); + void dumpHist(StoreEntry *e) const; + void dumpHash(StoreEntry *e) const; void unlinkList(IdleConnList *list) const; private: - static const char *key(const char *host, u_short port, const char *domain, Ip::Address &client_address); + static const char *key(const Comm::ConnectionPointer &destLink, const char *domain); int hist[PCONN_HIST_SZ]; hash_table *table; const char *descr; - };