From: Willy Tarreau <w@1wt.eu>
Date: Fri, 6 Sep 2019 17:05:50 +0000 (+0200)
Subject: MEDIUM: fd: do not use the FD_POLL_* flags in the pollers anymore
X-Git-Tag: v2.1-dev2~95
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6b3089856f47eec05a41bf7ba35e5ccdcaf8ce59;p=thirdparty%2Fhaproxy.git

MEDIUM: fd: do not use the FD_POLL_* flags in the pollers anymore

As mentioned in previous commit, these flags do not map well to
modern poller capabilities. Let's use the FD_EV_*_{R,W} flags instead.
This first patch only performs a 1-to-1 mapping making sure that the
previously reported flags are still reported identically while using
the closest possible semantics in the pollers.

It's worth noting that kqueue will now support improvements such as
returning distinctions between shut and errors on each direction,
though this is not exploited for now.
---

diff --git a/include/proto/fd.h b/include/proto/fd.h
index e3b0b2b2f7..3ee71a5a17 100644
--- a/include/proto/fd.h
+++ b/include/proto/fd.h
@@ -318,20 +318,32 @@ static inline void fd_want_send(int fd)
 	updt_fd_polling(fd);
 }
 
-/* Update events seen for FD <fd> and its state if needed. This should be called
- * by the poller to set FD_POLL_* flags. */
-static inline void fd_update_events(int fd, int evts)
+/* Update events seen for FD <fd> and its state if needed. This should be
+ * called by the poller, passing FD_EV_*_{R,W,RW} in <evts>. FD_EV_ERR_*
+ * doesn't need to also pass FD_EV_SHUT_*, it's implied. ERR and SHUT are
+ * allowed to be reported regardless of R/W readiness.
+ */
+static inline void fd_update_events(int fd, unsigned char evts)
 {
 	unsigned long locked = atleast2(fdtab[fd].thread_mask);
 	unsigned char old, new;
+	int new_flags;
+
+	new_flags =
+	      ((evts & FD_EV_READY_R) ? FD_POLL_IN  : 0) |
+	      ((evts & FD_EV_READY_W) ? FD_POLL_OUT : 0) |
+	      ((evts & FD_EV_SHUT_R)  ? FD_POLL_HUP : 0) |
+	      ((evts & FD_EV_SHUT_W)  ? FD_POLL_ERR : 0) |
+	      ((evts & FD_EV_ERR_R)   ? FD_POLL_ERR : 0) |
+	      ((evts & FD_EV_ERR_W)   ? FD_POLL_ERR : 0);
 
 	old = fdtab[fd].ev;
-	new = (old & FD_POLL_STICKY) | evts;
+	new = (old & FD_POLL_STICKY) | new_flags;
 
 	if (unlikely(locked)) {
 		/* Locked FDs (those with more than 2 threads) are atomically updated */
 		while (unlikely(new != old && !_HA_ATOMIC_CAS(&fdtab[fd].ev, &old, new)))
-			new = (old & FD_POLL_STICKY) | evts;
+			new = (old & FD_POLL_STICKY) | new_flags;
 	} else {
 		if (new != old)
 			fdtab[fd].ev = new;
diff --git a/src/ev_epoll.c b/src/ev_epoll.c
index 5172aecec6..dc156e58f8 100644
--- a/src/ev_epoll.c
+++ b/src/ev_epoll.c
@@ -218,27 +218,15 @@ REGPRM3 static void _do_poll(struct poller *p, int exp, int wake)
 			continue;
 		}
 
-		/* it looks complicated but gcc can optimize it away when constants
-		 * have same values... In fact it depends on gcc :-(
-		 */
-		if (EPOLLIN == FD_POLL_IN && EPOLLOUT == FD_POLL_OUT &&
-		    EPOLLPRI == FD_POLL_PRI && EPOLLERR == FD_POLL_ERR &&
-		    EPOLLHUP == FD_POLL_HUP) {
-			n = e & (EPOLLIN|EPOLLOUT|EPOLLPRI|EPOLLERR|EPOLLHUP);
-		}
-		else {
-			n =	((e & EPOLLIN ) ? FD_POLL_IN  : 0) |
-				((e & EPOLLPRI) ? FD_POLL_PRI : 0) |
-				((e & EPOLLOUT) ? FD_POLL_OUT : 0) |
-				((e & EPOLLERR) ? FD_POLL_ERR : 0) |
-				((e & EPOLLHUP) ? FD_POLL_HUP : 0);
-		}
+		n = ((e & EPOLLIN)    ? FD_EV_READY_R : 0) |
+		    ((e & EPOLLOUT)   ? FD_EV_READY_W : 0) |
+		    ((e & EPOLLRDHUP) ? FD_EV_SHUT_R  : 0) |
+		    ((e & EPOLLHUP)   ? FD_EV_SHUT_RW : 0) |
+		    ((e & EPOLLERR)   ? FD_EV_ERR_RW  : 0);
 
-		/* always remap RDHUP to HUP as they're used similarly */
-		if (e & EPOLLRDHUP) {
+		if ((e & EPOLLRDHUP) && !(cur_poller.flags & HAP_POLL_F_RDHUP))
 			_HA_ATOMIC_OR(&cur_poller.flags, HAP_POLL_F_RDHUP);
-			n |= FD_POLL_HUP;
-		}
+
 		fd_update_events(fd, n);
 	}
 	/* the caller will take care of cached events */
diff --git a/src/ev_evports.c b/src/ev_evports.c
index 46edb69621..e734b220ed 100644
--- a/src/ev_evports.c
+++ b/src/ev_evports.c
@@ -241,14 +241,10 @@ REGPRM3 static void _do_poll(struct poller *p, int exp, int wake)
 		/*
 		 * Set bits based on the events we received from the port:
 		 */
-		if (events & POLLIN)
-			n |= FD_POLL_IN;
-		if (events & POLLOUT)
-			n |= FD_POLL_OUT;
-		if (events & POLLERR)
-			n |= FD_POLL_ERR;
-		if (events & POLLHUP)
-			n |= FD_POLL_HUP;
+		n = ((e & POLLIN)    ? FD_EV_READY_R : 0) |
+		    ((e & POLLOUT)   ? FD_EV_READY_W : 0) |
+		    ((e & POLLHUP)   ? FD_EV_SHUT_RW : 0) |
+		    ((e & POLLERR)   ? FD_EV_ERR_RW  : 0);
 
 		/*
 		 * Call connection processing callbacks.  Note that it's
diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c
index 191ddce568..8f0b7c9072 100644
--- a/src/ev_kqueue.c
+++ b/src/ev_kqueue.c
@@ -196,16 +196,16 @@ REGPRM3 static void _do_poll(struct poller *p, int exp, int wake)
 			continue;
 		}
 
-		if (kev[count].filter ==  EVFILT_READ) {
+		if (kev[count].filter == EVFILT_READ) {
 			if (kev[count].data)
-				n |= FD_POLL_IN;
+				n |= FD_EV_READY_R;
 			if (kev[count].flags & EV_EOF)
-				n |= FD_POLL_HUP;
+				n |= FD_EV_SHUT_R;
 		}
-		else if (kev[count].filter ==  EVFILT_WRITE) {
-			n |= FD_POLL_OUT;
+		else if (kev[count].filter == EVFILT_WRITE) {
+			n |= FD_EV_READY_W;
 			if (kev[count].flags & EV_EOF)
-				n |= FD_POLL_ERR;
+				n |= FD_EV_ERR_RW;
 		}
 
 		fd_update_events(fd, n);
diff --git a/src/ev_poll.c b/src/ev_poll.c
index 6c5de9bea7..7655ca5590 100644
--- a/src/ev_poll.c
+++ b/src/ev_poll.c
@@ -231,25 +231,15 @@ REGPRM3 static void _do_poll(struct poller *p, int exp, int wake)
 			continue;
 		}
 
-		/* it looks complicated but gcc can optimize it away when constants
-		 * have same values... In fact it depends on gcc :-(
-		 */
-		if (POLLIN == FD_POLL_IN && POLLOUT == FD_POLL_OUT &&
-		    POLLERR == FD_POLL_ERR && POLLHUP == FD_POLL_HUP) {
-			n = e & (POLLIN|POLLOUT|POLLERR|POLLHUP);
-		}
-		else {
-			n =     ((e & POLLIN ) ? FD_POLL_IN  : 0) |
-				((e & POLLOUT) ? FD_POLL_OUT : 0) |
-				((e & POLLERR) ? FD_POLL_ERR : 0) |
-				((e & POLLHUP) ? FD_POLL_HUP : 0);
-		}
+		n = ((e & POLLIN)    ? FD_EV_READY_R : 0) |
+		    ((e & POLLOUT)   ? FD_EV_READY_W : 0) |
+		    ((e & POLLRDHUP) ? FD_EV_SHUT_R  : 0) |
+		    ((e & POLLHUP)   ? FD_EV_SHUT_RW : 0) |
+		    ((e & POLLERR)   ? FD_EV_ERR_RW  : 0);
 
-		/* always remap RDHUP to HUP as they're used similarly */
-		if (e & POLLRDHUP) {
+		if ((e & POLLRDHUP) && !(cur_poller.flags & HAP_POLL_F_RDHUP))
 			_HA_ATOMIC_OR(&cur_poller.flags, HAP_POLL_F_RDHUP);
-			n |= FD_POLL_HUP;
-		}
+
 		fd_update_events(fd, n);
 	}
 
diff --git a/src/ev_select.c b/src/ev_select.c
index 7ee915ea5a..dc66e71276 100644
--- a/src/ev_select.c
+++ b/src/ev_select.c
@@ -210,10 +210,10 @@ REGPRM3 static void _do_poll(struct poller *p, int exp, int wake)
 			}
 
 			if (FD_ISSET(fd, tmp_evts[DIR_RD]))
-				n |= FD_POLL_IN;
+				n |= FD_EV_READY_R;
 
 			if (FD_ISSET(fd, tmp_evts[DIR_WR]))
-				n |= FD_POLL_OUT;
+				n |= FD_EV_READY_W;
 
 			fd_update_events(fd, n);
 		}