From: Willy Tarreau <w@1wt.eu>
Date: Mon, 19 Jul 2010 16:16:03 +0000 (+0200)
Subject: [MAJOR] stream_sock: better wakeup conditions on read()
X-Git-Tag: v1.5-dev8~536
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5af1fa1df0f367d9ce5557c93e0a229263618729;p=thirdparty%2Fhaproxy.git

[MAJOR] stream_sock: better wakeup conditions on read()

After a read, there was a condition to mandatorily wake the task
up if the BF_READ_DONTWAIT flag was set. This was wrong because
the wakeup condition in this case can be deduced from the other
ones. Another condition was put on the other side not being in
SI_ST_EST state. It is not appropriate to do this because it
causes a useless wakeup at the beginning of every first request
in case of speculative polling, due to the fact that we don't
read anything and that the other side is still in SI_ST_INI.
Also, the wakeup was performed whenever to_forward was null,
which causes an unexpected wakeup upon the first read for the
same reason. However, those two conditions are valid if and
only if at least one read was performed.

Also, the BF_SHUTR flag was tested as part of the wakeup condition,
while this one can only be set if BF_READ_NULL is set too. So let's
simplify this ambiguous test by removing the BF_SHUTR part from the
condition to only process events.

Last, the BF_READ_DONTWAIT flag was unconditionally cleared,
while sometimes there would have been no I/O. Now we only clear
it once the I/O operation has been performed, which maintains
its validity until the I/O occurs.

Finally, those fixes saved approximately 16% of the per-session
wakeups and 20% of the epoll_ctl() calls, which translates into
slightly less under high load due to the request often being ready
when the read() occurs. A performance increase between 2 and 5% is
expected depending on the workload.

It does not seem necessary to backport this change to 1.4, eventhough
it fixes some performance issues. It may later be backported if
required to fix something else because the risk of regression seems
very low due to the fact that we're more in line with the documented
semantics.
---

diff --git a/src/stream_sock.c b/src/stream_sock.c
index 8cd721ea7e..1a824bd71b 100644
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -489,14 +489,15 @@ int stream_sock_read(int fd) {
 	/* we have to wake up if there is a special event or if we don't have
 	 * any more data to forward.
 	 */
-	if ((b->flags & (BF_READ_NULL|BF_READ_ERROR|BF_SHUTR|BF_READ_DONTWAIT)) ||
-	    !b->to_forward ||
+	if ((b->flags & (BF_READ_NULL|BF_READ_ERROR)) ||
 	    si->state != SI_ST_EST ||
-	    b->cons->state != SI_ST_EST ||
-	    (si->flags & SI_FL_ERR))
+	    (si->flags & SI_FL_ERR) ||
+	    ((b->flags & BF_READ_PARTIAL) && (!b->to_forward || b->cons->state != SI_ST_EST)))
 		task_wakeup(si->owner, TASK_WOKEN_IO);
-	
-	b->flags &= ~BF_READ_DONTWAIT;
+
+	if (b->flags & BF_READ_ACTIVITY)
+		b->flags &= ~BF_READ_DONTWAIT;
+
 	fdtab[fd].ev &= ~FD_POLL_IN;
 	return retval;