BUG/MEDIUM: channel: fix miscalculation of available buffer space.
The function channel_recv_limit() relies on channel_reserved() which
itself relies on channel_in_transit(). Individually they're OK but
combined they're doing the wrong thing.
The problem is that we refrain from filling buffers while to_forward
is even much larger than the buffer because of a semantic issue along
the call chain. This is particularly visible when offloading SSL on
moderately large files (1 MB), though it is also visible on clear text.
Twice the number of recv() calls are made compared to what is needed,
and the typical performance drops by 15-20% in SSL in 1.6 and later,
and no directly measurable drop in 1.5 except when using strace.
There's no need for all these intermediate functions, so let's get
rid of them and reimplement channel_recv_limit() from scratch in a
safer way.
This fix needs to be backported to 1.6 and 1.5 (at least). Note that in
1.5 the function is called buffer_max_len() and it may differ a bit.
(cherry picked from commit 999f643ed2dcf72779e8c18f300171d87177c04b)
(cherry picked from commit fed3c4efe837d2201c1bb6be7ad532227451b8ed)
[1.5: it's buffer_reserved() which was removed, channel_reserved() is a
boolean used for a different purpose there. There's no empty buffer in
1.5. The logic remains the same]
diff --git a/include/proto/channel.h b/include/proto/channel.h
index 3c835b8..b281119 100644
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -127,32 +127,6 @@
return chn->cons->state == SI_ST_EST;
}
-/* Returns the amount of bytes from the channel that are already scheduled for
- * leaving (buf->o) or that are still part of the input and expected to be sent
- * soon as covered by to_forward. This is useful to know by how much we can
- * shrink the rewrite reserve during forwards. Buffer data are not considered
- * in transit until the channel is connected, so that the reserve remains
- * protected.
- */
-static inline int channel_in_transit(const struct channel *chn)
-{
- int ret;
-
- if (!channel_may_send(chn))
- return 0;
-
- /* below, this is min(i, to_forward) optimized for the fast case */
- if (chn->to_forward >= chn->buf->i ||
- (CHN_INFINITE_FORWARD < MAX_RANGE(typeof(chn->buf->i)) &&
- chn->to_forward == CHN_INFINITE_FORWARD))
- ret = chn->buf->i;
- else
- ret = chn->to_forward;
-
- ret += chn->buf->o;
- return ret;
-}
-
/* Returns non-zero if the buffer input is considered full. This is used to
* decide when to stop reading into a buffer when we want to ensure that we
* leave the reserve untouched after all pending outgoing data are forwarded.
@@ -291,31 +265,54 @@
/* Buffer operations in the context of a channel */
/*************************************************/
-
-/* Return the number of reserved bytes in the channel's visible
- * buffer, which ensures that once all pending data are forwarded, the
- * buffer still has global.tune.maxrewrite bytes free. The result is
- * between 0 and global.tune.maxrewrite, which is itself smaller than
- * any chn->size. Special care is taken to avoid any possible integer
- * overflow in the operations.
- */
-static inline int buffer_reserved(const struct channel *chn)
-{
- int reserved;
-
- reserved = global.tune.maxrewrite - channel_in_transit(chn);
- if (reserved < 0)
- reserved = 0;
- return reserved;
-}
/* Return the max number of bytes the buffer can contain so that once all the
* pending bytes are forwarded, the buffer still has global.tune.maxrewrite
* bytes free. The result sits between chn->size - maxrewrite and chn->size.
+ * The principle is the following :
+ * - a non-connected buffer cannot touch the reserve
+ * - infinite forward can fill the buffer
+ * - all output bytes are ignored, they're leaving
+ * - all input bytes covered by to_forward are considered in transit and
+ * virtually don't take room
+ * - the reserve may be covered up to the min of (fwd-transit) since these
+ * bytes will be in transit later thus will only take temporary space.
+ *
+ * So the formula is to return this limit is :
+ * size - maxrewrite + min(fwd - min(i, fwd), maxrewrite)
+ * = size - maxrewrite + min( min(fwd - i, 0), maxrewrite)
+ *
+ * The code isn't written the most obvious way because we help the compiler
+ * optimise it as it cannot guess how to factor the result out. The most common
+ * path is jumpless.
*/
static inline int buffer_max_len(const struct channel *chn)
{
- return chn->buf->size - buffer_reserved(chn);
+ int transit;
+ int reserve;
+
+ /* return size - maxrewrite if we can't send */
+ reserve = global.tune.maxrewrite;
+ if (unlikely(!channel_may_send(chn)))
+ goto end;
+
+ /* This apparently tricky check is just a hint to let the compiler
+ * optimize all this code away as long as we don't change the types.
+ */
+ reserve = 0;
+ if (CHN_INFINITE_FORWARD < MAX_RANGE(typeof(chn->buf->i)) &&
+ chn->to_forward == CHN_INFINITE_FORWARD)
+ goto end;
+
+ transit = chn->to_forward - chn->buf->i;
+ if (transit < 0)
+ transit = 0;
+
+ reserve = global.tune.maxrewrite - transit;
+ if (reserve < 0)
+ reserve = 0;
+ end:
+ return chn->buf->size - reserve;
}
/* Returns the amount of space available at the input of the buffer, taking the