Hi again, On Sun, Jan 24, 2016 at 09:00:31PM +0100, Willy Tarreau wrote: > OK so I'm having a preliminary patch for the performance regression > causing incomplete buffers to be processed. It's not optimal yet, > but it's safe and works for me. Now I can reproduce the tests and > always observe the exact same performance between all runs +/- 0.2%.
I've cleaned up the patch and implemented the enforcement of the exact requested buffer size. Now the default setting being 16384, it leaves in a single SSL_write() and the performance issue is gone. I'm getting 416 req/s stable. I'll backport these patches to 1.6 but first I want to understand why we're seeing a freeze at exactly 16000 bytes. We have it in 1.5 too. I'm attaching the 3 patches to apply for Gary. They should apply cleanly over 1.6. That's all for this evening or I'll start doing stupid things. Willy
>From 999f643ed2dcf72779e8c18f300171d87177c04b Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Mon, 25 Jan 2016 01:09:11 +0100 Subject: 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_recv_limit() and it may differ a bit. --- include/proto/channel.h | 91 +++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 44 deletions(-) diff --git a/include/proto/channel.h b/include/proto/channel.h index 733f9d2..848ab02 100644 --- a/include/proto/channel.h +++ b/include/proto/channel.h @@ -158,32 +158,6 @@ static inline int channel_may_send(const struct channel *chn) return chn_cons(chn)->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 channel can still receive data. 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. @@ -324,30 +298,59 @@ static inline void channel_dont_read(struct channel *chn) /*************************************************/ -/* 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 channel_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 * data in transit 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 : + * - the empty buffer has a limit of zero + * - 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 channel_recv_limit(const struct channel *chn) { - return chn->buf->size - channel_reserved(chn); + int transit; + int reserve; + + /* return zero if empty */ + reserve = chn->buf->size; + if (chn->buf == &buf_empty) + goto end; + + /* 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 -- 1.7.12.2.21.g234cd45.dirty
>From 581bf81d34ee312fce1fe4d28b6d1f03995b350c Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Mon, 25 Jan 2016 02:19:13 +0100 Subject: MEDIUM: pools: add a new flag to avoid rounding pool size up Usually it's desirable to merge similarly sized pools, which is the reason why their size is rounded up to the next multiple of 16. But for the buffers this is problematic because we add the size of struct buffer to the user-requested size, and the rounding results in 8 extra bytes that are usable in the end. So the user gets more bytes than asked for, and in case of SSL it results in short writes for the extra bytes that are sent above multiples of 16 kB. So we add a new flag MEM_F_EXACT to request that the size is not rounded up when creating the entry. Thus it doesn't disable merging. --- include/common/memory.h | 1 + src/memory.c | 10 +++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/include/common/memory.h b/include/common/memory.h index 2b7121b..d52fb62 100644 --- a/include/common/memory.h +++ b/include/common/memory.h @@ -33,6 +33,7 @@ #else #define MEM_F_SHARED 0 #endif +#define MEM_F_EXACT 0x2 /* reserve an extra void* at the end of a pool for linking */ #ifdef DEBUG_MEMORY_POOLS diff --git a/src/memory.c b/src/memory.c index 036f786..53ab489 100644 --- a/src/memory.c +++ b/src/memory.c @@ -24,7 +24,9 @@ int mem_poison_byte = -1; /* Try to find an existing shared pool with the same characteristics and * returns it, otherwise creates this one. NULL is returned if no memory - * is available for a new creation. + * is available for a new creation. Two flags are supported : + * - MEM_F_SHARED to indicate that the pool may be shared with other users + * - MEM_F_EXACT to indicate that the size must not be rounded up */ struct pool_head *create_pool(char *name, unsigned int size, unsigned int flags) { @@ -41,8 +43,10 @@ struct pool_head *create_pool(char *name, unsigned int size, unsigned int flags) * so that the visible parts outside are not affected. */ - align = 16; - size = ((size + POOL_EXTRA + align - 1) & -align) - POOL_EXTRA; + if (!(flags & MEM_F_EXACT)) { + align = 16; + size = ((size + POOL_EXTRA + align - 1) & -align) - POOL_EXTRA; + } start = &pools; pool = NULL; -- 1.7.12.2.21.g234cd45.dirty
>From 484b53da5218034898964d8270da0d27f9de66a0 Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Mon, 25 Jan 2016 02:23:25 +0100 Subject: BUG/MEDIUM: buffers: do not round up buffer size during allocation MIME-Version: 1.0 Content-Type: text/plain; charset=latin1 Content-Transfer-Encoding: 8bit When users request 16384 bytes for a buffer, they get 16392 after rounding up. This is problematic for SSL as it systematically causes a small 8-bytes message to be appended after the first 16kB message and costs about 15% of performance. Let's add MEM_F_EXACT to use exactly the size we need. This requires previous patch (MEDIUM: pools: add a new flag to avoid rounding pool size up). This issue was introduced in 1.6 and causes trouble there, so this fix must be backported. This is issue was reported by Gary Barrueto and diagnosed by Cyril Bonté. --- src/buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer.c b/src/buffer.c index b083768..f47fbdd 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -36,7 +36,7 @@ int init_buffer() { void *buffer; - pool2_buffer = create_pool("buffer", sizeof (struct buffer) + global.tune.bufsize, MEM_F_SHARED); + pool2_buffer = create_pool("buffer", sizeof (struct buffer) + global.tune.bufsize, MEM_F_SHARED|MEM_F_EXACT); if (!pool2_buffer) return 0; -- 1.7.12.2.21.g234cd45.dirty
