From: Christopher Faulet <cfaulet@haproxy.com>
Date: Mon, 6 Nov 2023 07:45:22 +0000 (+0100)
Subject: BUG/MEDIUM: stconn: Don't report rcv/snd expiration date if SC cannot epxire
X-Git-Tag: v2.9-dev10~152
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4a2660aa455e3496eadb9c3cb661245c07c7a3bb;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: stconn: Don't report rcv/snd expiration date if SC cannot epxire

When receive or send expiration date of a stream-connector is retrieved, we
now automatically check if it may expire. If not, TICK_ETERNITY is returned.

The expiration dates of the frontend and backend stream-connectors are used
to compute the stream expiration date. This operation is performed at 2
places: at the end of process_stream() and in sc_notify() if the stream is
not woken up.

With this patch, there is no special changes for process_stream() because it
was already handled. It make thing a little simpler. However, it fixes
sc_notify() by avoiding to erroneously compute an expiration date in
past. This highly reduce the stream wakeups when there is contention on the
consumer side.

The bug was introduced with the commit 8073094bf ("NUG/MEDIUM: stconn:
Always update stream's expiration date after I/O"). It was an error to
unconditionnaly set the stream expiration data, without testing blocking
conditions on both SC.

This patch must be backported to 2.8.
---

diff --git a/include/haproxy/sc_strm.h b/include/haproxy/sc_strm.h
index 8810d616a4..98b803dd16 100644
--- a/include/haproxy/sc_strm.h
+++ b/include/haproxy/sc_strm.h
@@ -379,11 +379,25 @@ static inline int sc_snd_may_expire(const struct stconn *sc)
 	return 1;
 }
 
+static forceinline int sc_ep_rcv_ex(const struct stconn *sc)
+{
+	return ((tick_isset(sc->sedesc->lra) && sc_rcv_may_expire(sc))
+		? tick_add_ifset(sc->sedesc->lra, sc->ioto)
+		: TICK_ETERNITY);
+}
+
+static forceinline int sc_ep_snd_ex(const struct stconn *sc)
+{
+	return ((tick_isset(sc->sedesc->fsb) && sc_snd_may_expire(sc))
+		? tick_add_ifset(sc->sedesc->fsb, sc->ioto)
+		: TICK_ETERNITY);
+}
+
 static inline void sc_check_timeouts(const struct stconn *sc)
 {
-	if (likely(sc_rcv_may_expire(sc)) && unlikely(tick_is_expired(sc_ep_rcv_ex(sc), now_ms)))
+	if (unlikely(tick_is_expired(sc_ep_rcv_ex(sc), now_ms)))
 		sc_ic(sc)->flags |= CF_READ_TIMEOUT;
-	if (likely(sc_snd_may_expire(sc)) && unlikely(tick_is_expired(sc_ep_snd_ex(sc), now_ms)))
+	if (unlikely(tick_is_expired(sc_ep_snd_ex(sc), now_ms)))
 		sc_oc(sc)->flags |= CF_WRITE_TIMEOUT;
 }
 
diff --git a/include/haproxy/stconn.h b/include/haproxy/stconn.h
index 4ba6aca205..3b24b217e5 100644
--- a/include/haproxy/stconn.h
+++ b/include/haproxy/stconn.h
@@ -210,20 +210,6 @@ static forceinline size_t sc_ep_ff_data(struct stconn *sc)
 	return se_ff_data(sc->sedesc);
 }
 
-static forceinline int sc_ep_rcv_ex(const struct stconn *sc)
-{
-	return (tick_isset(sc->sedesc->lra)
-		? tick_add_ifset(sc->sedesc->lra, sc->ioto)
-		: TICK_ETERNITY);
-}
-
-static forceinline int sc_ep_snd_ex(const struct stconn *sc)
-{
-	return (tick_isset(sc->sedesc->fsb)
-		? tick_add_ifset(sc->sedesc->fsb, sc->ioto)
-		: TICK_ETERNITY);
-}
-
 /* Returns the stream endpoint from an connector, without any control */
 static inline void *__sc_endp(const struct stconn *sc)
 {
diff --git a/src/stream.c b/src/stream.c
index ef8d46625e..6a6166eea7 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -2518,17 +2518,6 @@ struct task *process_stream(struct task *t, void *context, unsigned int state)
 
 	update_exp_and_leave:
 		/* Note: please ensure that if you branch here you disable SC_FL_DONT_WAKE */
-		t->expire = (tick_is_expired(t->expire, now_ms) ? 0 : t->expire);
-
-		if (likely(sc_rcv_may_expire(scf)))
-			t->expire = tick_first(t->expire, sc_ep_rcv_ex(scf));
-		if (likely(sc_snd_may_expire(scf)))
-			t->expire = tick_first(t->expire, sc_ep_snd_ex(scf));
-		if (likely(sc_rcv_may_expire(scb)))
-			t->expire = tick_first(t->expire, sc_ep_rcv_ex(scb));
-		if (likely(sc_snd_may_expire(scb)))
-			t->expire = tick_first(t->expire, sc_ep_snd_ex(scb));
-
 		if (!req->analysers)
 			req->analyse_exp = TICK_ETERNITY;
 		if (!res->analysers)
@@ -2538,10 +2527,13 @@ struct task *process_stream(struct task *t, void *context, unsigned int state)
 		          (!tick_isset(req->analyse_exp) || tick_is_expired(req->analyse_exp, now_ms)))
 			req->analyse_exp = tick_add(now_ms, 5000);
 
+		t->expire = (tick_is_expired(t->expire, now_ms) ? 0 : t->expire);
+		t->expire = tick_first(t->expire, sc_ep_rcv_ex(scf));
+		t->expire = tick_first(t->expire, sc_ep_snd_ex(scf));
+		t->expire = tick_first(t->expire, sc_ep_rcv_ex(scb));
+		t->expire = tick_first(t->expire, sc_ep_snd_ex(scb));
 		t->expire = tick_first(t->expire, req->analyse_exp);
-
 		t->expire = tick_first(t->expire, res->analyse_exp);
-
 		t->expire = tick_first(t->expire, s->conn_exp);
 
 		if (unlikely(tick_is_expired(t->expire, now_ms))) {