From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 25 Sep 2008 17:27:58 +0000 (-0600)
Subject: Performance fix: Check half-closed descriptors at most once per second.
X-Git-Tag: SQUID_3_1_0_1~49^2~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7425712611aaf8ce97414a20de145d9ffbdbe9de;p=thirdparty%2Fsquid.git

Performance fix: Check half-closed descriptors at most once per second.

A few revisions back, comm checked half-closed descriptors once per second,
but the code was buggy. I replaced it with a simpler code that checked each
half-closed descriptor whenever the OS would mark it as ready for reading.
That was a bad idea: The checks wasted a lot of CPU cycles because half-closed
descriptors are usually ready for reading all the time.

This revision resurrects 1 check/sec limit, but hopefully with fewer bugs. In
my limited tests CPU usage seems to be back to normal.

All half-closed descriptors are now stored in TheHalfClosed set. When it is
time to check the corresponding connections, Comm schedules a read for
each descriptor that is not already reading. Conflicts with regular/user
reads are resolved as before -- we silently cancel the internal half-closed
read.

TODO: It is possible that we do not need to read at all and should call
getsockopt() instead to test the connection.
---

diff --git a/src/comm.cc b/src/comm.cc
index 2cc9fc1ff8..8977a5b970 100644
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -49,6 +49,7 @@
 #include "CommCalls.h"
 #include "IPAddress.h"
 #include "IPInterception.h"
+#include "DescriptorSet.h"
 
 #if defined(_SQUID_CYGWIN_)
 #include <sys/ioctl.h>
@@ -226,6 +227,11 @@ private:
 
 /* STATIC */
 
+static DescriptorSet *TheHalfClosed = NULL; /// the set of half-closed FDs
+static bool WillCheckHalfClosed = false; /// true if check is scheduled
+static EVH commHalfClosedCheck;
+static void commPlanHalfClosedCheck();
+
 static comm_err_t commBind(int s, struct addrinfo &);
 static void commSetReuseAddr(int);
 static void commSetNoLinger(int);
@@ -347,7 +353,7 @@ comm_read(int fd, char *buf, int size, AsyncCall::Pointer &callback)
     // Active/passive conflicts are OK and simply cancel passive monitoring.
     if (ccb->active()) {
         // if the assertion below fails, we have an active comm_read conflict
-        assert(commHasHalfClosedMonitor(fd));
+        assert(fd_table[fd].halfClosedReader != NULL);
         commStopHalfClosedMonitor(fd);
         assert(!ccb->active());
     }
@@ -1591,6 +1597,9 @@ _comm_close(int fd, char const *file, int line)
     startParams.fd = fd;
     ScheduleCallHere(startCall);
 
+    // a half-closed fd may lack a reader, so we stop monitoring explicitly
+    if (commHasHalfClosedMonitor(fd))
+        commStopHalfClosedMonitor(fd);
     commSetTimeout(fd, -1, NULL, NULL);
 
     // notify read/write handlers
@@ -1928,10 +1937,15 @@ comm_init(void) {
     RESERVED_FD = XMIN(100, Squid_MaxFD / 4);
 
     conn_close_pool = memPoolCreate("close_handler", sizeof(close_handler));
+
+    TheHalfClosed = new DescriptorSet;
 }
 
 void
 comm_exit(void) {
+    delete TheHalfClosed;
+    TheHalfClosed = NULL;
+
     safe_free(fd_table);
     safe_free(fdd_table);
     if (fdc_table) {
@@ -2395,37 +2409,65 @@ AcceptLimiter::kick() {
 // will close the connection on read errors.
 void
 commStartHalfClosedMonitor(int fd) {
+    debugs(5, 5, HERE << "adding FD " << fd << " to " << *TheHalfClosed);
     assert(isOpen(fd));
     assert(!commHasHalfClosedMonitor(fd));
+    (void)TheHalfClosed->add(fd); // could also assert the result
+    commPlanHalfClosedCheck(); // may schedule check if we added the first FD
+}
+
+static
+void
+commPlanHalfClosedCheck()
+{
+    if (!WillCheckHalfClosed && !TheHalfClosed->empty()) {
+        eventAdd("commHalfClosedCheck", &commHalfClosedCheck, NULL, 1.0, 1);
+        WillCheckHalfClosed = true;
+    }
+}
+
+/// iterates over all descriptors that may need half-closed tests and
+/// calls comm_read for those that do; re-schedules the check if needed
+static
+void
+commHalfClosedCheck(void *) {
+    debugs(5, 5, HERE << "checking " << *TheHalfClosed);
+
+    typedef DescriptorSet::const_iterator DSCI;
+    const DSCI end = TheHalfClosed->end();
+    for (DSCI i = TheHalfClosed->begin(); i != end; ++i) {
+        const int fd = *i;
+        if (!fd_table[fd].halfClosedReader) { // not reading already
+            AsyncCall::Pointer call = commCbCall(5,4, "commHalfClosedReader",
+                CommIoCbPtrFun(&commHalfClosedReader, NULL));
+            comm_read(fd, NULL, 0, call);
+            fd_table[fd].halfClosedReader = call;
+        }
+    }
 
-    AsyncCall::Pointer call = commCbCall(5,4, "commHalfClosedReader",
-	CommIoCbPtrFun(&commHalfClosedReader, NULL));
-    comm_read(fd, NULL, 0, call);
+    WillCheckHalfClosed = false; // as far as we know
+    commPlanHalfClosedCheck(); // may need to check again
 }
 
 /// checks whether we are waiting for possibly half-closed connection to close
 // We are monitoring if the read handler for the fd is the monitoring handler.
 bool
 commHasHalfClosedMonitor(int fd) {
-    assert(isOpen(fd));
-
-    if (const comm_io_callback_t *cb = COMMIO_FD_READCB(fd)) {
-	AsyncCall::Pointer call = cb->callback;
-	if (call != NULL) {
-	    // check whether the callback has the right type (it should)
-	    // and uses commHalfClosedReader as the address to call back
-            typedef CommIoCbPtrFun IoDialer;
-	    if (IoDialer *d = dynamic_cast<IoDialer*>(call->getDialer()))
-	        return d->handler == &commHalfClosedReader;
-	}
-    }
-    return false;
+    return TheHalfClosed->has(fd);
 }
 
 /// stop waiting for possibly half-closed connection to close
 static void
 commStopHalfClosedMonitor(int const fd) {
-    comm_read_cancel(fd, &commHalfClosedReader, NULL);
+    debugs(5, 5, HERE << "removing FD " << fd << " from " << *TheHalfClosed);
+
+    // cancel the read if one was scheduled
+    AsyncCall::Pointer reader = fd_table[fd].halfClosedReader;
+    if (reader != NULL)
+        comm_read_cancel(fd, reader);
+    fd_table[fd].halfClosedReader = NULL;
+
+    TheHalfClosed->del(fd);
 }
 
 /// I/O handler for the possibly half-closed connection monitoring code
@@ -2433,6 +2475,9 @@ static void
 commHalfClosedReader(int fd, char *, size_t size, comm_err_t flag, int, void *) {
     // there cannot be more data coming in on half-closed connections
     assert(size == 0); 
+    assert(commHasHalfClosedMonitor(fd)); // or we would have canceled the read
+
+    fd_table[fd].halfClosedReader = NULL; // done reading, for now
 
     // nothing to do if fd is being closed
     if (flag == COMM_ERR_CLOSING)
@@ -2446,7 +2491,7 @@ commHalfClosedReader(int fd, char *, size_t size, comm_err_t flag, int, void *)
     }
 
     // continue waiting for close or error
-    commStartHalfClosedMonitor(fd);
+    commPlanHalfClosedCheck(); // make sure this fd will be checked again
 }
 
 
diff --git a/src/fde.h b/src/fde.h
index d47a0eb670..7965e9bcff 100644
--- a/src/fde.h
+++ b/src/fde.h
@@ -101,6 +101,7 @@ public:
     time_t timeout;
     void *lifetime_data;
     AsyncCall::Pointer closeHandler;
+    AsyncCall::Pointer halfClosedReader; /// read handler for half-closed fds
     CommWriteStateData *wstate;         /* State data for comm_write */
     READ_HANDLER *read_method;
     WRITE_HANDLER *write_method;
@@ -121,6 +122,7 @@ private:
     inline void clear() {
         timeoutHandler = NULL;
         closeHandler = NULL;
+        halfClosedReader = NULL;
         // XXX: the following memset may corrupt or leak new or changed members
         memset(this, 0, sizeof(fde));
         local_addr.SetEmpty(); // IPAddress likes to be setup nicely.