[PATCH] HTTP/2: fixed buffer management with HTTP/2 auto-detection

[Maxim Dounin]

Hello!

On Fri, Oct 20, 2023 at 06:04:32PM +0400, Sergey Kandaurov wrote:

> > On 20 Oct 2023, at 17:23, Sergey Kandaurov <pluknet at nginx.com> wrote:
> > 
> > # HG changeset patch
> > # User Sergey Kandaurov <pluknet at nginx.com>
> > # Date 1697808142 -14400
> > #      Fri Oct 20 17:22:22 2023 +0400
> > # Node ID 318c8ace6aa24506004bfbb7d52674f61a3716a5
> > # Parent  3038bd4d78169a5e8a2624d79cf76f45f0805ddc
> > HTTP/2: fixed buffer management with HTTP/2 auto-detection.
> > 
> > As part of normal HTTP/2 processing, incomplete frames are saved in the
> > control state using a fixed size memcpy of NGX_HTTP_V2_STATE_BUFFER_SIZE.
> > For this matter, two state buffers are reserved in the HTTP/2 recv buffer.
> > 
> > As part of HTTP/2 auto-detection on plain TCP connections, initial data
> > is first read into a buffer specified by the client_header_buffer_size
> > directive that doesn't have state reservation.  Previously, this made it
> > possible to over-read the buffer as part of saving the state.
> > 
> > The fix is to read the available buffer size rather than a fixed size.
> > Although memcpy of a fixed size can produce a better optimized code,
> 
> From my limited testing, replacing a fixed size with an available size
> degrades "-O" optimized memcpy from SSE instructions over XMM registers
> to simple MOVs.

I don't think it matters compared to other costs within the loop.

> > handling of incomplete frames isn't a common execution path, so it was
> > sacrificed for the sake of simplicity of the fix.
> 
> Another approach is to displace initial data into the recv buffer
> for subsequent processing, which would require additional handling
> in ngx_http_v2_init().  After some pondering I declined it due to
> added complexity without a good reason.
> 
> > 
> > diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> > --- a/src/http/v2/ngx_http_v2.c
> > +++ b/src/http/v2/ngx_http_v2.c
> > @@ -386,13 +386,11 @@ ngx_http_v2_read_handler(ngx_event_t *re
> >     h2mcf = ngx_http_get_module_main_conf(h2c->http_connection->conf_ctx,
> >                                           ngx_http_v2_module);
> > 
> > -    available = h2mcf->recv_buffer_size - 2 * NGX_HTTP_V2_STATE_BUFFER_SIZE;
> > +    available = h2mcf->recv_buffer_size - NGX_HTTP_V2_STATE_BUFFER_SIZE;
> > 
> >     do {
> >         p = h2mcf->recv_buffer;
> > -
> > -        ngx_memcpy(p, h2c->state.buffer, NGX_HTTP_V2_STATE_BUFFER_SIZE);
> > -        end = p + h2c->state.buffer_used;
> > +        end = ngx_cpymem(p, h2c->state.buffer, h2c->state.buffer_used);
> > 
> >         n = c->recv(c, end, available);
> > 
> > @@ -2592,7 +2590,7 @@ ngx_http_v2_state_save(ngx_http_v2_conne
> >         return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_INTERNAL_ERROR);
> >     }
> > 
> > -    ngx_memcpy(h2c->state.buffer, pos, NGX_HTTP_V2_STATE_BUFFER_SIZE);
> > +    ngx_memcpy(h2c->state.buffer, pos, size);
> > 
> >     h2c->state.buffer_used = size;
> >     h2c->state.handler = handler;
> > diff --git a/src/http/v2/ngx_http_v2_module.c b/src/http/v2/ngx_http_v2_module.c
> > --- a/src/http/v2/ngx_http_v2_module.c
> > +++ b/src/http/v2/ngx_http_v2_module.c
> > @@ -388,7 +388,7 @@ ngx_http_v2_recv_buffer_size(ngx_conf_t 
> > {
> >     size_t *sp = data;
> > 
> > -    if (*sp <= 2 * NGX_HTTP_V2_STATE_BUFFER_SIZE) {
> > +    if (*sp <= NGX_HTTP_V2_STATE_BUFFER_SIZE) {
> >         return "value is too small";
> >     }
> > 

Looks good.

-- 
Maxim Dounin
http://mdounin.ru/