Optimization: Fewer epoll(2) system calls when closing a socket (#235)

Squid was calling epoll(2) twice to clear a socket interest. One call is
more than enough: Technically, close(2) is supposed to clear epoll(2)
registration for us, but I did not risk relying on that.

In other environments, socket interest changes are pooled together
before being submitted to the OS, so Squid was doing a bit of extra
work, but not making (many) extra system calls AFAICT.

Also fixed (previously unused) Comm::ResetSelect() on these platforms:
* epoll(2): The old resetting code did not clear our interest AFAICT.
* kqueue(2): The old resetting code made no sense to me at all.
* poll(2): There was no code at all.
* select(Win32): There was no code at all.

Even though Comm::ResetSelect() implementation is now the same for all
platforms, I did not make that code platform-agnostic because it is
possible to optimize it further in platform-specific ways.

diff --git a/src/client_side.cc b/src/client_side.cc
index 358d2c50ea42577e7a7b877647c00874f058841e..82556c2ec304ef91bf4015620d06c2efeb2431a8 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3187,8 +3187,7 @@ ConnStateData::parseTlsHandshake()
     }
 
     // We should disable read/write handlers
-    Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
-    Comm::SetSelect(clientConnection->fd, COMM_SELECT_WRITE, NULL, NULL, 0);
+    Comm::ResetSelect(clientConnection->fd);
 
     if (unsupportedProtocol) {
         Http::StreamPointer context = pipeline.front();

diff --git a/src/comm/Loops.h b/src/comm/Loops.h
index c7391ffddbc4b28a1b4275c22d342f148f9d9156..3f93adf7175c8cd1633bc5fee73d70c91d62e2ec 100644 (file)
--- a/src/comm/Loops.h
+++ b/src/comm/Loops.h
@@ -11,6 +11,7 @@
 
 #include "comm/Flag.h"
 #include "comm/forward.h"
+#include "defines.h"
 
 /* Comm layer select loops API.
  *
@@ -28,7 +29,11 @@ void SelectLoopInit(void);
 void SetSelect(int, unsigned int, PF *, void *, time_t);
 
 /// reset/undo/unregister the watch for an FD which was set by Comm::SetSelect()
-void ResetSelect(int);
+inline void
+ResetSelect(int fd)
+{
+    SetSelect(fd, COMM_SELECT_READ|COMM_SELECT_WRITE, nullptr, nullptr, 0);
+}
 
 /** Perform a select() or equivalent call.
  * This is used by the main select loop engine to check for FD with IO available.

diff --git a/src/comm/ModDevPoll.cc b/src/comm/ModDevPoll.cc
index 7e93690372f5e99d16d43000d66d1ac8e87300df..1be3c4f7f22391a072c6196b5eebfecf06b5200c 100644 (file)
--- a/src/comm/ModDevPoll.cc
+++ b/src/comm/ModDevPoll.cc
@@ -296,17 +296,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
         F->timeout = squid_curtime + timeout;
 }
 
-/** \brief Clear polling of file handle (both read and write)
- *
- * @param fd file descriptor to clear polling on
- */
-void
-Comm::ResetSelect(int fd)
-{
-    SetSelect(fd, COMM_SELECT_WRITE, NULL, NULL, 0);
-    SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0);
-}
-
 /** \brief Do poll and trigger callback functions as appropriate
  *
  * Check all connections for new connections and input data that is to be

diff --git a/src/comm/ModEpoll.cc b/src/comm/ModEpoll.cc
index 69fdc31c9b308a8347a66942f4729383e45266ab..cb485e48de8ad1595ca57052b619d12d4ee37e7b 100644 (file)
--- a/src/comm/ModEpoll.cc
+++ b/src/comm/ModEpoll.cc
@@ -180,14 +180,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
         F->timeout = squid_curtime + timeout;
 }
 
-void
-Comm::ResetSelect(int fd)
-{
-    fde *F = &fd_table[fd];
-    F->epoll_state = 0;
-    SetSelect(fd, 0, NULL, NULL, 0);
-}
-
 static void commIncomingStats(StoreEntry * sentry);
 
 static void

diff --git a/src/comm/ModKqueue.cc b/src/comm/ModKqueue.cc
index 51fca41f3700bfcfbd066377f409efe7785a6281..c34aa8980b2a4ce9c0260981c0268c2f6e2ab381 100644 (file)
--- a/src/comm/ModKqueue.cc
+++ b/src/comm/ModKqueue.cc
@@ -168,7 +168,7 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
 {
     fde *F = &fd_table[fd];
     assert(fd >= 0);
-    assert(F->flags.open);
+    assert(F->flags.open || (!handler && !client_data && !timeout));
     debugs(5, 5, HERE << "FD " << fd << ", type=" << type <<
            ", handler=" << handler << ", client_data=" << client_data <<
            ", timeout=" << timeout);
@@ -194,18 +194,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
 
 }
 
-void
-Comm::ResetSelect(int fd)
-{
-    fde *F = &fd_table[fd];
-    if (F->read_handler) {
-        kq_update_events(fd, EVFILT_READ, (PF *)1);
-    }
-    if (F->write_handler) {
-        kq_update_events(fd, EVFILT_WRITE, (PF *)1);
-    }
-}
-
 /*
  * Check all connections for new connections and input data that is to be
  * processed. Also check for connections with data queued and whether we can

diff --git a/src/comm/ModPoll.cc b/src/comm/ModPoll.cc
index 5cbf62405154a6bbeb53177ee553dd201589c9cb..614eacbf46ba3eba741f703971ecf0eab9c7f7ed 100644 (file)
--- a/src/comm/ModPoll.cc
+++ b/src/comm/ModPoll.cc
@@ -125,7 +125,7 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
 {
     fde *F = &fd_table[fd];
     assert(fd >= 0);
-    assert(F->flags.open);
+    assert(F->flags.open || (!handler && !client_data && !timeout));
     debugs(5, 5, HERE << "FD " << fd << ", type=" << type <<
            ", handler=" << handler << ", client_data=" << client_data <<
            ", timeout=" << timeout);
@@ -144,11 +144,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
         F->timeout = squid_curtime + timeout;
 }
 
-void
-Comm::ResetSelect(int fd)
-{
-}
-
 static int
 fdIsUdpListen(int fd)
 {

diff --git a/src/comm/ModSelect.cc b/src/comm/ModSelect.cc
index ba70305bbc1a29dfb8da4d9be863b9a39c3a457b..86c68b7d3cf4a324cb8ad274179522024b9972f7 100644 (file)
--- a/src/comm/ModSelect.cc
+++ b/src/comm/ModSelect.cc
@@ -124,7 +124,7 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
 {
     fde *F = &fd_table[fd];
     assert(fd >= 0);
-    assert(F->flags.open);
+    assert(F->flags.open || (!handler && !client_data && !timeout));
     debugs(5, 5, HERE << "FD " << fd << ", type=" << type <<
            ", handler=" << handler << ", client_data=" << client_data <<
            ", timeout=" << timeout);
@@ -145,11 +145,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
         F->timeout = squid_curtime + timeout;
 }
 
-void
-Comm::ResetSelect(int fd)
-{
-}
-
 static int
 fdIsUdpListener(int fd)
 {

diff --git a/src/comm/ModSelectWin32.cc b/src/comm/ModSelectWin32.cc
index c0f3144ef6f0c54be57c4670464ecbb7d53b9345..ebf13a52551361126c6af74ad5faae450b76f14a 100644 (file)
--- a/src/comm/ModSelectWin32.cc
+++ b/src/comm/ModSelectWin32.cc
@@ -118,7 +118,7 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
 {
     fde *F = &fd_table[fd];
     assert(fd >= 0);
-    assert(F->flags.open);
+    assert(F->flags.open || (!handler && !client_data && !timeout));
     debugs(5, 5, HERE << "FD " << fd << ", type=" << type <<
            ", handler=" << handler << ", client_data=" << client_data <<
            ", timeout=" << timeout);
@@ -139,11 +139,6 @@ Comm::SetSelect(int fd, unsigned int type, PF * handler, void *client_data, time
         F->timeout = squid_curtime + timeout;
 }
 
-void
-Comm::ResetSelect(int fd)
-{
-}
-
 static int
 fdIsUdpListener(int fd)
 {

diff --git a/src/fd.cc b/src/fd.cc
index ac1aca4257796a9a36e8eee3ef5813610204fe79..044e4d8fe3ef8bd8bf05f63f54e3d9b6f13127a2 100644 (file)
--- a/src/fd.cc
+++ b/src/fd.cc
@@ -92,8 +92,7 @@ fd_close(int fd)
     }
 
     debugs(51, 3, "fd_close FD " << fd << " " << F->desc);
-    Comm::SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0);
-    Comm::SetSelect(fd, COMM_SELECT_WRITE, NULL, NULL, 0);
+    Comm::ResetSelect(fd);
     F->flags.open = false;
     fdUpdateBiggest(fd, 0);
     --Number_FD;

diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc
index d40bda4fcb6ff99759bd02d588381963ecf11148..ca302f772a6a5afeb571c0d745161590eaba113c 100644 (file)
--- a/src/tests/stub_libcomm.cc
+++ b/src/tests/stub_libcomm.cc
@@ -52,7 +52,6 @@ void Comm::CallbackTableDestruct() STUB
 #include "comm/Loops.h"
 void Comm::SelectLoopInit(void) STUB
 void Comm::SetSelect(int, unsigned int, PF *, void *, time_t) STUB
-void Comm::ResetSelect(int) STUB
 Comm::Flag Comm::DoSelect(int) STUB_RETVAL(Comm::COMM_ERROR)
 void Comm::QuickPollRequired(void) STUB