<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN">
<html><body style='font-size: 10pt; font-family: Verdana,Geneva,sans-serif'>
<p>Hi,</p>
<p>Your patch looks very interesting.</p>
<p>But small comment by the way:</p>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">
<pre>So the answer is "no", right? ...</pre>
</blockquote>
<p>I had no problem with reaload at all... What I do so differently?<br />The last test was repeated 2000 times: added and removed up to 10 zones and reload with '-s reload'.<br />I see no problem. ???</p>
<p>Am 27.04.2015 03:25, schrieb Maxim Dounin:</p>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px"><!-- html ignored --><!-- head ignored --><!-- meta ignored -->
<pre>Hello!
On Fri, Apr 24, 2015 at 01:21:41AM +0200, Sergey Brester wrote:</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">Hello,
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">There are lots of style problems which need cleanup.</blockquote>
The newer, "nginx-style" compliant version of changeset (shmem fix2.patch) was already posted to nginx-devel (Thx, Filipe DA SILVA). You will find it also on under <a href="https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04">https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04</a> [4].</blockquote>
<pre>It still needs cleanup, but I don't think it worth detailed
comments as there are more important things to be addressed.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">Such things belong to ngx_win32_init.c. It may be also good enough to use ngx_pagesize, which is already set there.</blockquote>
Agree, but some (little) things can reside in same module, where these are used (noted as todo).
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">This probably should be somewhere at ngx_win32_config.h.</blockquote>
Imho, belong definitelly in shmem module. But don't want to fight about it :)</blockquote>
<pre>The question here is a balance between code locality and
visibility of platform-dependent constants for modifications. E.g.,
we've recently added 64-bit constants into ngx_win32_config.h
to allow compilation of 64-bit windows binaries - and I'm
perfectly sure shmem code would have been forgotten assuming it
used just magic addresses as in your patch.
May be some constants with appropriate comments at the top of the
ngx_shmem.c file will be a good compromise solution.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">It might be better idea to save the address at the end of the memory block allocated...</blockquote>
And for example stop to work of other new worker (when unfortunately overwriting of it occurs). That brings at once many problem and restrictions later (for example more difficult implementing of shared area resizing, etc). And for what? To save 2 line code for decrement addr pointer in ngx_shm_free?</blockquote>
<pre>It won't be overwritten as long as you allocate additional bytes
for it (as of now) and shm->size is not incremented. And yes,
saving 2 lines of code is important - especially if it happens
because the whole approach becomes simplier.
Anyway, I think that we should try to use the address which is
already present, see other comments. This will also avoid
unneeded restrictions on mappings where pointers are not stored.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px">Have you tried using an arbitrary addresses (with subsequent remap using proper base address for already existing mappings)? Does is work noticeably worse?</blockquote>
I've fought against ASLR since 2008 resp. x64 was released and it was essential. This specific solution is extensively tested and runs in production also without any problems (not only in nginx).</blockquote>
<pre>So the answer is "no", right? Either way, I tried it myself, and
it seems to be noticeably worse - in my tests in most cases it
can't use the address from the master process in workers due to
conflicts with other allocations. When combined with base address
selection, it seems to be much more stable.
Below is a patch which takes into account above comments, and
also fixes a problem observed with your patch on configuration
reloads, e.g., when something like:
proxy_cache_path cache1 keys_zone=cache1:10m;
is changed to
proxy_cache_path cache0 keys_zone=cache0:10m;
proxy_cache_path cache1 keys_zone=cache1:10m;
(i.e., when mappings are created in master and worker processes in
a different order, resulting in conflicts between a mapping we are
trying to create at some base address with a mapping already
remapped to this address).
Review, comments and testing appreciated.
# HG changeset patch
# User Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>>
# Date 1430097438 -10800
# Mon Apr 27 04:17:18 2015 +0300
# Node ID 89cd9c63c58e5d7c474d317f83584779e2342ee3
# Parent 859ce1c41f642c39d2db9741bdd305f3ee6507f5
Win32: shared memory base addresses and remapping.
Two mechanisms are implemented to make it possible to store pointers
in shared memory on Windows, in particular on Windows Vista and later
versions with ASLR:
- The ngx_shm_remap() function added to allow remapping of a shared memory
zone to the address originally used for it in the master process. While
important, it doesn't solve the problem by itself as in many cases it's
not possible to use the address because of conflicts with other
allocations.
- We now create mappings at the same address in all processes by starting
mappings at predefined addresses normally unused by newborn processes.
These two mechanisms combined allow to use shared memory on Windows
almost without problems, including reloads.
Based on the patch by Sergey Brester:
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2015-April/006836.html">http://mailman.nginx.org/pipermail/nginx-devel/2015-April/006836.html</a>
diff -r 859ce1c41f64 -r 89cd9c63c58e src/core/ngx_cycle.c
--- a/src/core/ngx_cycle.c Mon Apr 27 03:44:30 2015 +0300
+++ b/src/core/ngx_cycle.c Mon Apr 27 04:17:18 2015 +0300
@@ -863,6 +863,22 @@ ngx_init_zone_pool(ngx_cycle_t *cycle, n
return NGX_OK;
}
+#if (NGX_WIN32)
+
+ /* remap at the required address */
+
+ if (ngx_shm_remap(&zn->shm, sp->addr) != NGX_OK) {
+ return NGX_ERROR;
+ }
+
+ sp = (ngx_slab_pool_t *) zn->shm.addr;
+
+ if (sp == sp->addr) {
+ return NGX_OK;
+ }
+
+#endif
+
ngx_log_error(NGX_LOG_EMERG, cycle->log, 0,
"shared zone \"%V\" has no equal addresses: %p vs %p",
&zn->shm.name, sp->addr, sp);
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_shmem.c
--- a/src/os/win32/ngx_shmem.c Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_shmem.c Mon Apr 27 04:17:18 2015 +0300
@@ -9,11 +9,44 @@
#include <ngx_core.h>
+/*
+ * Base addresses selected by system for shared memory mappings are likely
+ * to be different on Windows Vista and later versions due to address space
+ * layout randomization. This is however incompatible with storing absolute
+ * addresses within the shared memory.
+ *
+ * To make it possible to store absolute addresses we create mappings
+ * at the same address in all processes by starting mappings at predefined
+ * addresses. The addresses was selected somewhat randomly in order to
+ * minimize the probability that some other library doing something similar
+ * conflicts with us. The addresses are from the following typically free
+ * blocks:
+ *
+ * - 0x10000000 .. 0x70000000 (about 1.8 GB in total) on 32-bit platforms
+ * - 0x000000007fff0000 .. 0x000007f68e8b0000 (about 8 TB) on 64-bit platforms
+ *
+ * Additionally, we allow to change the mapping address once it was detected
+ * to be different from one originally used. This is needed to support
+ * reconfiguration.
+ */
+
+
+#ifdef _WIN64
+#define NGX_SHMEM_BASE 0x0000047047e00000
+#else
+#define NGX_SHMEM_BASE 0x2efe0000
+#endif
+
+
+ngx_uint_t ngx_allocation_granularity;
+
+
ngx_int_t
ngx_shm_alloc(ngx_shm_t *shm)
{
- u_char *name;
- uint64_t size;
+ u_char *name;
+ uint64_t size;
+ static u_char *base = (u_char *) NGX_SHMEM_BASE;
name = ngx_alloc(shm->name.len + 2 + NGX_INT32_LEN, shm->log);
if (name == NULL) {
@@ -46,6 +79,27 @@ ngx_shm_alloc(ngx_shm_t *shm)
shm->exists = 1;
}
+ shm->addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, base);
+
+ if (shm->addr != NULL) {
+ base += ngx_align(size, ngx_allocation_granularity);
+ return NGX_OK;
+ }
+
+ ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, ngx_errno,
+ "MapViewOfFileEx(%uz, %p) of file mapping \"%V\" failed, "
+ "retry without a base address",
+ shm->size, base, &shm->name);
+
+ /*
+ * Order of shared memory zones may be different in the master process
+ * and worker processes after reconfiguration. As a result, the above
+ * may fail due to a conflict with a previously created mapping remapped
+ * to a different address. Additionally, there may be a conflict with
+ * some other uses of the memory. In this case we retry without a base
+ * address to let the system assign the address itself.
+ */
+
shm->addr = MapViewOfFile(shm->handle, FILE_MAP_WRITE, 0, 0, 0);
if (shm->addr != NULL) {
@@ -66,6 +120,30 @@ ngx_shm_alloc(ngx_shm_t *shm)
}
+ngx_int_t
+ngx_shm_remap(ngx_shm_t *shm, u_char *addr)
+{
+ if (UnmapViewOfFile(shm->addr) == 0) {
+ ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
+ "UnmapViewOfFile(%p) of file mapping \"%V\" failed",
+ shm->addr, &shm->name);
+ return NGX_ERROR;
+ }
+
+ shm->addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, addr);
+
+ if (shm->addr != NULL) {
+ return NGX_OK;
+ }
+
+ ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
+ "MapViewOfFileEx(%uz, %p) of file mapping \"%V\" failed",
+ shm->size, addr, &shm->name);
+
+ return NGX_ERROR;
+}
+
+
void
ngx_shm_free(ngx_shm_t *shm)
{
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_shmem.h
--- a/src/os/win32/ngx_shmem.h Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_shmem.h Mon Apr 27 04:17:18 2015 +0300
@@ -24,7 +24,10 @@ typedef struct {
ngx_int_t ngx_shm_alloc(ngx_shm_t *shm);
+ngx_int_t ngx_shm_remap(ngx_shm_t *shm, u_char *addr);
void ngx_shm_free(ngx_shm_t *shm);
+extern ngx_uint_t ngx_allocation_granularity;
+
#endif /* _NGX_SHMEM_H_INCLUDED_ */
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_win32_init.c
--- a/src/os/win32/ngx_win32_init.c Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_win32_init.c Mon Apr 27 04:17:18 2015 +0300
@@ -118,6 +118,7 @@ ngx_os_init(ngx_log_t *log)
GetSystemInfo(&si);
ngx_pagesize = si.dwPageSize;
+ ngx_allocation_granularity = si.dwAllocationGranularity;
ngx_ncpu = si.dwNumberOfProcessors;
ngx_cacheline_size = NGX_CPU_CACHE_LINE;
</pre>
</blockquote>
</body></html>