Skip to content

Commit e3c00af

Browse files
guptaskguptask
committed
cleaned up the interface. no need for use posix in some of the SHM APIs.
1 parent 4275baf commit e3c00af

File tree

13 files changed

+162
-114
lines changed

13 files changed

+162
-114
lines changed

‎cachelib/allocator/CacheAllocator-inl.h‎

Lines changed: 45 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
7272
AccessContainer::getRequiredSize(
7373
config_.accessConfig.getNumBuckets()),
7474
nullptr,
75-
ShmSegmentOpts(config_.accessConfig.getPageSize(),
76-
false,
77-
config_.usePosixShm))
75+
ShmSegmentOpts(config_.accessConfig.getPageSize()))
7876
.addr,
7977
compressor_,
8078
[this](Item* it) -> WriteHandle { return acquire(it); })),
@@ -85,9 +83,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
8583
AccessContainer::getRequiredSize(
8684
config_.chainedItemAccessConfig.getNumBuckets()),
8785
nullptr,
88-
ShmSegmentOpts(config_.accessConfig.getPageSize(),
89-
false,
90-
config_.usePosixShm))
86+
ShmSegmentOpts(config_.accessConfig.getPageSize()))
9187
.addr,
9288
compressor_,
9389
[this](Item* it) -> WriteHandle { return acquire(it); })),
@@ -103,8 +99,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
10399
config_.isNvmCacheEncryptionEnabled(),
104100
config_.isNvmCacheTruncateAllocSizeEnabled()} {
105101
initCommon(false);
106-
shmManager_->removeShm(detail::kShmInfoName,
107-
PosixSysVSegmentOpts(config_.usePosixShm));
102+
shmManager_->removeShm(detail::kShmInfoName);
108103
}
109104

110105
template <typename CacheTrait>
@@ -122,19 +117,17 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
122117
accessContainer_(std::make_unique<AccessContainer>(
123118
deserializer_->deserialize<AccessSerializationType>(),
124119
config_.accessConfig,
125-
shmManager_->attachShm(
126-
detail::kShmHashTableName,
127-
nullptr,
128-
ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),
120+
shmManager_->attachShm(detail::kShmHashTableName,
121+
nullptr,
122+
ShmSegmentOpts(PageSizeT::NORMAL)),
129123
compressor_,
130124
[this](Item* it) -> WriteHandle { return acquire(it); })),
131125
chainedItemAccessContainer_(std::make_unique<AccessContainer>(
132126
deserializer_->deserialize<AccessSerializationType>(),
133127
config_.chainedItemAccessConfig,
134-
shmManager_->attachShm(
135-
detail::kShmChainedItemHashTableName,
136-
nullptr,
137-
ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),
128+
shmManager_->attachShm(detail::kShmChainedItemHashTableName,
129+
nullptr,
130+
ShmSegmentOpts(PageSizeT::NORMAL)),
138131
compressor_,
139132
[this](Item* it) -> WriteHandle { return acquire(it); })),
140133
chainedItemLocks_(config_.chainedItemsLockPower,
@@ -155,8 +148,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
155148
// We will create a new info shm segment on shutDown(). If we don't remove
156149
// this info shm segment here and the new info shm segment's size is larger
157150
// than this one, creating new one will fail.
158-
shmManager_->removeShm(detail::kShmInfoName,
159-
PosixSysVSegmentOpts(config_.usePosixShm));
151+
shmManager_->removeShm(detail::kShmInfoName);
160152
}
161153

162154
template <typename CacheTrait>
@@ -174,7 +166,11 @@ std::unique_ptr<MemoryAllocator>
174166
CacheAllocator<CacheTrait>::createNewMemoryAllocator() {
175167
ShmSegmentOpts opts;
176168
opts.alignment = sizeof(Slab);
177-
opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
169+
if (config_.usePosixShm) {
170+
opts.typeOpts = PosixShmSegmentOpts{};
171+
} else {
172+
opts.typeOpts = SysVShmSegmentOpts{};
173+
}
178174
return std::make_unique<MemoryAllocator>(
179175
getAllocatorConfig(config_),
180176
shmManager_
@@ -189,7 +185,11 @@ std::unique_ptr<MemoryAllocator>
189185
CacheAllocator<CacheTrait>::restoreMemoryAllocator() {
190186
ShmSegmentOpts opts;
191187
opts.alignment = sizeof(Slab);
192-
opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
188+
if (config_.usePosixShm) {
189+
opts.typeOpts = PosixShmSegmentOpts{};
190+
} else {
191+
opts.typeOpts = SysVShmSegmentOpts{};
192+
}
193193
return std::make_unique<MemoryAllocator>(
194194
deserializer_->deserialize<MemoryAllocator::SerializationType>(),
195195
shmManager_
@@ -298,8 +298,7 @@ void CacheAllocator<CacheTrait>::initWorkers() {
298298
template <typename CacheTrait>
299299
std::unique_ptr<Deserializer> CacheAllocator<CacheTrait>::createDeserializer() {
300300
auto infoAddr = shmManager_->attachShm(
301-
detail::kShmInfoName, nullptr,
302-
ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm));
301+
detail::kShmInfoName, nullptr, ShmSegmentOpts(PageSizeT::NORMAL, false));
303302
return std::make_unique<Deserializer>(
304303
reinterpret_cast<uint8_t*>(infoAddr.addr),
305304
reinterpret_cast<uint8_t*>(infoAddr.addr) + infoAddr.size);
@@ -3162,7 +3161,11 @@ void CacheAllocator<CacheTrait>::saveRamCache() {
31623161
ioBuf->coalesce();
31633162

31643163
ShmSegmentOpts opts;
3165-
opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
3164+
if (config_.usePosixShm) {
3165+
opts.typeOpts = PosixShmSegmentOpts{};
3166+
} else {
3167+
opts.typeOpts = SysVShmSegmentOpts{};
3168+
}
31663169

31673170
void* infoAddr =
31683171
shmManager_
@@ -3537,11 +3540,25 @@ bool CacheAllocator<
35373540
// cache dir did not exist. Try to nuke the segments we know by name.
35383541
// Any other concurrent process can not be attached to the segments or
35393542
// even if it does, we want to mark it for destruction.
3540-
ShmManager::removeByName(cacheDir, detail::kShmInfoName, posix);
3541-
ShmManager::removeByName(cacheDir, detail::kShmCacheName, posix);
3542-
ShmManager::removeByName(cacheDir, detail::kShmHashTableName, posix);
3543-
ShmManager::removeByName(cacheDir, detail::kShmChainedItemHashTableName,
3544-
posix);
3543+
if (posix) {
3544+
ShmManager::removeByName(cacheDir, detail::kShmInfoName,
3545+
PosixShmSegmentOpts{});
3546+
ShmManager::removeByName(cacheDir, detail::kShmCacheName,
3547+
PosixShmSegmentOpts{});
3548+
ShmManager::removeByName(cacheDir, detail::kShmHashTableName,
3549+
PosixShmSegmentOpts{});
3550+
ShmManager::removeByName(cacheDir, detail::kShmChainedItemHashTableName,
3551+
PosixShmSegmentOpts{});
3552+
} else { // SysV
3553+
ShmManager::removeByName(cacheDir, detail::kShmInfoName,
3554+
SysVShmSegmentOpts{});
3555+
ShmManager::removeByName(cacheDir, detail::kShmCacheName,
3556+
SysVShmSegmentOpts{});
3557+
ShmManager::removeByName(cacheDir, detail::kShmHashTableName,
3558+
SysVShmSegmentOpts{});
3559+
ShmManager::removeByName(cacheDir, detail::kShmChainedItemHashTableName,
3560+
SysVShmSegmentOpts{});
3561+
}
35453562

35463563
// TODO(SHM_FILE): try to nuke segments of differente types (which require
35473564
// extra info)

‎cachelib/allocator/ReadOnlySharedCacheView.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,15 @@ class ReadOnlySharedCacheView {
4848
// address chosen by the kernel
4949
explicit ReadOnlySharedCacheView(const std::string& cacheDir,
5050
bool usePosixShm,
51-
void* addr = nullptr)
52-
: shm_(ShmManager::attachShmReadOnly(
53-
cacheDir, detail::kShmCacheName, usePosixShm, addr)) {}
51+
void* addr = nullptr) {
52+
if (usePosixShm) {
53+
shm_ = ShmManager::attachShmReadOnly(
54+
cacheDir, detail::kShmCacheName, PosixShmSegmentOpts{}, addr);
55+
} else { // SysV
56+
shm_ = ShmManager::attachShmReadOnly(
57+
cacheDir, detail::kShmCacheName, SysVShmSegmentOpts{}, addr);
58+
}
59+
}
5460

5561
// returns the absolute address at which the shared memory mapping is mounted.
5662
// The caller can add a relative offset obtained from

‎cachelib/allocator/TempShmMapping.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ TempShmMapping::TempShmMapping(size_t size)
3434
TempShmMapping::~TempShmMapping() {
3535
try {
3636
if (addr_) {
37-
shmManager_->removeShm(detail::kTempShmCacheName.str(),
38-
PosixSysVSegmentOpts(false /* posix */));
37+
shmManager_->removeShm(detail::kTempShmCacheName.str());
3938
}
4039
if (shmManager_) {
4140
shmManager_.reset();
@@ -78,8 +77,7 @@ void* TempShmMapping::createShmMapping(ShmManager& shmManager,
7877
return shmAddr;
7978
} catch (...) {
8079
if (shmAddr) {
81-
shmManager.removeShm(detail::kTempShmCacheName.str(),
82-
PosixSysVSegmentOpts(false /* posix */));
80+
shmManager.removeShm(detail::kTempShmCacheName.str());
8381
} else {
8482
munmap(addr, size);
8583
}

‎cachelib/allocator/memory/tests/SlabAllocatorTest.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,7 @@ TEST_F(SlabAllocatorTest, AdviseRelease) {
584584
shmName += std::to_string(::getpid());
585585
shmManager.createShm(shmName, allocSize, memory);
586586

587-
SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); };
587+
SCOPE_EXIT { shmManager.removeShm(shmName); };
588588

589589
memory = util::align(Slab::kSize, size, memory, allocSize);
590590

@@ -714,7 +714,7 @@ TEST_F(SlabAllocatorTest, AdviseSaveRestore) {
714714
ShmManager shmManager(cacheDir, false /* posix */);
715715
shmManager.createShm(shmName, allocSize, memory);
716716

717-
SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); };
717+
SCOPE_EXIT { shmManager.removeShm(shmName); };
718718

719719
{
720720
SlabAllocator s(memory, size, config);

‎cachelib/allocator/tests/BaseAllocatorTest.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1753,8 +1753,14 @@ class BaseAllocatorTest : public AllocatorTest<AllocatorT> {
17531753
ASSERT_NE(nullptr, handle.get());
17541754
}
17551755
}
1756-
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
1757-
tempCacheDir, detail::kTempShmCacheName.str(), config.usePosixShm));
1756+
if (config.usePosixShm) {
1757+
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
1758+
tempCacheDir, detail::kTempShmCacheName.str(),
1759+
PosixShmSegmentOpts{}));
1760+
} else {
1761+
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
1762+
tempCacheDir, detail::kTempShmCacheName.str(), SysVShmSegmentOpts{}));
1763+
}
17581764
ASSERT_FALSE(util::pathExists(tempCacheDir));
17591765
}
17601766

‎cachelib/allocator/tests/TestBase-inl.h‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -301,35 +301,50 @@ void AllocatorTest<AllocatorT>::runSerializationTest(
301301
template <typename AllocatorT>
302302
void AllocatorTest<AllocatorT>::testInfoShmIsRemoved(
303303
typename AllocatorT::Config config) {
304-
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
305-
config.getCacheDir(), detail::kShmInfoName, config.usePosixShm));
304+
if (config.usePosixShm) {
305+
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
306+
config.getCacheDir(), detail::kShmInfoName, PosixShmSegmentOpts{}));
307+
} else {
308+
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
309+
config.getCacheDir(), detail::kShmInfoName, SysVShmSegmentOpts{}));
310+
}
306311
}
307312

308313
template <typename AllocatorT>
309314
void AllocatorTest<AllocatorT>::testShmIsRemoved(
310315
typename AllocatorT::Config config) {
311316
testInfoShmIsRemoved(config);
317+
ShmTypeOpts opts;
318+
if (config.usePosixShm) {
319+
opts = PosixShmSegmentOpts{};
320+
} else {
321+
opts = SysVShmSegmentOpts{};
322+
}
312323
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
313-
config.getCacheDir(), detail::kShmHashTableName, config.usePosixShm));
324+
config.getCacheDir(), detail::kShmHashTableName, opts));
314325
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
315-
config.getCacheDir(), detail::kShmCacheName, config.usePosixShm));
326+
config.getCacheDir(), detail::kShmCacheName, opts));
316327
ASSERT_FALSE(AllocatorT::ShmManager::segmentExists(
317-
config.getCacheDir(), detail::kShmChainedItemHashTableName,
318-
config.usePosixShm));
328+
config.getCacheDir(), detail::kShmChainedItemHashTableName, opts));
319329
}
320330

321331
template <typename AllocatorT>
322332
void AllocatorTest<AllocatorT>::testShmIsNotRemoved(
323333
typename AllocatorT::Config config) {
334+
ShmTypeOpts opts;
335+
if (config.usePosixShm) {
336+
opts = PosixShmSegmentOpts{};
337+
} else {
338+
opts = SysVShmSegmentOpts{};
339+
}
324340
ASSERT_TRUE(AllocatorT::ShmManager::segmentExists(
325-
config.getCacheDir(), detail::kShmInfoName, config.usePosixShm));
341+
config.getCacheDir(), detail::kShmInfoName, opts));
326342
ASSERT_TRUE(AllocatorT::ShmManager::segmentExists(
327-
config.getCacheDir(), detail::kShmHashTableName, config.usePosixShm));
343+
config.getCacheDir(), detail::kShmHashTableName, opts));
328344
ASSERT_TRUE(AllocatorT::ShmManager::segmentExists(
329-
config.getCacheDir(), detail::kShmCacheName, config.usePosixShm));
345+
config.getCacheDir(), detail::kShmCacheName, opts));
330346
ASSERT_TRUE(AllocatorT::ShmManager::segmentExists(
331-
config.getCacheDir(), detail::kShmChainedItemHashTableName,
332-
config.usePosixShm));
347+
config.getCacheDir(), detail::kShmChainedItemHashTableName, opts));
333348
}
334349
} // namespace tests
335350
} // namespace cachelib

‎cachelib/persistence/PersistenceManager.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,8 @@ std::unique_ptr<ShmSegment> PersistenceManager::saveShm(
326326
PersistenceStreamWriter& writer,
327327
PersistenceType type,
328328
const std::string& name) {
329-
auto segment = ShmManager::attachShmReadOnly(cacheDir_, name, true);
329+
auto segment =
330+
ShmManager::attachShmReadOnly(cacheDir_, name, PosixShmSegmentOpts{});
330331
auto shm = segment->getCurrentMapping();
331332
CACHELIB_CHECK_THROWF(shm.size > 0, "shm {} is empty.", name);
332333

‎cachelib/shm/Shm.h‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,12 @@ class ShmSegment {
5252
if (auto* v = std::get_if<FileShmSegmentOpts>(&opts.typeOpts)) {
5353
segment_ =
5454
std::make_unique<FileShmSegment>(ShmNew, std::move(name), size, opts);
55-
} else if (auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts)) {
56-
if (v->usePosix)
57-
segment_ = std::make_unique<PosixShmSegment>(ShmNew, std::move(name),
58-
size, opts);
59-
else
60-
segment_ = std::make_unique<SysVShmSegment>(ShmNew, std::move(name),
61-
size, opts);
55+
} else if (auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts)) {
56+
segment_ = std::make_unique<PosixShmSegment>(ShmNew, std::move(name),
57+
size, opts);
58+
} else { // SysV segment
59+
segment_ =
60+
std::make_unique<SysVShmSegment>(ShmNew, std::move(name), size, opts);
6261
}
6362
}
6463

@@ -69,13 +68,12 @@ class ShmSegment {
6968
if (std::get_if<FileShmSegmentOpts>(&opts.typeOpts)) {
7069
segment_ =
7170
std::make_unique<FileShmSegment>(ShmAttach, std::move(name), opts);
72-
} else if (auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts)) {
73-
if (v->usePosix)
74-
segment_ =
75-
std::make_unique<PosixShmSegment>(ShmAttach, std::move(name), opts);
76-
else
77-
segment_ =
78-
std::make_unique<SysVShmSegment>(ShmAttach, std::move(name), opts);
71+
} else if (auto* v = std::get_if<PosixShmSegmentOpts>(&opts.typeOpts)) {
72+
segment_ =
73+
std::make_unique<PosixShmSegment>(ShmAttach, std::move(name), opts);
74+
} else { // SysV segment
75+
segment_ =
76+
std::make_unique<SysVShmSegment>(ShmAttach, std::move(name), opts);
7977
}
8078
}
8179

‎cachelib/shm/ShmCommon.h‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,25 +82,24 @@ struct FileShmSegmentOpts {
8282
std::string path;
8383
};
8484

85-
struct PosixSysVSegmentOpts {
86-
PosixSysVSegmentOpts(bool usePosix = false) : usePosix(usePosix) {}
87-
bool usePosix;
88-
};
85+
struct PosixShmSegmentOpts {};
86+
87+
struct SysVShmSegmentOpts {};
8988

90-
using ShmTypeOpts = std::variant<FileShmSegmentOpts, PosixSysVSegmentOpts>;
89+
using ShmTypeOpts =
90+
std::variant<FileShmSegmentOpts, PosixShmSegmentOpts, SysVShmSegmentOpts>;
9191

9292
struct ShmSegmentOpts {
9393
PageSizeT pageSize{PageSizeT::NORMAL};
9494
bool readOnly{false};
9595
size_t alignment{1}; // alignment for mapping.
9696
ShmTypeOpts typeOpts{}; // opts specific to segment type
9797

98-
explicit ShmSegmentOpts(PageSizeT p) : pageSize(p) {}
99-
explicit ShmSegmentOpts(PageSizeT p, bool ro) : pageSize(p), readOnly(ro) {}
98+
explicit ShmSegmentOpts(PageSizeT p) : pageSize(p), typeOpts() {}
10099
explicit ShmSegmentOpts(PageSizeT p, bool ro, const std::string& path)
101100
: pageSize(p), readOnly(ro), typeOpts(path) {}
102-
explicit ShmSegmentOpts(PageSizeT p, bool ro, bool posix)
103-
: pageSize(p), readOnly(ro), typeOpts(posix) {}
101+
explicit ShmSegmentOpts(PageSizeT p, bool ro)
102+
: pageSize(p), readOnly(ro), typeOpts() {}
104103
ShmSegmentOpts() : pageSize(PageSizeT::NORMAL) {}
105104
};
106105

0 commit comments

Comments
 (0)