Has anyone had success compiling Nginx with zlib-ng instead of default Zlib versions? I seem to be able to compile Nginx with standard Zlib and various other Zlib libraries (e.g. Intel optimized or Cloudflare) but compiling with Zlib-NG always fails. I’ve tried passing in various options via with-zlib-opt to try to include the —zlib-compat flag for the Zlib NG configure directives but no matter what syntax I use, it seems like it always fails (whether I have that param or not). Perhaps I’m just struggling with the proper use of Zlib NG in an Nginx compile context.
If Nginx should compile with Zlib NG, is there any documentation on what params to use in the Nginx compile command to get it to work?
--
Lance Dockins
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20230321/6b23aa15/attachment.htm>
Hi Lance,
thanks for your question.
Since this is more or less related to nginx development or
new features I'd suggest to use nginx-devel mailing list
instead, thank you.
On Tue, Mar 21, 2023 at 04:06:00PM -0500, Lance Dockins wrote:
>
> Has anyone had success compiling Nginx with zlib-ng instead of
> default Zlib versions? I seem to be able to compile Nginx with
> standard Zlib and various other Zlib libraries (e.g. Intel
> optimized or Cloudflare) but compiling with Zlib-NG always fails.
NGINX builds well with zlib. In case of new functionality, like
an ability to build with zlib-ng, the source code requires some
patches.
> I’ve tried passing in various options via with-zlib-opt to try
> to include the —zlib-compat flag for the Zlib NG configure
> directives but no matter what syntax I use, it seems like it
> always fails (whether I have that param or not). Perhaps I’m
> just struggling with the proper use of Zlib NG in an Nginx
> compile context.
>
> If Nginx should compile with Zlib NG, is there any documentation
> on what params to use in the Nginx compile command to get it to
> work?
Some ideas can be found inside the zlib-ng project on GH, not
sure is that working solution or not, so you can try.
I'd also recommend to raise a request in https://trac.nginx.org/nginx/
about this feature request and provide patches for source code
and documentation.
Thank you.
--
Sergey A. Osokin
I regularly build with zlib-ng, unfortunately it requires patching the
zlib-ng files to enable zlib compatibility mode as nginx doesn't seem to
have a way to pass options to configure.
Edit "configure" in the zlib-ng directory and change the line compat=0 to
compat=1. Then specify --with-zlib=/path/to/zlib-ng in your nginx configure
and you should be set.
Be aware that the memory requirements of zlib-ng have changed since support
for it was added to nginx, so you will see a lot of "gzip filter failed to
use preallocated memory" alerts in your log file when using zlib-ng.
On Wed, 22 Mar 2023 at 15:50, Sergey A. Osokin <osa at freebsd.org.ru> wrote:
> Hi Lance,
>
> thanks for your question.
>
> Since this is more or less related to nginx development or
> new features I'd suggest to use nginx-devel mailing list
> instead, thank you.
>
> On Tue, Mar 21, 2023 at 04:06:00PM -0500, Lance Dockins wrote:
> >
> > Has anyone had success compiling Nginx with zlib-ng instead of
> > default Zlib versions? I seem to be able to compile Nginx with
> > standard Zlib and various other Zlib libraries (e.g. Intel
> > optimized or Cloudflare) but compiling with Zlib-NG always fails.
>
> NGINX builds well with zlib. In case of new functionality, like
> an ability to build with zlib-ng, the source code requires some
> patches.
>
> > I’ve tried passing in various options via with-zlib-opt to try
> > to include the —zlib-compat flag for the Zlib NG configure
> > directives but no matter what syntax I use, it seems like it
> > always fails (whether I have that param or not). Perhaps I’m
> > just struggling with the proper use of Zlib NG in an Nginx
> > compile context.
> >
> > If Nginx should compile with Zlib NG, is there any documentation
> > on what params to use in the Nginx compile command to get it to
> > work?
>
> Some ideas can be found inside the zlib-ng project on GH, not
> sure is that working solution or not, so you can try.
>
> I'd also recommend to raise a request in https://trac.nginx.org/nginx/
> about this feature request and provide patches for source code
> and documentation.
>
> Thank you.
>
> --
> Sergey A. Osokin
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20230322/c535b358/attachment.htm>
Thank you, Richard. I’ll give that a shot. I already have to do that sort of patching with a variety of other things in the build that I use so that particular adjustment isn’t too bad.
Just for clarity, are you saying that the hash sizes within zlib-ng have increased since Maxim’s last patch for that to accommodate zlib-ng? That patch was back in 2021 and is part of Nginx core now.
https://mailman.nginx.org/pipermail/nginx-devel/2021-April/013945.html
I think it’s coded to use a 128k hash per that patch. If the hash size has increased again since that patch, that might justify a bug report to Nginx devel. Since the code in that patch specifically relates to that error, I thought I’d ask in case you have still been seeing that error with newer Nginx versions that have come out since that patch was implemented.
--
Lance
On Mar 22, 2023 at 5:28 PM -0500, Richard Stanway via nginx <nginx at nginx.org>, wrote:
> I regularly build with zlib-ng, unfortunately it requires patching the zlib-ng files to enable zlib compatibility mode as nginx doesn't seem to have a way to pass options to configure.
>
> Edit "configure" in the zlib-ng directory and change the line compat=0 to compat=1. Then specify --with-zlib=/path/to/zlib-ng in your nginx configure and you should be set.
>
> Be aware that the memory requirements of zlib-ng have changed since support for it was added to nginx, so you will see a lot of "gzip filter failed to use preallocated memory" alerts in your log file when using zlib-ng.
>
>
> > On Wed, 22 Mar 2023 at 15:50, Sergey A. Osokin <osa at freebsd.org.ru> wrote:
> > > Hi Lance,
> > >
> > > thanks for your question.
> > >
> > > Since this is more or less related to nginx development or
> > > new features I'd suggest to use nginx-devel mailing list
> > > instead, thank you.
> > >
> > > On Tue, Mar 21, 2023 at 04:06:00PM -0500, Lance Dockins wrote:
> > > >
> > > > Has anyone had success compiling Nginx with zlib-ng instead of
> > > > default Zlib versions? I seem to be able to compile Nginx with
> > > > standard Zlib and various other Zlib libraries (e.g. Intel
> > > > optimized or Cloudflare) but compiling with Zlib-NG always fails.
> > >
> > > NGINX builds well with zlib. In case of new functionality, like
> > > an ability to build with zlib-ng, the source code requires some
> > > patches.
> > >
> > > > I’ve tried passing in various options via with-zlib-opt to try
> > > > to include the —zlib-compat flag for the Zlib NG configure
> > > > directives but no matter what syntax I use, it seems like it
> > > > always fails (whether I have that param or not). Perhaps I’m
> > > > just struggling with the proper use of Zlib NG in an Nginx
> > > > compile context.
> > > >
> > > > If Nginx should compile with Zlib NG, is there any documentation
> > > > on what params to use in the Nginx compile command to get it to
> > > > work?
> > >
> > > Some ideas can be found inside the zlib-ng project on GH, not
> > > sure is that working solution or not, so you can try.
> > >
> > > I'd also recommend to raise a request in https://trac.nginx.org/nginx/
> > > about this feature request and provide patches for source code
> > > and documentation.
> > >
> > > Thank you.
> > >
> > > --
> > > Sergey A. Osokin
> > > _______________________________________________
> > > nginx mailing list
> > > nginx at nginx.org
> > > https://mailman.nginx.org/mailman/listinfo/nginx
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20230322/6769c06e/attachment-0001.htm>
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));
On Thu, 23 Mar 2023 at 04:16, Lance Dockins <lance at wordkeeper.com> wrote:
>
> Thank you, Richard. I’ll give that a shot. I already have to do that sort of patching with a variety of other things in the build that I use so that particular adjustment isn’t too bad.
>
> Just for clarity, are you saying that the hash sizes within zlib-ng have increased since Maxim’s last patch for that to accommodate zlib-ng? That patch was back in 2021 and is part of Nginx core now.
> https://mailman.nginx.org/pipermail/nginx-devel/2021-April/013945.html
>
> I think it’s coded to use a 128k hash per that patch. If the hash size has increased again since that patch, that might justify a bug report to Nginx devel. Since the code in that patch specifically relates to that error, I thought I’d ask in case you have still been seeing that error with newer Nginx versions that have come out since that patch was implemented.
>
>
> --
> Lance
>
> On Mar 22, 2023 at 5:28 PM -0500, Richard Stanway via nginx <nginx at nginx.org>, wrote:
>
> I regularly build with zlib-ng, unfortunately it requires patching the zlib-ng files to enable zlib compatibility mode as nginx doesn't seem to have a way to pass options to configure.
>
> Edit "configure" in the zlib-ng directory and change the line compat=0 to compat=1. Then specify --with-zlib=/path/to/zlib-ng in your nginx configure and you should be set.
>
> Be aware that the memory requirements of zlib-ng have changed since support for it was added to nginx, so you will see a lot of "gzip filter failed to use preallocated memory" alerts in your log file when using zlib-ng.
>
>
> On Wed, 22 Mar 2023 at 15:50, Sergey A. Osokin <osa at freebsd.org.ru> wrote:
>>
>> Hi Lance,
>>
>> thanks for your question.
>>
>> Since this is more or less related to nginx development or
>> new features I'd suggest to use nginx-devel mailing list
>> instead, thank you.
>>
>> On Tue, Mar 21, 2023 at 04:06:00PM -0500, Lance Dockins wrote:
>> >
>> > Has anyone had success compiling Nginx with zlib-ng instead of
>> > default Zlib versions? I seem to be able to compile Nginx with
>> > standard Zlib and various other Zlib libraries (e.g. Intel
>> > optimized or Cloudflare) but compiling with Zlib-NG always fails.
>>
>> NGINX builds well with zlib. In case of new functionality, like
>> an ability to build with zlib-ng, the source code requires some
>> patches.
>>
>> > I’ve tried passing in various options via with-zlib-opt to try
>> > to include the —zlib-compat flag for the Zlib NG configure
>> > directives but no matter what syntax I use, it seems like it
>> > always fails (whether I have that param or not). Perhaps I’m
>> > just struggling with the proper use of Zlib NG in an Nginx
>> > compile context.
>> >
>> > If Nginx should compile with Zlib NG, is there any documentation
>> > on what params to use in the Nginx compile command to get it to
>> > work?
>>
>> Some ideas can be found inside the zlib-ng project on GH, not
>> sure is that working solution or not, so you can try.
>>
>> I'd also recommend to raise a request in https://trac.nginx.org/nginx/
>> about this feature request and provide patches for source code
>> and documentation.
>>
>> Thank you.
>>
>> --
>> Sergey A. Osokin
>> _______________________________________________
>> nginx mailing list
>> nginx at nginx.org
>> https://mailman.nginx.org/mailman/listinfo/nginx
>
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
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.
For example, allocations for a 1k responses are as follows (note
"a:8192" in most of the lines, that is, nginx thinks these are
state allocations):
2023/03/24 03:26:10 [debug] 36809#100069: *2 http gzip filter
2023/03/24 03:26:10 [debug] 36809#100069: *2 malloc: 21DEE5C0:176144
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:6036 a:8192 p:21DEE5C0
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4180 a:8192 p:21DF05C0
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4164 a:8192 p:21DF25C0
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:131140 a:131140 p:21DF45C0
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4164 a:8192 p:21E14604
2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip in: 21C31D84
Allocations for 4k response are as follows (and generate an
alert):
2023/03/24 03:44:29 [debug] 36863#100652: *2 http gzip filter
2023/03/24 03:44:29 [debug] 36863#100652: *2 malloc: 21DEE5C0:188432
2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:6036 a:8192 p:21DEE5C0
2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:16468 a:16468 p:21DF05C0
2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:16452 a:16452 p:21DF4614
2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:131140 a:131140 p:21DF8658
2023/03/24 03:44:29 [alert] 36863#100652: *2 gzip filter failed to use preallocated memory: 16452 of 16180 while sending response to client, client: 127.0.0.1, server: one, request: "GET /t/4k HTTP/1.1", host: "127.0.0.1:8080"
2023/03/24 03:44:29 [debug] 36863#100652: *2 malloc: 21DC58C0:16452
2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip in: 21C31D98
The "+ 288" you are using should be enough to cover additional
memory used for alignment, but it is not enough to account
for misinterpretation when using gzip_comp_level above 1 (so nginx
won't allocate additional memory assuming window bits will be
adjusted to 13).
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.
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,
+ * 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;
}
--
Maxim Dounin
http://mdounin.ru/
Thanks for the patch! I've been running it for about an hour and
haven't seen the preallocated memory alert since, so it's looking good
here.
On Fri, 24 Mar 2023 at 03: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.
>
> For example, allocations for a 1k responses are as follows (note
> "a:8192" in most of the lines, that is, nginx thinks these are
> state allocations):
>
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 http gzip filter
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 malloc: 21DEE5C0:176144
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:6036 a:8192 p:21DEE5C0
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4180 a:8192 p:21DF05C0
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4164 a:8192 p:21DF25C0
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:131140 a:131140 p:21DF45C0
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip alloc: n:1 s:4164 a:8192 p:21E14604
> 2023/03/24 03:26:10 [debug] 36809#100069: *2 gzip in: 21C31D84
>
> Allocations for 4k response are as follows (and generate an
> alert):
>
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 http gzip filter
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 malloc: 21DEE5C0:188432
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:6036 a:8192 p:21DEE5C0
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:16468 a:16468 p:21DF05C0
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:16452 a:16452 p:21DF4614
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip alloc: n:1 s:131140 a:131140 p:21DF8658
> 2023/03/24 03:44:29 [alert] 36863#100652: *2 gzip filter failed to use preallocated memory: 16452 of 16180 while sending response to client, client: 127.0.0.1, server: one, request: "GET /t/4k HTTP/1.1", host: "127.0.0.1:8080"
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 malloc: 21DC58C0:16452
> 2023/03/24 03:44:29 [debug] 36863#100652: *2 gzip in: 21C31D98
>
> The "+ 288" you are using should be enough to cover additional
> memory used for alignment, but it is not enough to account
> for misinterpretation when using gzip_comp_level above 1 (so nginx
> won't allocate additional memory assuming window bits will be
> adjusted to 13).
>
> 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.
>
> 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,
> + * 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;
> }
>
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
> 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.
> + * 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?
> }
>
Looks good otherwise.
--
Sergey Kandaurov
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/