From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Fri, 6 Jul 2018 04:08:37 +0000 (+0000)
Subject: Optimization: Fewer epoll(2) system calls when closing a socket (#235)
X-Git-Tag: SQUID_4_2~10
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e1db46616b784eb9caa76f6bfe0009513f2655a6;p=thirdparty%2Fsquid.git

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 358d2c50ea..82556c2ec3 100644
--- 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 c7391ffddb..3f93adf717 100644
--- 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 7e93690372..1be3c4f7f2 100644
--- 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 69fdc31c9b..cb485e48de 100644
--- 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 51fca41f37..c34aa8980b 100644
--- 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 5cbf624051..614eacbf46 100644
--- 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 ba70305bbc..86c68b7d3c 100644
--- 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 c0f3144ef6..ebf13a5255 100644
--- 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 ac1aca4257..044e4d8fe3 100644
--- 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 d40bda4fcb..ca302f772a 100644
--- 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