cleaned up the interface. no need for use posix in some of the SHM APIs.

guptask · guptask · commit 2eb9eacefa6a · 2022-06-08T17:17:09.000-07:00
diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h
@@ -68,26 +68,24 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
       accessContainer_(std::make_unique<AccessContainer>(
           config_.accessConfig,
           shmManager_
-              ->createShm(detail::kShmHashTableName,
-                          AccessContainer::getRequiredSize(
-                              config_.accessConfig.getNumBuckets()),
-                          nullptr,
-                          ShmSegmentOpts(config_.accessConfig.getPageSize(),
-                                         false,
-                                         config_.usePosixShm))
+              ->createShm(
+                  detail::kShmHashTableName,
+                  AccessContainer::getRequiredSize(
+                      config_.accessConfig.getNumBuckets()),
+                  nullptr,
+                  ShmSegmentOpts(config_.accessConfig.getPageSize(), false))
               .addr,
           compressor_,
           [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemAccessContainer_(std::make_unique<AccessContainer>(
           config_.chainedItemAccessConfig,
           shmManager_
-              ->createShm(detail::kShmChainedItemHashTableName,
-                          AccessContainer::getRequiredSize(
-                              config_.chainedItemAccessConfig.getNumBuckets()),
-                          nullptr,
-                          ShmSegmentOpts(config_.accessConfig.getPageSize(),
-                                         false,
-                                         config_.usePosixShm))
+              ->createShm(
+                  detail::kShmChainedItemHashTableName,
+                  AccessContainer::getRequiredSize(
+                      config_.chainedItemAccessConfig.getNumBuckets()),
+                  nullptr,
+                  ShmSegmentOpts(config_.accessConfig.getPageSize(), false))
               .addr,
           compressor_,
           [this](Item* it) -> WriteHandle { return acquire(it); })),
@@ -103,8 +101,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
                      config_.isNvmCacheEncryptionEnabled(),
                      config_.isNvmCacheTruncateAllocSizeEnabled()} {
   initCommon(false);
-  shmManager_->removeShm(detail::kShmInfoName,
-                         PosixSysVSegmentOpts(config_.usePosixShm));
+  shmManager_->removeShm(detail::kShmInfoName);
 }
 
 template <typename CacheTrait>
@@ -122,19 +119,17 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
       accessContainer_(std::make_unique<AccessContainer>(
           deserializer_->deserialize<AccessSerializationType>(),
           config_.accessConfig,
-          shmManager_->attachShm(
-              detail::kShmHashTableName,
-              nullptr,
-              ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),
+          shmManager_->attachShm(detail::kShmHashTableName,
+                                 nullptr,
+                                 ShmSegmentOpts(PageSizeT::NORMAL, false)),
           compressor_,
           [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemAccessContainer_(std::make_unique<AccessContainer>(
           deserializer_->deserialize<AccessSerializationType>(),
           config_.chainedItemAccessConfig,
-          shmManager_->attachShm(
-              detail::kShmChainedItemHashTableName,
-              nullptr,
-              ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),
+          shmManager_->attachShm(detail::kShmChainedItemHashTableName,
+                                 nullptr,
+                                 ShmSegmentOpts(PageSizeT::NORMAL, false)),
           compressor_,
           [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemLocks_(config_.chainedItemsLockPower,
@@ -155,8 +150,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
   // We will create a new info shm segment on shutDown(). If we don't remove
   // this info shm segment here and the new info shm segment's size is larger
   // than this one, creating new one will fail.
-  shmManager_->removeShm(detail::kShmInfoName,
-                         PosixSysVSegmentOpts(config_.usePosixShm));
+  shmManager_->removeShm(detail::kShmInfoName);
 }
 
 template <typename CacheTrait>
@@ -174,7 +168,8 @@ std::unique_ptr<MemoryAllocator>
 CacheAllocator<CacheTrait>::createNewMemoryAllocator() {
   ShmSegmentOpts opts;
   opts.alignment = sizeof(Slab);
-  opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
+  opts.typeOpts =
+      config_.usePosixShm ? PosixShmSegmentOpts{} : SysVShmSegmentOpts{};
   return std::make_unique<MemoryAllocator>(
       getAllocatorConfig(config_),
       shmManager_
@@ -189,7 +184,8 @@ std::unique_ptr<MemoryAllocator>
 CacheAllocator<CacheTrait>::restoreMemoryAllocator() {
   ShmSegmentOpts opts;
   opts.alignment = sizeof(Slab);
-  opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
+  opts.typeOpts =
+      config_.usePosixShm ? PosixShmSegmentOpts{} : SysVShmSegmentOpts{};
   return std::make_unique<MemoryAllocator>(
       deserializer_->deserialize<MemoryAllocator::SerializationType>(),
       shmManager_
@@ -298,8 +294,7 @@ void CacheAllocator<CacheTrait>::initWorkers() {
 template <typename CacheTrait>
 std::unique_ptr<Deserializer> CacheAllocator<CacheTrait>::createDeserializer() {
   auto infoAddr = shmManager_->attachShm(
-      detail::kShmInfoName, nullptr,
-      ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm));
+      detail::kShmInfoName, nullptr, ShmSegmentOpts(PageSizeT::NORMAL, false));
   return std::make_unique<Deserializer>(
       reinterpret_cast<uint8_t*>(infoAddr.addr),
       reinterpret_cast<uint8_t*>(infoAddr.addr) + infoAddr.size);
@@ -3162,7 +3157,8 @@ void CacheAllocator<CacheTrait>::saveRamCache() {
   ioBuf->coalesce();
 
   ShmSegmentOpts opts;
-  opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
+  opts.typeOpts =
+      config_.usePosixShm ? PosixShmSegmentOpts{} : SysVShmSegmentOpts{};
 
   void* infoAddr =
       shmManager_
diff --git a/cachelib/allocator/TempShmMapping.cpp b/cachelib/allocator/TempShmMapping.cpp
@@ -34,8 +34,7 @@ TempShmMapping::TempShmMapping(size_t size)
 TempShmMapping::~TempShmMapping() {
   try {
     if (addr_) {
-      shmManager_->removeShm(detail::kTempShmCacheName.str(),
-                             PosixSysVSegmentOpts(false /* posix */));
+      shmManager_->removeShm(detail::kTempShmCacheName.str());
     }
     if (shmManager_) {
       shmManager_.reset();
@@ -78,8 +77,7 @@ void* TempShmMapping::createShmMapping(ShmManager& shmManager,
     return shmAddr;
   } catch (...) {
     if (shmAddr) {
-      shmManager.removeShm(detail::kTempShmCacheName.str(),
-                           PosixSysVSegmentOpts(false /* posix */));
+      shmManager.removeShm(detail::kTempShmCacheName.str());
     } else {
       munmap(addr, size);
     }
diff --git a/cachelib/allocator/memory/tests/SlabAllocatorTest.cpp b/cachelib/allocator/memory/tests/SlabAllocatorTest.cpp
@@ -584,7 +584,7 @@ TEST_F(SlabAllocatorTest, AdviseRelease) {
   shmName += std::to_string(::getpid());
   shmManager.createShm(shmName, allocSize, memory);
 
-  SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); };
+  SCOPE_EXIT { shmManager.removeShm(shmName); };
 
   memory = util::align(Slab::kSize, size, memory, allocSize);
 
@@ -714,7 +714,7 @@ TEST_F(SlabAllocatorTest, AdviseSaveRestore) {
   ShmManager shmManager(cacheDir, false /* posix */);
   shmManager.createShm(shmName, allocSize, memory);
 
-  SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); };
+  SCOPE_EXIT { shmManager.removeShm(shmName); };
 
   {
     SlabAllocator s(memory, size, config);
diff --git a/cachelib/shm/Shm.h b/cachelib/shm/Shm.h
@@ -52,13 +52,12 @@ class ShmSegment {
     if (auto* v = std::get_if<FileShmSegmentOpts>(&opts.typeOpts)) {
       segment_ =
           std::make_unique<FileShmSegment>(ShmNew, std::move(name), size, opts);
-    } else if (auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts)) {
-      if (v->usePosix)
-        segment_ = std::make_unique<PosixShmSegment>(ShmNew, std::move(name),
-                                                     size, opts);
-      else
-        segment_ = std::make_unique<SysVShmSegment>(ShmNew, std::move(name),
-                                                    size, opts);
+    } else if (auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts)) {
+      segment_ = std::make_unique<PosixShmSegment>(ShmNew, std::move(name),
+                                                   size, opts);
+    } else { // SysV segment
+      segment_ =
+          std::make_unique<SysVShmSegment>(ShmNew, std::move(name), size, opts);
     }
   }
 
@@ -69,13 +68,12 @@ class ShmSegment {
     if (std::get_if<FileShmSegmentOpts>(&opts.typeOpts)) {
       segment_ =
           std::make_unique<FileShmSegment>(ShmAttach, std::move(name), opts);
-    } else if (auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts)) {
-      if (v->usePosix)
-        segment_ =
-            std::make_unique<PosixShmSegment>(ShmAttach, std::move(name), opts);
-      else
-        segment_ =
-            std::make_unique<SysVShmSegment>(ShmAttach, std::move(name), opts);
+    } else if (auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts)) {
+      segment_ =
+          std::make_unique<PosixShmSegment>(ShmAttach, std::move(name), opts);
+    } else { // SysV segment
+      segment_ =
+          std::make_unique<SysVShmSegment>(ShmAttach, std::move(name), opts);
     }
   }
 
diff --git a/cachelib/shm/ShmCommon.h b/cachelib/shm/ShmCommon.h
@@ -82,12 +82,12 @@ struct FileShmSegmentOpts {
   std::string path;
 };
 
-struct PosixSysVSegmentOpts {
-  PosixSysVSegmentOpts(bool usePosix = false) : usePosix(usePosix) {}
-  bool usePosix;
-};
+struct PosixShmSegmentOpts {};
+
+struct SysVShmSegmentOpts {};
 
-using ShmTypeOpts = std::variant<FileShmSegmentOpts, PosixSysVSegmentOpts>;
+using ShmTypeOpts =
+    std::variant<FileShmSegmentOpts, PosixShmSegmentOpts, SysVShmSegmentOpts>;
 
 struct ShmSegmentOpts {
   PageSizeT pageSize{PageSizeT::NORMAL};
@@ -96,11 +96,10 @@ struct ShmSegmentOpts {
   ShmTypeOpts typeOpts{}; // opts specific to segment type
 
   explicit ShmSegmentOpts(PageSizeT p) : pageSize(p) {}
-  explicit ShmSegmentOpts(PageSizeT p, bool ro) : pageSize(p), readOnly(ro) {}
   explicit ShmSegmentOpts(PageSizeT p, bool ro, const std::string& path)
       : pageSize(p), readOnly(ro), typeOpts(path) {}
-  explicit ShmSegmentOpts(PageSizeT p, bool ro, bool posix)
-      : pageSize(p), readOnly(ro), typeOpts(posix) {}
+  explicit ShmSegmentOpts(PageSizeT p, bool ro)
+      : pageSize(p), readOnly(ro), typeOpts() {}
   ShmSegmentOpts() : pageSize(PageSizeT::NORMAL) {}
 };
 
diff --git a/cachelib/shm/ShmManager.cpp b/cachelib/shm/ShmManager.cpp
@@ -141,9 +141,13 @@ bool ShmManager::initFromFile() {
 
   for (const auto& kv : *object.nameToKeyMap_ref()) {
     if (kv.second.get_path() == "") {
-      PosixSysVSegmentOpts type;
-      type.usePosix = kv.second.get_usePosix();
-      nameToOpts_.insert({kv.first, type});
+      if (kv.second.get_usePosix()) {
+        PosixShmSegmentOpts type;
+        nameToOpts_.insert({kv.first, type});
+      } else {
+        SysVShmSegmentOpts type;
+        nameToOpts_.insert({kv.first, type});
+      }
     } else {
       FileShmSegmentOpts type;
       type.path = kv.second.get_path();
@@ -178,15 +182,15 @@ typename ShmManager::ShutDownRes ShmManager::writeActiveSegmentsToFile() {
     serialization::ShmTypeObject key;
     if (const auto* opts = std::get_if<FileShmSegmentOpts>(&kv.second)) {
       key.path() = opts->path;
-    } else {
-      try {
-        const auto& v = std::get<PosixSysVSegmentOpts>(kv.second);
-        key.usePosix() = v.usePosix;
-        key.path() = "";
-      } catch (std::bad_variant_access&) {
-        throw std::invalid_argument(folly::sformat("Not a valid segment"));
-      }
+    } else if (const auto* opts =
+                   std::get_if<PosixShmSegmentOpts>(&kv.second)) {
+      key.usePosix() = true;
+      key.path() = "";
+    } else { // SysV
+      key.usePosix() = false;
+      key.path() = "";
     }
+
     const auto it = segments_.find(name);
     // segment exists and is active.
     if (it != segments_.end() && it->second->isActive()) {
@@ -227,10 +231,7 @@ namespace {
 bool removeSegByName(ShmTypeOpts typeOpts, const std::string& uniqueName) {
   if (const auto* v = std::get_if<FileShmSegmentOpts>(&typeOpts)) {
     return FileShmSegment::removeByPath(v->path);
-  }
-
-  bool usePosix = std::get<PosixSysVSegmentOpts>(typeOpts).usePosix;
-  if (usePosix) {
+  } else if (std::get_if<PosixShmSegmentOpts>(&typeOpts)) {
     return PosixShmSegment::removeByName(uniqueName);
   } else {
     return SysVShmSegment::removeByName(uniqueName);
@@ -310,13 +311,13 @@ ShmAddr ShmManager::createShm(const std::string& shmName,
   // attached or mapped
   // TODO(SHM_FILE): should we try to remove the segment using all possible
   // segment types?
-  removeShm(shmName, opts.typeOpts);
+  removeShm(shmName);
 
   DCHECK(segments_.find(shmName) == segments_.end());
   DCHECK(nameToOpts_.find(shmName) == nameToOpts_.end());
 
-  const auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts);
-  if (v && usePosix_ != v->usePosix) {
+  const auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts);
+  if (v && !usePosix_) {
     throw std::invalid_argument(folly::sformat("Expected {} but got {} segment",
                                                usePosix_ ? "posix" : "SysV",
                                                usePosix_ ? "SysV" : "posix"));
@@ -347,9 +348,11 @@ ShmAddr ShmManager::createShm(const std::string& shmName,
   }
 
   auto ret = newSeg->getCurrentMapping();
-  if (v) {
-    PosixSysVSegmentOpts opts;
-    opts.usePosix = v->usePosix;
+  if (std::get_if<PosixShmSegmentOpts>(&opts.typeOpts)) {
+    PosixShmSegmentOpts opts;
+    nameToOpts_.emplace(shmName, opts);
+  } else if (std::get_if<SysVShmSegmentOpts>(&opts.typeOpts)) {
+    SysVShmSegmentOpts opts;
     nameToOpts_.emplace(shmName, opts);
   } else {
     FileShmSegmentOpts opts;
@@ -368,8 +371,8 @@ void ShmManager::attachNewShm(const std::string& shmName, ShmSegmentOpts opts) {
         folly::sformat("Unable to find any segment with name {}", shmName));
   }
 
-  const auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts);
-  if (v && usePosix_ != v->usePosix) {
+  const auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts);
+  if (v && !usePosix_) {
     throw std::invalid_argument(folly::sformat("Expected {} but got {} segment",
                                                usePosix_ ? "posix" : "SysV",
                                                usePosix_ ? "SysV" : "posix"));
@@ -423,7 +426,7 @@ ShmAddr ShmManager::attachShm(const std::string& shmName,
   return shm.getCurrentMapping();
 }
 
-bool ShmManager::removeShm(const std::string& shmName, ShmTypeOpts typeOpts) {
+bool ShmManager::removeShm(const std::string& shmName) {
   try {
     auto& shm = getShmByName(shmName);
     shm.detachCurrentMapping();
@@ -437,7 +440,11 @@ bool ShmManager::removeShm(const std::string& shmName, ShmTypeOpts typeOpts) {
     DCHECK(shm.isInvalid());
   } catch (const std::invalid_argument&) {
     // shm by this name is not attached.
-    const bool wasPresent = removeSegByName(typeOpts, uniqueIdForName(shmName));
+    if (nameToOpts_.find(shmName) == nameToOpts_.end()) {
+      return false;
+    }
+    const bool wasPresent =
+        removeSegByName(nameToOpts_[shmName], uniqueIdForName(shmName));
     if (!wasPresent) {
       DCHECK(segments_.end() == segments_.find(shmName));
       DCHECK(nameToOpts_.end() == nameToOpts_.find(shmName));
diff --git a/cachelib/shm/ShmManager.h b/cachelib/shm/ShmManager.h
@@ -99,7 +99,7 @@ class ShmManager {
   // @param shmName   name of the segment
   // @return  true if such a segment existed and we removed it.
   //          false if segment never existed
-  bool removeShm(const std::string& segName, ShmTypeOpts opts);
+  bool removeShm(const std::string& segName);
 
   // gets a current segment by the name that is managed by this
   // instance. The lifetime of the returned object is same as the
diff --git a/cachelib/shm/tests/common.h b/cachelib/shm/tests/common.h
@@ -98,7 +98,7 @@ class ShmTest : public ShmTestBase {
 
 class ShmTestPosix : public ShmTest {
  public:
-  ShmTestPosix() { opts.typeOpts = PosixSysVSegmentOpts(true); }
+  ShmTestPosix() { opts.typeOpts = PosixShmSegmentOpts(); }
 
  private:
   void clearSegment() override {
@@ -114,7 +114,7 @@ class ShmTestPosix : public ShmTest {
 
 class ShmTestSysV : public ShmTest {
  public:
-  ShmTestSysV() { opts.typeOpts = PosixSysVSegmentOpts(false); }
+  ShmTestSysV() { opts.typeOpts = SysVShmSegmentOpts(); }
 
  private:
   void clearSegment() override {
diff --git a/cachelib/shm/tests/test_shm_manager.cpp b/cachelib/shm/tests/test_shm_manager.cpp
