Fixed shared memory cleanup code -- we were not returning freed pages to Pages.

Added Ipc::StoreMapCleaner API so that map users are notified when the slot is
about to be overwritten or freed. Users need a chance to update their state
(e.g., return the no longer used shared page) before the extra information in
the slot disappears.

diff --git a/src/MemStore.cc b/src/MemStore.cc
index b8f0d0760dc327b1fb4f1ce368a33171c961b43b..e6bec8bf4cddf24ae035f39f47aa9a40736b8888 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -33,8 +33,10 @@ MemStore::init()
         const int64_t entrySize = Ipc::Mem::PageSize(); // for now
         const int64_t entryCount = Config.memMaxSize / entrySize;
         // TODO: warn if we cannot cache at least one item (misconfiguration)
-        if (entryCount > 0)
+        if (entryCount > 0) {
             map = new MemStoreMap("cache_mem", entryCount);
+            map->cleaner = this;
+        }
     }
 }
 
@@ -295,3 +297,10 @@ MemStore::copyToShm(StoreEntry &e, MemStoreMap::Extras &extras)
     extras.storedSize = copied;
     return true;
 }
+
+void
+MemStore::cleanReadable(const sfileno fileno)
+{
+    Ipc::Mem::PutPage(map->extras(fileno).page);
+}
+

diff --git a/src/MemStore.h b/src/MemStore.h
index bb4320bba62c9eed7fb60ce4ee43d0a9f0631500..547f07c83c8c4b9e2a49a9e3022170f408630f08 100644 (file)
--- a/src/MemStore.h
+++ b/src/MemStore.h
@@ -10,7 +10,7 @@
 
 /// Stores HTTP entities in RAM. Current implementation uses shared memory.
 /// Unlike a disk store (SwapDir), operations are synchronous (and fast).
-class MemStore: public Store {
+class MemStore: public Store, public Ipc::StoreMapCleaner {
 public:
     MemStore();
     virtual ~MemStore();
@@ -39,6 +39,9 @@ protected:
     bool copyToShm(StoreEntry &e, MemStoreMap::Extras &extras);
     bool copyFromShm(StoreEntry &e, const MemStoreMap::Extras &extras);
 
+    // Ipc::StoreMapCleaner API
+    virtual void cleanReadable(const sfileno fileno);
+
 private:
     MemStoreMap *map; ///< index of mem-cached entries
 };

diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index ec300446dde9d7082a25f75f77c86c8f1421675d..de92915bbbf79043daa03c6ba079cd6ce130f1e1 100644 (file)
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -11,7 +11,7 @@
 
 Ipc::StoreMap::StoreMap(const char *const aPath, const int limit,
     size_t sharedSizeExtra):
-    path(aPath), shm(aPath), shared(NULL)
+    cleaner(NULL), path(aPath), shm(aPath), shared(NULL)
 {
     const size_t mySharedSize = Shared::MemSize(limit);
     shm.create(mySharedSize + sharedSizeExtra);
@@ -20,7 +20,7 @@ Ipc::StoreMap::StoreMap(const char *const aPath, const int limit,
 }
 
 Ipc::StoreMap::StoreMap(const char *const aPath):
-    path(aPath), shm(aPath), shared(NULL)
+    cleaner(NULL), path(aPath), shm(aPath), shared(NULL)
 {
     shm.open();
     assert(shm.mem());
@@ -43,12 +43,12 @@ Ipc::StoreMap::openForWriting(const cache_key *const key, sfileno &fileno)
     if (lock.lockExclusive()) {
         assert(s.state != Slot::Writeable); // until we start breaking locks
 
-        // free if the entry was dirty, keeping the entry locked
-        if (s.waitingToBeFreed == true)
+        // free if the entry was used, keeping the entry locked
+        if (s.waitingToBeFreed == true || s.state == Slot::Readable)
             freeLocked(s, true);
 
-        if (s.state == Slot::Empty) // we may also overwrite a Readable slot
-            ++shared->count;
+        assert(s.state == Slot::Empty);
+        ++shared->count;
         s.state = Slot::Writeable;
         fileno = idx;
         //s.setKey(key); // XXX: the caller should do that
@@ -253,6 +253,9 @@ Ipc::StoreMap::slotByKey(const cache_key *const key)
 void
 Ipc::StoreMap::freeLocked(Slot &s, bool keepLocked)
 {
+    if (s.state == Slot::Readable && cleaner)
+        cleaner->cleanReadable(&s - shared->slots);
+
     s.waitingToBeFreed = false;
     s.state = Slot::Empty;
     if (!keepLocked)

diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h
index 3489d2891f1f9dc5b399f8e9efb5ce7f5e7265c6..9ca540f625796f93490b096e00d96e443e3c6480 100644 (file)
--- a/src/ipc/StoreMap.h
+++ b/src/ipc/StoreMap.h
@@ -3,6 +3,7 @@
 
 #include "ipc/ReadWriteLock.h"
 #include "ipc/mem/Segment.h"
+#include "typedefs.h"
 
 namespace Ipc {
 
@@ -43,6 +44,8 @@ public:
     State state; ///< current state
 };
 
+class StoreMapCleaner;
+
 /// map of StoreMapSlots indexed by their keys, with read/write slot locking 
 /// kids extend to store custom data
 class StoreMap
@@ -82,6 +85,8 @@ public:
     /// adds approximate current stats to the supplied ones
     void updateStats(ReadWriteLockStats &stats) const;
 
+    StoreMapCleaner *cleaner; ///< notified before a readable entry is freed
+
 protected:
     class Shared {
     public:
@@ -112,6 +117,17 @@ private:
     Shared *shared; ///< pointer to shared memory
 };
 
+/// API for adjusting external state when dirty map slot is being freed
+class StoreMapCleaner
+{
+public:
+    virtual ~StoreMapCleaner() {}
+
+    /// adjust slot-linked state before a locked Readable slot is erased
+    virtual void cleanReadable(const sfileno fileno) = 0;
+};
+
+
 } // namespace Ipc
 
 // We do not reuse struct _fileMap because we cannot control its size,