Centralized PagePool/PageStack ID generation (#525)

Easy-to-find-in-cache.log and predictable/stable stack IDs for shared
memory pages and/or index slot numbers are very useful when debugging
cache metadata corruption issues because they allow to track related
(e.g. same-stack) operations across huge SMP logs.

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 93a9807dba87512543d0fdf2ecf7750dbd18b39f..c6081ec469cf41c96183ec6bf7523a57e6dcd412 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -30,11 +30,6 @@ static const char *SpaceLabel = "cache_mem_space";
 static const char *ExtrasLabel = "cache_mem_ex";
 // TODO: sync with Rock::SwapDir::*Path()
 
-// We store free slot IDs (i.e., "space") as Page objects so that we can use
-// Ipc::Mem::PageStack. Pages require pool IDs. The value here is not really
-// used except for a positivity test. A unique value is handy for debugging.
-static const uint32_t SpacePoolId = 510716;
-
 /// Packs to shared memory, allocating new slots/pages as needed.
 /// Requires an Ipc::StoreMapAnchor locked for writing.
 class ShmWriter: public Packable
@@ -810,7 +805,7 @@ MemStore::noteFreeMapSlice(const Ipc::StoreMapSliceId sliceId)
     debugs(20, 9, "slice " << sliceId << " freed " << pageId);
     assert(pageId);
     Ipc::Mem::PageId slotId;
-    slotId.pool = SpacePoolId;
+    slotId.pool = Ipc::Mem::PageStack::IdForMemStoreSpace();
     slotId.number = sliceId + 1;
     if (!waitingFor) {
         // must zero pageId before we give slice (and pageId extras!) to others
@@ -1014,7 +1009,8 @@ MemStoreRr::create()
     const int64_t entryLimit = MemStore::EntryLimit();
     assert(entryLimit > 0);
     Must(!spaceOwner);
-    spaceOwner = shm_new(Ipc::Mem::PageStack)(SpaceLabel, SpacePoolId,
+    spaceOwner = shm_new(Ipc::Mem::PageStack)(SpaceLabel,
+                 Ipc::Mem::PageStack::IdForMemStoreSpace(),
                  entryLimit, 0);
     Must(!mapOwner);
     mapOwner = MemStoreMap::Init(MapLabel, entryLimit);

diff --git a/src/fs/rock/RockRebuild.cc b/src/fs/rock/RockRebuild.cc
index 00e0cb6ccb43e0227bbe56e6736b38aef92adc80..b83c367a4ae78ed230bd17ffc8121634e5282b2b 100644 (file)
--- a/src/fs/rock/RockRebuild.cc
+++ b/src/fs/rock/RockRebuild.cc
@@ -593,7 +593,7 @@ Rock::Rebuild::freeSlot(const SlotId slotId, const bool invalid)
     }
 
     Ipc::Mem::PageId pageId;
-    pageId.pool = sd->index+1;
+    pageId.pool = Ipc::Mem::PageStack::IdForSwapDirSpace(sd->index);
     pageId.number = slotId+1;
     sd->freeSlots->push(pageId);
 }

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index 6e77b63f894effab61b004726cc3a2e9044e818a..b2fa699100f3dd6ea024fa3867a187a63f3fd986 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -754,7 +754,7 @@ void
 Rock::SwapDir::noteFreeMapSlice(const Ipc::StoreMapSliceId sliceId)
 {
     Ipc::Mem::PageId pageId;
-    pageId.pool = index+1;
+    pageId.pool = Ipc::Mem::PageStack::IdForSwapDirSpace(index);
     pageId.number = sliceId+1;
     if (waitingForPage) {
         *waitingForPage = pageId;
@@ -1138,7 +1138,9 @@ void Rock::SwapDirRr::create()
             // TODO: somehow remove pool id and counters from PageStack?
             Ipc::Mem::Owner<Ipc::Mem::PageStack> *const freeSlotsOwner =
                 shm_new(Ipc::Mem::PageStack)(sd->freeSlotsPath(),
-                                             i+1, capacity, 0);
+                                             Ipc::Mem::PageStack::IdForSwapDirSpace(i),
+                                             capacity,
+                                             0);
             freeSlotsOwners.push_back(freeSlotsOwner);
 
             // TODO: add method to initialize PageStack with no free pages

diff --git a/src/ipc/Makefile.am b/src/ipc/Makefile.am
index f4ac2899627d6cdee026dadfe641ee615759d962..9b3402768248b0ed4a56f567e91bc84ecf2a3dfc 100644 (file)
--- a/src/ipc/Makefile.am
+++ b/src/ipc/Makefile.am
@@ -64,7 +64,8 @@ libipc_la_SOURCES = \
        mem/PageStack.h \
        mem/Pointer.h \
        mem/Segment.cc \
-       mem/Segment.h
+       mem/Segment.h \
+       mem/forward.h
 
 DEFS += -DDEFAULT_STATEDIR=\"$(localstatedir)/run/squid\"
 

diff --git a/src/ipc/mem/Page.h b/src/ipc/mem/Page.h
index a9e36db0a51c29b889b88be053587d5151f35ae5..3c42e2f732df385c68d165fc39d35194eef37e82 100644 (file)
--- a/src/ipc/mem/Page.h
+++ b/src/ipc/mem/Page.h
@@ -9,6 +9,8 @@
 #ifndef SQUID_IPC_MEM_PAGE_H
 #define SQUID_IPC_MEM_PAGE_H
 
+#include "ipc/mem/forward.h"
+
 #include <iosfwd>
 
 namespace Ipc
@@ -30,7 +32,12 @@ public:
     typedef const uint32_t PageId::*SaferBool;
     operator SaferBool() const { return set() ? &PageId::number : NULL; }
 
-    uint32_t pool; ///< page pool ID within Squid
+    /// The ID of a PagePool (and/or PageStack) this page belongs to.
+    /// Positive values are (ab)used to detect in-use pages. See set().
+    /// Eventually, they may identify a PageStack in a multi-segment PagePool.
+    /// These IDs also distinguish page pools/stacks in debugging logs.
+    PoolId pool;
+
     // uint32_t segment; ///< memory segment ID within the pool; unused for now
     uint32_t number; ///< page number within the segment
 

diff --git a/src/ipc/mem/PagePool.cc b/src/ipc/mem/PagePool.cc
index 09846c1662f9736882c24634dbcd3898108e7fcc..292f2870a9f458d3943775298ee7550d07cc95af 100644 (file)
--- a/src/ipc/mem/PagePool.cc
+++ b/src/ipc/mem/PagePool.cc
@@ -16,12 +16,9 @@
 // Ipc::Mem::PagePool
 
 Ipc::Mem::PagePool::Owner *
-Ipc::Mem::PagePool::Init(const char *const id, const unsigned int capacity, const size_t pageSize)
+Ipc::Mem::PagePool::Init(const char *const shmId, const Ipc::Mem::PoolId stackId, const unsigned int capacity, const size_t pageSize)
 {
-    static uint32_t LastPagePoolId = 0;
-    if (++LastPagePoolId == 0)
-        ++LastPagePoolId; // skip zero pool id
-    return shm_new(PageStack)(id, LastPagePoolId, capacity, pageSize);
+    return shm_new(PageStack)(shmId, stackId, capacity, pageSize);
 }
 
 Ipc::Mem::PagePool::PagePool(const char *const id):

diff --git a/src/ipc/mem/PagePool.h b/src/ipc/mem/PagePool.h
index 61a433ef12ef8495cdf002942f465f76de270bd5..479c9f4186206d380d1e9cae1b66b7b0f1ab8f39 100644 (file)
--- a/src/ipc/mem/PagePool.h
+++ b/src/ipc/mem/PagePool.h
@@ -27,7 +27,7 @@ class PagePool
 public:
     typedef Ipc::Mem::Owner<PageStack> Owner;
 
-    static Owner *Init(const char *const id, const unsigned int capacity, const size_t pageSize);
+    static Owner *Init(const char *const shmId, const Ipc::Mem::PoolId stackId, const unsigned int capacity, const size_t pageSize);
 
     PagePool(const char *const id);
 

diff --git a/src/ipc/mem/PageStack.cc b/src/ipc/mem/PageStack.cc
index f3149064c3e27e3924fa5165e2244d6381112ddb..1f02e35f386077fe5fbc38afb0b9538896e40fbe 100644 (file)
--- a/src/ipc/mem/PageStack.cc
+++ b/src/ipc/mem/PageStack.cc
@@ -37,12 +37,14 @@ Ipc::Mem::PageStackStorageSlot::put(const PointerOrMarker expected, const Pointe
 
 /* Ipc::Mem::PageStack */
 
-Ipc::Mem::PageStack::PageStack(const uint32_t aPoolId, const PageCount aCapacity, const size_t aPageSize):
+Ipc::Mem::PageStack::PageStack(const PoolId aPoolId, const PageCount aCapacity, const size_t aPageSize):
     thePoolId(aPoolId), capacity_(aCapacity), thePageSize(aPageSize),
     size_(0),
     head_(Slot::NilPtr),
     slots_(aCapacity)
 {
+    assert(thePoolId);
+
     assert(capacity_ < Slot::TakenPage);
     assert(capacity_ < Slot::NilPtr);
 
@@ -123,7 +125,7 @@ Ipc::Mem::PageStack::sharedMemorySize() const
 }
 
 size_t
-Ipc::Mem::PageStack::SharedMemorySize(const uint32_t, const PageCount capacity, const size_t pageSize)
+Ipc::Mem::PageStack::SharedMemorySize(const PoolId, const PageCount capacity, const size_t pageSize)
 {
     const auto levelsSize = PageId::maxPurpose * sizeof(Levels_t);
     const size_t pagesDataSize = capacity * pageSize;

diff --git a/src/ipc/mem/PageStack.h b/src/ipc/mem/PageStack.h
index eb761fb92bae2945393055d6597ac9f77fbf1a08..11e2921465a05e0eb5e65135e716026e7f883a9e 100644 (file)
--- a/src/ipc/mem/PageStack.h
+++ b/src/ipc/mem/PageStack.h
@@ -10,6 +10,7 @@
 #define SQUID_IPC_MEM_PAGE_STACK_H
 
 #include "ipc/mem/FlexibleArray.h"
+#include "ipc/mem/forward.h"
 
 #include <atomic>
 #include <limits>
@@ -71,7 +72,7 @@ public:
     /// the number of (free and/or used) pages in a stack
     typedef unsigned int PageCount;
 
-    PageStack(const uint32_t aPoolId, const PageCount aCapacity, const size_t aPageSize);
+    PageStack(const PoolId aPoolId, const PageCount aCapacity, const size_t aPageSize);
 
     PageCount capacity() const { return capacity_; }
     size_t pageSize() const { return thePageSize; }
@@ -86,7 +87,7 @@ public:
     bool pageIdIsValid(const PageId &page) const;
 
     /// total shared memory size required to share
-    static size_t SharedMemorySize(const uint32_t aPoolId, const PageCount capacity, const size_t pageSize);
+    static size_t SharedMemorySize(const PoolId aPoolId, const PageCount capacity, const size_t pageSize);
     size_t sharedMemorySize() const;
 
     /// shared memory size required only by PageStack, excluding
@@ -98,13 +99,26 @@ public:
     static size_t LevelsPaddingSize(const PageCount capacity);
     size_t levelsPaddingSize() const { return LevelsPaddingSize(capacity_); }
 
+    /**
+     * The following functions return PageStack IDs for the corresponding
+     * PagePool or a similar PageStack user. The exact values are unimportant,
+     * but their uniqueness and stability eases debugging.
+     */
+
+    /// stack of free cache_mem slot positions
+    static PoolId IdForMemStoreSpace() { return 10; }
+    /// multipurpose PagePool of shared memory pages
+    static PoolId IdForMultipurposePool() { return 200; } // segments could use 2xx
+    /// stack of free rock cache_dir slot numbers
+    static PoolId IdForSwapDirSpace(const int dirIdx) { return 900 + dirIdx + 1; }
+
 private:
     using Slot = PageStackStorageSlot;
 
     // XXX: theFoo members look misplaced due to messy separation of PagePool
     // (which should support multiple Segments but does not) and PageStack
     // (which should not calculate the Segment size but does) duties.
-    const uint32_t thePoolId; ///< pool ID
+    const PoolId thePoolId; ///< pool ID
     const PageCount capacity_; ///< the maximum number of pages
     const size_t thePageSize; ///< page size, used to calculate shared memory size
     /// a lower bound for the number of free pages (for debugging purposes)

diff --git a/src/ipc/mem/Pages.cc b/src/ipc/mem/Pages.cc
index f8b5f6b3f831fecd53b9f874df3d452d730e2310..8dfa0957d9825ad39ce9e2d6154cda63b33c4966 100644 (file)
--- a/src/ipc/mem/Pages.cc
+++ b/src/ipc/mem/Pages.cc
@@ -118,7 +118,9 @@ void
 SharedMemPagesRr::create()
 {
     Must(!owner);
-    owner = Ipc::Mem::PagePool::Init(PagePoolId, Ipc::Mem::PageLimit(),
+    owner = Ipc::Mem::PagePool::Init(PagePoolId,
+                                     Ipc::Mem::PageStack::IdForMultipurposePool(),
+                                     Ipc::Mem::PageLimit(),
                                      Ipc::Mem::PageSize());
 }
 

diff --git a/src/ipc/mem/forward.h b/src/ipc/mem/forward.h
new file mode 100644 (file)
index 0000000..c44f4ce
--- /dev/null
+++ b/src/ipc/mem/forward.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 1996-2019 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_IPC_MEM_FORWARD_H
+#define SQUID_IPC_MEM_FORWARD_H
+
+namespace Ipc
+{
+
+namespace Mem
+{
+    typedef uint32_t PoolId;
+} // namespace Mem
+
+} // namespace Ipc
+
+#endif /* SQUID_IPC_MEM_FORWARD_H */
+