Re: [PATCH] Allow MemPool late initialization

[Amos Jeffries]

On 02/07/11 08:47, Alex Rousskov wrote:
On 06/30/2011 06:13 AM, Amos Jeffries wrote:
The problem:
On 24/06/11 02:43, Kinkie wrote:
Hi all,

2011/06/23 16:38:39 kid3| assertion failed: mem.cc:516: "MemPools[t]"

at startup


FQDN had memDataInit() in its component setup, which moved. The assert
is in memCheckInit() and tests that memInit() worked properly. But if a
pool is initialized only when its component is loaded, that check will
fail on several conditions unrelated to the operation of memory.
Seemingly trivial changes to component loading order is one case.

This patch allows modules to initialize/register their own pools on
demand later in the startup process.

* shuffle MEM_DONTFREE which is an existing fixed entry that must not be
memInitCheck()'d to the end of the MemPool type enum list.

* update memCheckInit() to stop scanning for missing pools at that marker.

* shuffle pool types which are initialized by their components after the
marker value. Such that no false problem is reported if (a) the
component is never initialized for that worker, or (b) the component is
only initialized during the configuration process.

* document this layout significance in the enum list to aid future pool
additions or moves.

* add asserts to memAllocate() and memFree() to highlight the cases of
brokenness memCheckInit() was catching. Using assert() instead of if()
so that optimized builds can avoid the penalty of an extra test on each
alloc/free.

+    ++t;
+
+    do {
...
-    }
+    } while(++t<  MEM_DONTFREE);


I think the above can be shortened/clarified as

     while (++t<  MEM_DONTFREE) { ... }

or even go back to the old for-loop that was there, but stop it at
MEM_DONTFREE.


Thank you. Seems to work.
Amended patch attached.


NP: so far this patch gets me past the assert Kinkie hit (and two others
similar). Then dies in Strand.cc (2 workers, minimal config) with a
Segmentation fault that should not exist and cannot be replicated using -N.

Do you get the same segmentation fault if you undo your patch but
disable memCheckInit() so that the latter is not in the way?

Yes, simple #if 0...#endif  around the content of memCheckInit() leads 
to the same SegFault in 3.2. Opened bug 3264 about this.

Amos
--
Please be using
  Current Stable Squid 2.7.STABLE9 or 3.1.14
  Beta testers wanted for 3.2.0.9
=== modified file 'src/enums.h'
--- src/enums.h	2011-03-18 11:36:50 +0000
+++ src/enums.h	2011-07-04 10:59:09 +0000
@@ -216,21 +216,23 @@
     MEM_CLIENT_INFO,
     MEM_LINK_LIST,
     MEM_DLINK_NODE,
-    MEM_DONTFREE,
     MEM_DREAD_CTRL,
     MEM_DWRITE_Q,
-    MEM_FQDNCACHE_ENTRY,
-    MEM_FWD_SERVER,
     MEM_HTTP_HDR_CC,
     MEM_HTTP_HDR_CONTENT_RANGE,
-    MEM_IPCACHE_ENTRY,
     MEM_MD5_DIGEST,
     MEM_NETDBENTRY,
     MEM_NET_DB_NAME,
     MEM_RELIST,
+    // IMPORTANT: leave this here. pools above are initialized early with memInit()
+    MEM_DONTFREE,
+    // following pools are initialized late by their component if needed (or never)
+    MEM_FQDNCACHE_ENTRY,
+    MEM_FWD_SERVER,
 #if !USE_DNSSERVERS
     MEM_IDNS_QUERY,
 #endif
+    MEM_IPCACHE_ENTRY,
     MEM_MAX
 } mem_type;
 

=== modified file 'src/mem.cc'
--- src/mem.cc	2011-06-22 12:21:39 +0000
+++ src/mem.cc	2011-07-04 10:59:43 +0000
@@ -202,6 +202,7 @@
 void *
 memAllocate(mem_type type)
 {
+    assert(MemPools[type]);
     return MemPools[type]->alloc();
 }
 
@@ -209,6 +210,7 @@
 void
 memFree(void *p, int type)
 {
+    assert(MemPools[type]);
     MemPools[type]->freeOne(p);
 }
 
@@ -503,15 +505,13 @@
 void
 memCheckInit(void)
 {
-    mem_type t;
-
-    for (t = MEM_NONE, ++t; t < MEM_MAX; ++t) {
-        if (MEM_DONTFREE == t)
-            continue;
-
+    mem_type t = MEM_NONE;
+
+    while (++t < MEM_DONTFREE) {
         /*
          * If you hit this assertion, then you forgot to add a
          * memDataInit() line for type 't'.
+         * Or placed the pool type in the wrong section of the enum list.
          */
         assert(MemPools[t]);
     }