How to compile Nginx with zlib-ng

[Maxim Dounin]

Hello!

On Mon, Mar 27, 2023 at 09:57:34PM +0400, Sergey Kandaurov wrote:

> > On 24 Mar 2023, at 06:07, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > Hello!
> > 
> > On Thu, Mar 23, 2023 at 09:33:19PM +0100, Richard Stanway via nginx wrote:
> > 
> >> Yes, when using the latest zlib-ng on nginx-1.21.6 I received the
> >> alerts. Previous versions of zlib-ng have worked great after the 2021
> >> patch. I tried to update it myself as follows based on advice of
> >> zlib-ng GitHub issues, while it reduced the number of alerts logged it
> >> did not completely solve the issue so it seems the memory requirements
> >> may have further changed. While I would appreciate a proper patch
> >> making it into nginx, the seemingly-frequent upstream changes may make
> >> this difficult to maintain.
> >> 
> >> -        ctx->allocated = 8192 + 16 + (1 << (wbits + 2))
> >> +        ctx->allocated = 8192 + 288 + 16 + (1 << (wbits + 2))
> >>                          + 131072 + (1 << (memlevel + 8));
> > 
> > It looks like there are at least two changes in zlib-ng since I 
> > looked into it:
> > 
> > - Window bits are no longer forced to 13 on compression level 1.
> > 
> > - All allocations use custom alloc_aligned() wrapper, and 
> >  therefore all allocations are larger than expected by (64 + 
> >  sizeof(void*)).
> > 
> > Further, due to the wrapper nginx sees all allocations as an 
> > allocation of 1 element of a given size, so it misinterprets 
> > some allocations as the state allocation.
> > 
> > [..]
> > 
> > Please try the following patch, it should help with recent versions:
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1679622670 -10800
> > #      Fri Mar 24 04:51:10 2023 +0300
> > # Node ID 67a0999550c3622e51639acb8bde57d199826f7e
> > # Parent  d1cf09451ae84b930ce66fa6d63ae3f7eeeac5a5
> > Gzip: compatibility with recent zlib-ng versions.
> > 
> > It now uses custom alloc_aligned() wrapper for all allocations,
> > therefore all allocations are larger than expected by (64 + sizeof(void*)).
> > Further, they are seen as allocations of 1 element.  Relevant calculations
> > were adjusted to reflect this, and state allocation is now protected
> > with a flag to avoid misinterpreting other allocations as the zlib
> > deflate_state allocation.
> > 
> > Further, it no longer forces window bits to 13 on compression level 1,
> > so the comment was adjusted to reflect this.
> 
> For the record, the corresponding zlib-ng git commits:
> ce6789c7e093e8e6bb6fc591bbdf0f805999bdb9
> a39e323a4db80a57feecf2ae212c08070234050c
> 
> > 
> > diff --git a/src/http/modules/ngx_http_gzip_filter_module.c b/src/http/modules/ngx_http_gzip_filter_module.c
> > --- a/src/http/modules/ngx_http_gzip_filter_module.c
> > +++ b/src/http/modules/ngx_http_gzip_filter_module.c
> > @@ -57,6 +57,7 @@ typedef struct {
> >     unsigned             nomem:1;
> >     unsigned             buffering:1;
> >     unsigned             zlib_ng:1;
> > +    unsigned             state_allocated:1;
> > 
> >     size_t               zin;
> >     size_t               zout;
> > @@ -514,9 +515,10 @@ ngx_http_gzip_filter_memory(ngx_http_req
> >     } else {
> >         /*
> >          * Another zlib variant, https://github.com/zlib-ng/zlib-ng.
> > -         * It forces window bits to 13 for fast compression level,
> > -         * uses 16-byte padding in one of window-sized buffers, and
> > -         * uses 128K hash.
> > +         * It used to force window bits to 13 for fast compression level,
> 
> BTW, it makes sense to peel off this extra allocation somewhere in future,
> similar to how it was done for updated handling of zlib variant from Intel.

Sure, but it's not significant compared to the 128k hash anyway.

> > +         * uses (64 + sizeof(void*)) additional space on all allocations
> > +         * for alignment, 16-byte padding in one of window-sized buffers,
> > +         * and 128K hash.
> >          */
> > 
> >         if (conf->level == 1) {
> > @@ -524,7 +526,8 @@ ngx_http_gzip_filter_memory(ngx_http_req
> >         }
> > 
> >         ctx->allocated = 8192 + 16 + (1 << (wbits + 2))
> > -                         + 131072 + (1 << (memlevel + 8));
> > +                         + 131072 + (1 << (memlevel + 8))
> > +                         + 4 * (64 + sizeof(void*));
> >         ctx->zlib_ng = 1;
> >     }
> > }
> > @@ -926,13 +929,16 @@ ngx_http_gzip_filter_alloc(void *opaque,
> > 
> >     alloc = items * size;
> > 
> > -    if (items == 1 && alloc % 512 != 0 && alloc < 8192) {
> > -
> > +    if (items == 1 && alloc % 512 != 0 && alloc < 8192
> > +        && !ctx->state_allocated)
> > +    {
> >         /*
> >          * The zlib deflate_state allocation, it takes about 6K,
> >          * we allocate 8K.  Other allocations are divisible by 512.
> >          */
> > 
> > +        ctx->state_allocated = 1;
> > +
> >         alloc = 8192;
> 
> Nitpicking:
> now the allocation size comment and assignment look separated.
> Mind moving the ctx->state_allocated line below?

I've tried previously, and it looks ugly.  So it ended up in this 
way.

Note well that the comment is not about setting "alloc" to 8192, but 
mostly about the whole if() branch: it describes the allocation we 
are going to do, and how this particular allocation is selected 
among others.  As such, the order as currently used in the patch 
is quite logical, as the code first ensures that we won't 
accidentally enter the same if() again, and then updates "alloc" to 
be 8192.

> 
> >     }
> > 
> 
> Looks good otherwise.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.

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