From 7485170ab2131d541dec58850bfaaef83de439e5 Mon Sep 17 00:00:00 2001 From: "Chorazewicz, Igor" Date: Tue, 2 Nov 2021 16:00:53 +0100 Subject: [PATCH 01/13] Run centos and debian workflows on push and PR --- .github/workflows/build-cachelib-centos.yml | 5 +++-- .github/workflows/build-cachelib-debian.yml | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-cachelib-centos.yml b/.github/workflows/build-cachelib-centos.yml index 3b071a186a..5cd28db1b6 100644 --- a/.github/workflows/build-cachelib-centos.yml +++ b/.github/workflows/build-cachelib-centos.yml @@ -1,7 +1,8 @@ name: build-cachelib-centos-latest on: - schedule: - - cron: '30 5 * * 1,4' + push: + pull_request: + jobs: build-cachelib-centos8-latest: name: "CentOS/latest - Build CacheLib with all dependencies" diff --git a/.github/workflows/build-cachelib-debian.yml b/.github/workflows/build-cachelib-debian.yml index a2ae44a569..182759e175 100644 --- a/.github/workflows/build-cachelib-debian.yml +++ b/.github/workflows/build-cachelib-debian.yml @@ -1,7 +1,8 @@ name: build-cachelib-debian-10 on: - schedule: - - cron: '30 5 * * 2,6' + push: + pull_request: + jobs: build-cachelib-debian-10: name: "Debian/Buster - Build CacheLib with all dependencies" From 7a6dbc2f215c6692f0c5c92cb8b089a65071beb3 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Tue, 19 Oct 2021 20:34:22 -0400 Subject: [PATCH 02/13] Introduce FileShmSegment for file-backed shared memory It's implementation is mostly based on PosixShmSegment. Also, extend ShmManager and ShmSegmentOpts to support this new segment type. --- cachelib/allocator/CacheAllocator-inl.h | 38 ++- cachelib/allocator/CacheAllocator.h | 3 +- cachelib/allocator/TempShmMapping.cpp | 6 +- cachelib/shm/CMakeLists.txt | 1 + cachelib/shm/FileShmSegment.cpp | 341 ++++++++++++++++++++++++ cachelib/shm/FileShmSegment.h | 116 ++++++++ cachelib/shm/PosixShmSegment.cpp | 14 +- cachelib/shm/PosixShmSegment.h | 2 - cachelib/shm/Shm.h | 35 ++- cachelib/shm/ShmCommon.h | 23 ++ cachelib/shm/ShmManager.cpp | 58 ++-- cachelib/shm/ShmManager.h | 8 +- 12 files changed, 590 insertions(+), 55 deletions(-) create mode 100644 cachelib/shm/FileShmSegment.cpp create mode 100644 cachelib/shm/FileShmSegment.h diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index 15cfee7432..a6b2ee0b94 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -68,7 +68,8 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) AccessContainer::getRequiredSize( config_.accessConfig.getNumBuckets()), nullptr, - ShmSegmentOpts(config_.accessConfig.getPageSize())) + ShmSegmentOpts(config_.accessConfig.getPageSize(), + false, config_.usePosixShm)) .addr, compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), @@ -79,7 +80,8 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) AccessContainer::getRequiredSize( config_.chainedItemAccessConfig.getNumBuckets()), nullptr, - ShmSegmentOpts(config_.accessConfig.getPageSize())) + ShmSegmentOpts(config_.accessConfig.getPageSize(), + false, config_.usePosixShm)) .addr, compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), @@ -89,7 +91,8 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(), config_.isNvmCacheTruncateAllocSizeEnabled()} { initCommon(false); - shmManager_->removeShm(detail::kShmInfoName); + shmManager_->removeShm(detail::kShmInfoName, + PosixSysVSegmentOpts(config_.usePosixShm)); } template @@ -107,13 +110,15 @@ CacheAllocator::CacheAllocator(SharedMemAttachT, Config config) accessContainer_(std::make_unique( deserializer_->deserialize(), config_.accessConfig, - shmManager_->attachShm(detail::kShmHashTableName), + shmManager_->attachShm(detail::kShmHashTableName, nullptr, + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)), compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), chainedItemAccessContainer_(std::make_unique( deserializer_->deserialize(), config_.chainedItemAccessConfig, - shmManager_->attachShm(detail::kShmChainedItemHashTableName), + shmManager_->attachShm(detail::kShmChainedItemHashTableName, nullptr, + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)), compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), chainedItemLocks_(config_.chainedItemsLockPower, @@ -130,7 +135,8 @@ CacheAllocator::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); + shmManager_->removeShm(detail::kShmInfoName, + PosixSysVSegmentOpts(config_.usePosixShm)); } template @@ -148,6 +154,7 @@ std::unique_ptr CacheAllocator::createNewMemoryAllocator() { ShmSegmentOpts opts; opts.alignment = sizeof(Slab); + opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); return std::make_unique( getAllocatorConfig(config_), shmManager_ @@ -162,6 +169,7 @@ std::unique_ptr CacheAllocator::restoreMemoryAllocator() { ShmSegmentOpts opts; opts.alignment = sizeof(Slab); + opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); return std::make_unique( deserializer_->deserialize(), shmManager_ @@ -265,7 +273,8 @@ void CacheAllocator::initWorkers() { template std::unique_ptr CacheAllocator::createDeserializer() { - auto infoAddr = shmManager_->attachShm(detail::kShmInfoName); + auto infoAddr = shmManager_->attachShm(detail::kShmInfoName, nullptr, + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)); return std::make_unique( reinterpret_cast(infoAddr.addr), reinterpret_cast(infoAddr.addr) + infoAddr.size); @@ -3041,8 +3050,11 @@ void CacheAllocator::saveRamCache() { std::unique_ptr ioBuf = serializedBuf.move(); ioBuf->coalesce(); - void* infoAddr = - shmManager_->createShm(detail::kShmInfoName, ioBuf->length()).addr; + ShmSegmentOpts opts; + opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); + + void* infoAddr = shmManager_->createShm(detail::kShmInfoName, ioBuf->length(), + nullptr, opts).addr; Serializer serializer(reinterpret_cast(infoAddr), reinterpret_cast(infoAddr) + ioBuf->length()); serializer.writeToBuffer(std::move(ioBuf)); @@ -3386,7 +3398,7 @@ bool CacheAllocator::stopReaper(std::chrono::seconds timeout) { template bool CacheAllocator::cleanupStrayShmSegments( - const std::string& cacheDir, bool posix) { + const std::string& cacheDir, bool posix /*TODO(SHM_FILE): const std::vector& config */) { if (util::getStatIfExists(cacheDir, nullptr) && util::isDir(cacheDir)) { try { // cache dir exists. clean up only if there are no other processes @@ -3405,6 +3417,12 @@ bool CacheAllocator::cleanupStrayShmSegments( ShmManager::removeByName(cacheDir, detail::kShmHashTableName, posix); ShmManager::removeByName(cacheDir, detail::kShmChainedItemHashTableName, posix); + + // TODO(SHM_FILE): try to nuke segments of differente types (which require + // extra info) + // for (auto &tier : config) { + // ShmManager::removeByName(cacheDir, tierShmName, config_.memoryTiers[i].opts); + // } } return true; } diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index a065ff208f..9b2831a0dd 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1035,7 +1035,8 @@ class CacheAllocator : public CacheBase { // returns true if there was no error in trying to cleanup the segment // because another process was attached. False if the user tried to clean up // and the cache was actually attached. - static bool cleanupStrayShmSegments(const std::string& cacheDir, bool posix); + static bool cleanupStrayShmSegments(const std::string& cacheDir, bool posix + /*TODO: const std::vector& config = {} */); // gives a relative offset to a pointer within the cache. uint64_t getItemPtrAsOffset(const void* ptr); diff --git a/cachelib/allocator/TempShmMapping.cpp b/cachelib/allocator/TempShmMapping.cpp index cb7eb49ded..f6d3d18ec4 100644 --- a/cachelib/allocator/TempShmMapping.cpp +++ b/cachelib/allocator/TempShmMapping.cpp @@ -34,7 +34,8 @@ TempShmMapping::TempShmMapping(size_t size) TempShmMapping::~TempShmMapping() { try { if (addr_) { - shmManager_->removeShm(detail::kTempShmCacheName.str()); + shmManager_->removeShm(detail::kTempShmCacheName.str(), + PosixSysVSegmentOpts(false /* posix */)); } if (shmManager_) { shmManager_.reset(); @@ -77,7 +78,8 @@ void* TempShmMapping::createShmMapping(ShmManager& shmManager, return shmAddr; } catch (...) { if (shmAddr) { - shmManager.removeShm(detail::kTempShmCacheName.str()); + shmManager.removeShm(detail::kTempShmCacheName.str(), + PosixSysVSegmentOpts(false /* posix */)); } else { munmap(addr, size); } diff --git a/cachelib/shm/CMakeLists.txt b/cachelib/shm/CMakeLists.txt index 2b04b31039..c3eeed4ad7 100644 --- a/cachelib/shm/CMakeLists.txt +++ b/cachelib/shm/CMakeLists.txt @@ -16,6 +16,7 @@ add_thrift_file(SHM shm.thrift frozen2) add_library (cachelib_shm ${SHM_THRIFT_FILES} + FileShmSegment.cpp PosixShmSegment.cpp ShmCommon.cpp ShmManager.cpp diff --git a/cachelib/shm/FileShmSegment.cpp b/cachelib/shm/FileShmSegment.cpp new file mode 100644 index 0000000000..40628aebf6 --- /dev/null +++ b/cachelib/shm/FileShmSegment.cpp @@ -0,0 +1,341 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "cachelib/shm/FileShmSegment.h" + +#include +#include +#include +#include +#include + +#include "cachelib/common/Utils.h" + +namespace facebook { +namespace cachelib { + +constexpr static mode_t kRWMode = 0666; +typedef struct stat stat_t; + +namespace detail { + +// TODO(SHM_FILE): move those *Impl functions to common file, there are copied +// from PosixShmSegment.cpp +static int openImpl(const char* name, int flags) { + const int fd = open(name, flags); + + if (fd != -1) { + return fd; + } + + switch (errno) { + case EEXIST: + case EMFILE: + case ENFILE: + case EACCES: + util::throwSystemError(errno); + break; + case ENAMETOOLONG: + case EINVAL: + util::throwSystemError(errno, "Invalid segment name"); + break; + case ENOENT: + if (!(flags & O_CREAT)) { + util::throwSystemError(errno); + } else { + XDCHECK(false); + // FIXME: posix says that ENOENT is thrown only when O_CREAT + // is not set. However, it seems to be set even when O_CREAT + // was set and the parent of path name does not exist. + util::throwSystemError(errno, "Invalid errno"); + } + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } + return kInvalidFD; +} + +static void unlinkImpl(const char* const name) { + const int ret = unlink(name); + if (ret == 0) { + return; + } + + switch (errno) { + case ENOENT: + case EACCES: + util::throwSystemError(errno); + break; + case ENAMETOOLONG: + case EINVAL: + util::throwSystemError(errno, "Invalid segment name"); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +static void ftruncateImpl(int fd, size_t size) { + const int ret = ftruncate(fd, size); + if (ret == 0) { + return; + } + switch (errno) { + case EBADF: + case EINVAL: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +static void fstatImpl(int fd, stat_t* buf) { + const int ret = fstat(fd, buf); + if (ret == 0) { + return; + } + switch (errno) { + case EBADF: + case ENOMEM: + case EOVERFLOW: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +static void* mmapImpl( + void* addr, size_t length, int prot, int flags, int fd, off_t offset) { + void* ret = mmap(addr, length, prot, flags, fd, offset); + if (ret != MAP_FAILED) { + return ret; + } + + switch (errno) { + case EACCES: + case EAGAIN: + if (flags & MAP_LOCKED) { + util::throwSystemError(ENOMEM); + break; + } + case EBADF: + case EINVAL: + case ENFILE: + case ENODEV: + case ENOMEM: + case EPERM: + case ETXTBSY: + case EOVERFLOW: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } + return nullptr; +} + +static void munmapImpl(void* addr, size_t length) { + const int ret = munmap(addr, length); + + if (ret == 0) { + return; + } else if (errno == EINVAL) { + util::throwSystemError(errno); + } else { + XDCHECK(false); + util::throwSystemError(EINVAL, "Invalid errno"); + } +} + +} // namespace detail + +FileShmSegment::FileShmSegment(ShmAttachT, + const std::string& name, + ShmSegmentOpts opts) + : ShmBase(std::move(opts), name), + fd_(getExisting(getPath(), opts_)) { + XDCHECK_NE(fd_, kInvalidFD); + markActive(); + createReferenceMapping(); +} + +FileShmSegment::FileShmSegment(ShmNewT, + const std::string& name, + size_t size, + ShmSegmentOpts opts) + : ShmBase(std::move(opts), name), + fd_(createNewSegment(getPath())) { + markActive(); + resize(size); + XDCHECK(isActive()); + XDCHECK_NE(fd_, kInvalidFD); + // this ensures that the segment lives while the object lives. + createReferenceMapping(); +} + +FileShmSegment::~FileShmSegment() { + try { + // delete the reference mapping so the segment can be deleted if its + // marked to be. + deleteReferenceMapping(); + } catch (const std::system_error& e) { + } + + // need to close the fd without throwing any exceptions. so we call close + // directly. + if (fd_ != kInvalidFD) { + const int ret = close(fd_); + if (ret != 0) { + XDCHECK_NE(errno, EIO); + XDCHECK_NE(errno, EINTR); + XDCHECK_EQ(errno, EBADF); + XDCHECK(!errno); + } + } +} + +int FileShmSegment::createNewSegment(const std::string& name) { + constexpr static int createFlags = O_RDWR | O_CREAT | O_EXCL; + return detail::openImpl(name.c_str(), createFlags); +} + +int FileShmSegment::getExisting(const std::string& name, + const ShmSegmentOpts& opts) { + int flags = opts.readOnly ? O_RDONLY : O_RDWR; + return detail::openImpl(name.c_str(), flags); +} + +void FileShmSegment::markForRemoval() { + if (isActive()) { + // we still have the fd open. so we can use it to perform ftruncate + // even after marking for removal through unlink. The fd does not get + // recycled until we actually destroy this object. + removeByPath(getPath()); + markForRemove(); + } else { + XDCHECK(false); + } +} + +bool FileShmSegment::removeByPath(const std::string& path) { + try { + detail::unlinkImpl(path.c_str()); + return true; + } catch (const std::system_error& e) { + // unlink is opaque unlike sys-V api where its through the shmid. Hence + // if someone has already unlinked it for us, we just let it pass. + if (e.code().value() != ENOENT) { + throw; + } + return false; + } +} + +std::string FileShmSegment::getPath() const { + return std::get(opts_.typeOpts).path; +} + +size_t FileShmSegment::getSize() const { + if (isActive() || isMarkedForRemoval()) { + stat_t buf = {}; + detail::fstatImpl(fd_, &buf); + return buf.st_size; + } else { + throw std::runtime_error(folly::sformat( + "Trying to get size of segment with name {} in an invalid state", + getName())); + } + return 0; +} + +void FileShmSegment::resize(size_t size) const { + size = detail::getPageAlignedSize(size, opts_.pageSize); + XDCHECK(isActive() || isMarkedForRemoval()); + if (isActive() || isMarkedForRemoval()) { + XDCHECK_NE(fd_, kInvalidFD); + detail::ftruncateImpl(fd_, size); + } else { + throw std::runtime_error(folly::sformat( + "Trying to resize segment with name {} in an invalid state", + getName())); + } +} + +void* FileShmSegment::mapAddress(void* addr) const { + size_t size = getSize(); + if (!detail::isPageAlignedSize(size, opts_.pageSize) || + !detail::isPageAlignedAddr(addr, opts_.pageSize)) { + util::throwSystemError(EINVAL, "Address/size not aligned"); + } + +#ifndef MAP_HUGE_2MB +#define MAP_HUGE_2MB (21 << MAP_HUGE_SHIFT) +#endif + +#ifndef MAP_HUGE_1GB +#define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT) +#endif + + int flags = MAP_SHARED; + if (opts_.pageSize == PageSizeT::TWO_MB) { + flags |= MAP_HUGETLB | MAP_HUGE_2MB; + } else if (opts_.pageSize == PageSizeT::ONE_GB) { + flags |= MAP_HUGETLB | MAP_HUGE_1GB; + } + // If users pass in an address, they must make sure that address is unused. + if (addr != nullptr) { + flags |= MAP_FIXED; + } + + const int prot = opts_.readOnly ? PROT_READ : PROT_WRITE | PROT_READ; + + void* retAddr = detail::mmapImpl(addr, size, prot, flags, fd_, 0); + // if there was hint for mapping, then fail if we cannot respect this + // because we want to be specific about mapping to exactly that address. + if (retAddr != nullptr && addr != nullptr && retAddr != addr) { + util::throwSystemError(EINVAL, "Address already mapped"); + } + XDCHECK(retAddr == addr || addr == nullptr); + return retAddr; +} + +void FileShmSegment::unMap(void* addr) const { + detail::munmapImpl(addr, getSize()); +} + +void FileShmSegment::createReferenceMapping() { + // create a mapping that lasts the life of this object. mprotect it to + // ensure there are no actual accesses. + referenceMapping_ = detail::mmapImpl( + nullptr, detail::getPageSize(), PROT_NONE, MAP_SHARED, fd_, 0); + XDCHECK(referenceMapping_ != nullptr); +} + +void FileShmSegment::deleteReferenceMapping() const { + if (referenceMapping_ != nullptr) { + detail::munmapImpl(referenceMapping_, detail::getPageSize()); + } +} +} // namespace cachelib +} // namespace facebook diff --git a/cachelib/shm/FileShmSegment.h b/cachelib/shm/FileShmSegment.h new file mode 100644 index 0000000000..bccb72d674 --- /dev/null +++ b/cachelib/shm/FileShmSegment.h @@ -0,0 +1,116 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include + +#include "cachelib/shm/ShmCommon.h" + +namespace facebook { +namespace cachelib { + +/* This class lets you manage a pmem shared memory segment identified by + * name. This is very similar to the Posix shared memory segment, except + * that it allows for resizing of the segments on the fly. This can let the + * application logic to grow/shrink the shared memory segment at its end. + * Accessing the pages truncated on shrinking will result in SIGBUS. + * + * Segments can be created and attached to the process's address space. + * Segments can be marked for removal, even while they are currently attached + * to some process's address space. Upon which, any subsequent attach fails + * until a new segment of the same name is created. Once the last process + * attached to the segment unmaps the memory from its address space, the + * physical memory associated with this segment is freed. + * + * At any given point of time, there is only ONE unique attachable segment by + * name, but there could exist several unattachable segments which were once + * referenced by the same name living in process address space while all of + * them are marked for removal. + */ + +class FileShmSegment : public ShmBase { + public: + // attach to an existing pmem segment with the given name + // + // @param name Name of the segment + // @param opts the options for attaching to the segment. + FileShmSegment(ShmAttachT, + const std::string& name, + ShmSegmentOpts opts = {}); + + // create a new segment + // @param name The name of the segment + // @param size The size of the segment. This will be rounded up to the + // nearest page size. + FileShmSegment(ShmNewT, + const std::string& name, + size_t size, + ShmSegmentOpts opts = {}); + + // destructor + ~FileShmSegment() override; + + std::string getKeyStr() const noexcept override { return getPath(); } + + // marks the current segment to be removed once it is no longer mapped + // by any process in the kernel. + void markForRemoval() override; + + // return the current size of the segment. throws std::system_error + // with EINVAL if the segment is invalid or appropriate errno if the + // segment exists but we have a bad fd or kernel is out of memory. + size_t getSize() const override; + + // attaches the segment from the start to the address space of the + // caller. the address must be page aligned. + // @param addr the start of the address for attaching. + // + // @return the address where the segment was mapped to. This will be same + // as addr if addr is not nullptr + // @throw std::system_error with EINVAL if the segment is not valid or + // address/length are not page aligned. + void* mapAddress(void* addr) const override; + + // unmaps the memory from addr up to the given length from the + // address space. + void unMap(void* addr) const override; + + // useful for removing without attaching + // @return true if the segment existed. false otherwise + static bool removeByPath(const std::string& path); + + private: + static int createNewSegment(const std::string& name); + static int getExisting(const std::string& name, const ShmSegmentOpts& opts); + + // returns the key type corresponding to the given name. + std::string getPath() const; + + // resize the segment + // @param size the new size + // @return none + // @throw Throws std::system_error with appropriate errno + void resize(size_t size) const; + + void createReferenceMapping(); + void deleteReferenceMapping() const; + + // file descriptor associated with the shm. This has FD_CLOEXEC set + // and once opened, we close this only on destruction of this object + int fd_{kInvalidFD}; +}; +} // namespace cachelib +} // namespace facebook diff --git a/cachelib/shm/PosixShmSegment.cpp b/cachelib/shm/PosixShmSegment.cpp index 9126e1ac8e..42c9e2ba33 100644 --- a/cachelib/shm/PosixShmSegment.cpp +++ b/cachelib/shm/PosixShmSegment.cpp @@ -32,7 +32,7 @@ typedef struct stat stat_t; namespace detail { -int shmOpenImpl(const char* name, int flags) { +static int shmOpenImpl(const char* name, int flags) { const int fd = shm_open(name, flags, kRWMode); if (fd != -1) { @@ -68,7 +68,7 @@ int shmOpenImpl(const char* name, int flags) { return kInvalidFD; } -void unlinkImpl(const char* const name) { +static void shmUnlinkImpl(const char* const name) { const int ret = shm_unlink(name); if (ret == 0) { return; @@ -89,7 +89,7 @@ void unlinkImpl(const char* const name) { } } -void ftruncateImpl(int fd, size_t size) { +static void ftruncateImpl(int fd, size_t size) { const int ret = ftruncate(fd, size); if (ret == 0) { return; @@ -105,7 +105,7 @@ void ftruncateImpl(int fd, size_t size) { } } -void fstatImpl(int fd, stat_t* buf) { +static void fstatImpl(int fd, stat_t* buf) { const int ret = fstat(fd, buf); if (ret == 0) { return; @@ -122,7 +122,7 @@ void fstatImpl(int fd, stat_t* buf) { } } -void* mmapImpl( +static void* mmapImpl( void* addr, size_t length, int prot, int flags, int fd, off_t offset) { void* ret = mmap(addr, length, prot, flags, fd, offset); if (ret != MAP_FAILED) { @@ -153,7 +153,7 @@ void* mmapImpl( return nullptr; } -void munmapImpl(void* addr, size_t length) { +static void munmapImpl(void* addr, size_t length) { const int ret = munmap(addr, length); if (ret == 0) { @@ -239,7 +239,7 @@ void PosixShmSegment::markForRemoval() { bool PosixShmSegment::removeByName(const std::string& segmentName) { try { auto key = createKeyForName(segmentName); - detail::unlinkImpl(key.c_str()); + detail::shmUnlinkImpl(key.c_str()); return true; } catch (const std::system_error& e) { // unlink is opaque unlike sys-V api where its through the shmid. Hence diff --git a/cachelib/shm/PosixShmSegment.h b/cachelib/shm/PosixShmSegment.h index 13ce8ff5ee..da5050a290 100644 --- a/cachelib/shm/PosixShmSegment.h +++ b/cachelib/shm/PosixShmSegment.h @@ -22,8 +22,6 @@ namespace facebook { namespace cachelib { -constexpr int kInvalidFD = -1; - /* This class lets you manage a posix shared memory segment identified by * name. This is very similar to the System V shared memory segment, except * that it allows for resizing of the segments on the fly. This can let the diff --git a/cachelib/shm/Shm.h b/cachelib/shm/Shm.h index 334f053b88..626fb7fa12 100644 --- a/cachelib/shm/Shm.h +++ b/cachelib/shm/Shm.h @@ -22,6 +22,7 @@ #include #include "cachelib/common/Utils.h" +#include "cachelib/shm/FileShmSegment.h" #include "cachelib/shm/PosixShmSegment.h" #include "cachelib/shm/ShmCommon.h" #include "cachelib/shm/SysVShmSegment.h" @@ -50,14 +51,17 @@ class ShmSegment { ShmSegment(ShmNewT, std::string name, size_t size, - bool usePosix, ShmSegmentOpts opts = {}) { - if (usePosix) { - segment_ = std::make_unique(ShmNew, std::move(name), - size, opts); - } else { - segment_ = - std::make_unique(ShmNew, std::move(name), size, opts); + if (auto *v = std::get_if(&opts.typeOpts)) { + segment_ = std::make_unique( + ShmNew, std::move(name), size, opts); + } else if (auto *v = std::get_if(&opts.typeOpts)) { + if (v->usePosix) + segment_ = std::make_unique( + ShmNew, std::move(name), size, opts); + else + segment_ = std::make_unique( + ShmNew, std::move(name), size, opts); } } @@ -66,14 +70,17 @@ class ShmSegment { // @param opts the options for the segment. ShmSegment(ShmAttachT, std::string name, - bool usePosix, ShmSegmentOpts opts = {}) { - if (usePosix) { - segment_ = - std::make_unique(ShmAttach, std::move(name), opts); - } else { - segment_ = - std::make_unique(ShmAttach, std::move(name), opts); + if (std::get_if(&opts.typeOpts)) { + segment_ = std::make_unique( + ShmAttach, std::move(name), opts); + } else if (auto *v = std::get_if(&opts.typeOpts)) { + if (v->usePosix) + segment_ = std::make_unique( + ShmAttach, std::move(name), opts); + else + segment_ = std::make_unique( + ShmAttach, std::move(name), opts); } } diff --git a/cachelib/shm/ShmCommon.h b/cachelib/shm/ShmCommon.h index ebccbf68b7..c3363f4e34 100644 --- a/cachelib/shm/ShmCommon.h +++ b/cachelib/shm/ShmCommon.h @@ -18,6 +18,7 @@ #include #include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wconversion" @@ -37,13 +38,35 @@ enum PageSizeT { ONE_GB, }; +constexpr int kInvalidFD = -1; + +// TODO(SHM_FILE): maybe we could use this inside the Tier Config class? +struct FileShmSegmentOpts { + FileShmSegmentOpts(std::string path = ""): path(path) {} + std::string path; +}; + +struct PosixSysVSegmentOpts { + PosixSysVSegmentOpts(bool usePosix = false): usePosix(usePosix) {} + bool usePosix; +}; + +using ShmTypeOpts = std::variant; + struct ShmSegmentOpts { PageSizeT pageSize{PageSizeT::NORMAL}; bool readOnly{false}; size_t alignment{1}; // alignment for mapping. + 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) {} ShmSegmentOpts() : pageSize(PageSizeT::NORMAL) {} }; diff --git a/cachelib/shm/ShmManager.cpp b/cachelib/shm/ShmManager.cpp index 6ac855a8d4..dacdda0670 100644 --- a/cachelib/shm/ShmManager.cpp +++ b/cachelib/shm/ShmManager.cpp @@ -205,24 +205,34 @@ typename ShmManager::ShutDownRes ShmManager::shutDown() { namespace { -bool removeSegByName(bool posix, const std::string& uniqueName) { - return posix ? PosixShmSegment::removeByName(uniqueName) - : SysVShmSegment::removeByName(uniqueName); +bool removeSegByName(ShmTypeOpts typeOpts, const std::string& uniqueName) { + if (auto *v = std::get_if(&typeOpts)) { + return FileShmSegment::removeByPath(v->path); + } + + bool usePosix = std::get(typeOpts).usePosix; + if (usePosix) { + return PosixShmSegment::removeByName(uniqueName); + } else { + return SysVShmSegment::removeByName(uniqueName); + } } } // namespace void ShmManager::removeByName(const std::string& dir, const std::string& name, - bool posix) { - removeSegByName(posix, uniqueIdForName(name, dir)); + ShmTypeOpts typeOpts) { + removeSegByName(typeOpts, uniqueIdForName(name, dir)); } bool ShmManager::segmentExists(const std::string& cacheDir, const std::string& shmName, - bool posix) { + ShmTypeOpts typeOpts) { try { - ShmSegment(ShmAttach, uniqueIdForName(shmName, cacheDir), posix); + ShmSegmentOpts opts; + opts.typeOpts = typeOpts; + ShmSegment(ShmAttach, uniqueIdForName(shmName, cacheDir), opts); return true; } catch (const std::exception& e) { return false; @@ -230,10 +240,10 @@ bool ShmManager::segmentExists(const std::string& cacheDir, } std::unique_ptr ShmManager::attachShmReadOnly( - const std::string& dir, const std::string& name, bool posix, void* addr) { + const std::string& dir, const std::string& name, ShmTypeOpts typeOpts, void* addr) { ShmSegmentOpts opts{PageSizeT::NORMAL, true /* read only */}; - auto shm = std::make_unique(ShmAttach, uniqueIdForName(name, dir), - posix, opts); + opts.typeOpts = typeOpts; + auto shm = std::make_unique(ShmAttach, uniqueIdForName(name, dir), opts); if (!shm->mapAddress(addr)) { throw std::invalid_argument(folly::sformat( "Error mapping shm {} under {}, addr: {}", name, dir, addr)); @@ -248,6 +258,7 @@ void ShmManager::cleanup(const std::string& dir, bool posix) { } void ShmManager::removeAllSegments() { + // TODO(SHM_FILE): extend this once we have opts stored in nameToKey_ for (const auto& kv : nameToKey_) { removeSegByName(usePosix_, uniqueIdForName(kv.first)); } @@ -255,6 +266,7 @@ void ShmManager::removeAllSegments() { } void ShmManager::removeUnAttachedSegments() { + // TODO(SHM_FILE): extend this once we have opts stored in nameToKey_ auto it = nameToKey_.begin(); while (it != nameToKey_.end()) { const auto name = it->first; @@ -275,15 +287,24 @@ ShmAddr ShmManager::createShm(const std::string& shmName, // we are going to create a new segment most likely after trying to attach // to an old one. detach and remove any old ones if they have already been // attached or mapped - removeShm(shmName); + // TODO(SHM_FILE): should we try to remove the segment using all possible + // segment types? + removeShm(shmName, opts.typeOpts); DCHECK(segments_.find(shmName) == segments_.end()); DCHECK(nameToKey_.find(shmName) == nameToKey_.end()); + if (auto *v = std::get_if(&opts.typeOpts)) { + if (usePosix_ != v->usePosix) + throw std::invalid_argument( + folly::sformat("Expected {} but got {} segment", + usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); + } + std::unique_ptr newSeg; try { newSeg = std::make_unique(ShmNew, uniqueIdForName(shmName), - size, usePosix_, opts); + size, opts); } catch (const std::system_error& e) { // if segment already exists by this key and we dont know about // it(EEXIST), its an invalid state. @@ -318,12 +339,19 @@ void ShmManager::attachNewShm(const std::string& shmName, ShmSegmentOpts opts) { folly::sformat("Unable to find any segment with name {}", shmName)); } + if (auto *v = std::get_if(&opts.typeOpts)) { + if (usePosix_ != v->usePosix) + throw std::invalid_argument( + folly::sformat("Expected {} but got {} segment", + usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); + } + // This means the segment exists and we can try to attach it. try { segments_.emplace(shmName, std::make_unique(ShmAttach, uniqueIdForName(shmName), - usePosix_, opts)); + opts)); } catch (const std::system_error& e) { // we are trying to attach. nothing can get invalid if an error happens // here. @@ -357,7 +385,7 @@ ShmAddr ShmManager::attachShm(const std::string& shmName, return shm.getCurrentMapping(); } -bool ShmManager::removeShm(const std::string& shmName) { +bool ShmManager::removeShm(const std::string& shmName, ShmTypeOpts typeOpts) { try { auto& shm = getShmByName(shmName); shm.detachCurrentMapping(); @@ -372,7 +400,7 @@ bool ShmManager::removeShm(const std::string& shmName) { } catch (const std::invalid_argument&) { // shm by this name is not attached. const bool wasPresent = - removeSegByName(usePosix_, uniqueIdForName(shmName)); + removeSegByName(typeOpts, uniqueIdForName(shmName)); if (!wasPresent) { DCHECK(segments_.end() == segments_.find(shmName)); DCHECK(nameToKey_.end() == nameToKey_.find(shmName)); diff --git a/cachelib/shm/ShmManager.h b/cachelib/shm/ShmManager.h index 34c6abc66c..21ad173b3d 100644 --- 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); + bool removeShm(const std::string& segName, ShmTypeOpts opts); // gets a current segment by the name that is managed by this // instance. The lifetime of the returned object is same as the @@ -128,13 +128,13 @@ class ShmManager { // cacheDir without instanciating. static void removeByName(const std::string& cacheDir, const std::string& segName, - bool posix); + ShmTypeOpts shmOpts); // Useful for checking whether a segment exists by name associated with a // given cacheDir without instanciating. This should be ONLY used in tests. static bool segmentExists(const std::string& cacheDir, const std::string& segName, - bool posix); + ShmTypeOpts shmOpts); // free up and remove all the segments related to the cache directory. static void cleanup(const std::string& cacheDir, bool posix); @@ -152,7 +152,7 @@ class ShmManager { static std::unique_ptr attachShmReadOnly( const std::string& cacheDir, const std::string& segName, - bool posix, + ShmTypeOpts opts, void* addr = nullptr); private: From cc5643ca9c5af13c8478090447677dfcedc6d2b4 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Fri, 15 Oct 2021 22:13:55 -0400 Subject: [PATCH 03/13] Adjust and enable tests for ShmFileSegment --- .../memory/tests/SlabAllocatorTest.cpp | 4 +- cachelib/shm/tests/common.h | 40 +- cachelib/shm/tests/test_page_size.cpp | 20 +- cachelib/shm/tests/test_shm.cpp | 55 +-- cachelib/shm/tests/test_shm_death_style.cpp | 24 +- cachelib/shm/tests/test_shm_manager.cpp | 380 +++++++++++------- 6 files changed, 331 insertions(+), 192 deletions(-) diff --git a/cachelib/allocator/memory/tests/SlabAllocatorTest.cpp b/cachelib/allocator/memory/tests/SlabAllocatorTest.cpp index 337b5edbcc..6b1d0c8773 100644 --- 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); }; + SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); }; 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); }; + SCOPE_EXIT { shmManager.removeShm(shmName, PosixSysVSegmentOpts(false)); }; { SlabAllocator s(memory, size, config); diff --git a/cachelib/shm/tests/common.h b/cachelib/shm/tests/common.h index 8b2605fe57..b7baa435a7 100644 --- a/cachelib/shm/tests/common.h +++ b/cachelib/shm/tests/common.h @@ -69,6 +69,7 @@ class ShmTest : public ShmTestBase { // parallel by fbmake runtests. const std::string segmentName{}; const size_t shmSize{0}; + ShmSegmentOpts opts; protected: void SetUp() final { @@ -87,17 +88,19 @@ class ShmTest : public ShmTestBase { virtual void clearSegment() = 0; // common tests - void testCreateAttach(bool posix); - void testAttachReadOnly(bool posix); - void testMapping(bool posix); - void testMappingAlignment(bool posix); - void testLifetime(bool posix); - void testPageSize(PageSizeT, bool posix); + void testCreateAttach(); + void testAttachReadOnly(); + void testMapping(); + void testMappingAlignment(); + void testLifetime(); + void testPageSize(PageSizeT); }; class ShmTestPosix : public ShmTest { public: - ShmTestPosix() {} + ShmTestPosix() { + opts.typeOpts = PosixSysVSegmentOpts(true); + } private: void clearSegment() override { @@ -113,7 +116,9 @@ class ShmTestPosix : public ShmTest { class ShmTestSysV : public ShmTest { public: - ShmTestSysV() {} + ShmTestSysV() { + opts.typeOpts = PosixSysVSegmentOpts(false); + } private: void clearSegment() override { @@ -126,6 +131,25 @@ class ShmTestSysV : public ShmTest { } } }; + +class ShmTestFile : public ShmTest { + public: + ShmTestFile() { + opts.typeOpts = FileShmSegmentOpts("/tmp/" + segmentName); + } + + private: + void clearSegment() override { + try { + auto path = std::get(opts.typeOpts).path; + FileShmSegment::removeByPath(path); + } catch (const std::system_error& e) { + if (e.code().value() != ENOENT) { + throw; + } + } + } +}; } // namespace tests } // namespace cachelib } // namespace facebook diff --git a/cachelib/shm/tests/test_page_size.cpp b/cachelib/shm/tests/test_page_size.cpp index 8ebe5b249c..52084d96e9 100644 --- a/cachelib/shm/tests/test_page_size.cpp +++ b/cachelib/shm/tests/test_page_size.cpp @@ -28,20 +28,20 @@ namespace facebook { namespace cachelib { namespace tests { -void ShmTest::testPageSize(PageSizeT p, bool posix) { - ShmSegmentOpts opts{p}; +void ShmTest::testPageSize(PageSizeT p) { + opts.pageSize = p; size_t size = getPageAlignedSize(4096, p); ASSERT_TRUE(isPageAlignedSize(size, p)); // create with unaligned size ASSERT_NO_THROW({ - ShmSegment s(ShmNew, segmentName, size, posix, opts); + ShmSegment s(ShmNew, segmentName, size, opts); ASSERT_TRUE(s.mapAddress(nullptr)); ASSERT_EQ(p, getPageSizeInSMap(s.getCurrentMapping().addr)); }); ASSERT_NO_THROW({ - ShmSegment s2(ShmAttach, segmentName, posix, opts); + ShmSegment s2(ShmAttach, segmentName, opts); ASSERT_TRUE(s2.mapAddress(nullptr)); ASSERT_EQ(p, getPageSizeInSMap(s2.getCurrentMapping().addr)); }); @@ -52,13 +52,17 @@ void ShmTest::testPageSize(PageSizeT p, bool posix) { // complete yet. See https://fburl.com/f0umrcwq . We will re-enable these // tests on sandcastle when these get fixed. -TEST_F(ShmTestPosix, PageSizesNormal) { testPageSize(PageSizeT::NORMAL, true); } +TEST_F(ShmTestPosix, PageSizesNormal) { testPageSize(PageSizeT::NORMAL); } -TEST_F(ShmTestPosix, PageSizesTwoMB) { testPageSize(PageSizeT::TWO_MB, true); } +TEST_F(ShmTestPosix, PageSizesTwoMB) { testPageSize(PageSizeT::TWO_MB); } -TEST_F(ShmTestSysV, PageSizesNormal) { testPageSize(PageSizeT::NORMAL, false); } +TEST_F(ShmTestSysV, PageSizesNormal) { testPageSize(PageSizeT::NORMAL); } -TEST_F(ShmTestSysV, PageSizesTwoMB) { testPageSize(PageSizeT::TWO_MB, false); } +TEST_F(ShmTestSysV, PageSizesTwoMB) { testPageSize(PageSizeT::TWO_MB); } + +TEST_F(ShmTestFile, PageSizesNormal) { testPageSize(PageSizeT::NORMAL); } + +TEST_F(ShmTestFile, PageSizesTwoMB) { testPageSize(PageSizeT::TWO_MB); } } // namespace tests } // namespace cachelib diff --git a/cachelib/shm/tests/test_shm.cpp b/cachelib/shm/tests/test_shm.cpp index 822c6f7455..2b3baccf18 100644 --- a/cachelib/shm/tests/test_shm.cpp +++ b/cachelib/shm/tests/test_shm.cpp @@ -28,11 +28,11 @@ using facebook::cachelib::detail::getPageSize; using facebook::cachelib::detail::getPageSizeInSMap; using facebook::cachelib::detail::isPageAlignedSize; -void ShmTest::testCreateAttach(bool posix) { +void ShmTest::testCreateAttach() { const unsigned char magicVal = 'd'; { // create with 0 size should round up to page size - ShmSegment s(ShmNew, segmentName, 0, posix); + ShmSegment s(ShmNew, segmentName, 0, opts); ASSERT_EQ(getPageSize(), s.getSize()); s.markForRemoval(); } @@ -40,14 +40,14 @@ void ShmTest::testCreateAttach(bool posix) { { // create with unaligned size ASSERT_TRUE(isPageAlignedSize(shmSize)); - ShmSegment s(ShmNew, segmentName, shmSize + 500, posix); + ShmSegment s(ShmNew, segmentName, shmSize + 500, opts); ASSERT_EQ(shmSize + getPageSize(), s.getSize()); s.markForRemoval(); } auto addr = getNewUnmappedAddr(); { - ShmSegment s(ShmNew, segmentName, shmSize, posix); + ShmSegment s(ShmNew, segmentName, shmSize, opts); ASSERT_EQ(s.getSize(), shmSize); ASSERT_FALSE(s.isMapped()); ASSERT_TRUE(s.mapAddress(addr)); @@ -57,14 +57,14 @@ void ShmTest::testCreateAttach(bool posix) { ASSERT_TRUE(s.isMapped()); checkMemory(addr, s.getSize(), 0); writeToMemory(addr, s.getSize(), magicVal); - ASSERT_THROW(ShmSegment(ShmNew, segmentName, shmSize, posix), + ASSERT_THROW(ShmSegment(ShmNew, segmentName, shmSize, opts), std::system_error); const auto m = s.getCurrentMapping(); ASSERT_EQ(m.size, shmSize); } ASSERT_NO_THROW({ - ShmSegment s2(ShmAttach, segmentName, posix); + ShmSegment s2(ShmAttach, segmentName, opts); ASSERT_EQ(s2.getSize(), shmSize); ASSERT_TRUE(s2.mapAddress(addr)); checkMemory(addr, s2.getSize(), magicVal); @@ -73,15 +73,17 @@ void ShmTest::testCreateAttach(bool posix) { }); } -TEST_F(ShmTestPosix, CreateAttach) { testCreateAttach(true); } +TEST_F(ShmTestPosix, CreateAttach) { testCreateAttach(); } -TEST_F(ShmTestSysV, CreateAttach) { testCreateAttach(false); } +TEST_F(ShmTestSysV, CreateAttach) { testCreateAttach(); } -void ShmTest::testMapping(bool posix) { +TEST_F(ShmTestFile, CreateAttach) { testCreateAttach(); } + +void ShmTest::testMapping() { const unsigned char magicVal = 'z'; auto addr = getNewUnmappedAddr(); { // create a segment - ShmSegment s(ShmNew, segmentName, shmSize, posix); + ShmSegment s(ShmNew, segmentName, shmSize, opts); ASSERT_TRUE(s.mapAddress(addr)); ASSERT_TRUE(s.isMapped()); // creating another mapping should fail @@ -95,7 +97,7 @@ void ShmTest::testMapping(bool posix) { // map with nullptr { - ShmSegment s(ShmAttach, segmentName, posix); + ShmSegment s(ShmAttach, segmentName, opts); ASSERT_TRUE(s.mapAddress(nullptr)); ASSERT_TRUE(s.isMapped()); const auto m = s.getCurrentMapping(); @@ -107,7 +109,7 @@ void ShmTest::testMapping(bool posix) { } { - ShmSegment s(ShmAttach, segmentName, posix); + ShmSegment s(ShmAttach, segmentName, opts); // can map again. ASSERT_TRUE(s.mapAddress(addr)); ASSERT_TRUE(s.isMapped()); @@ -148,13 +150,15 @@ void ShmTest::testMapping(bool posix) { } } -TEST_F(ShmTestPosix, Mapping) { testMapping(true); } +TEST_F(ShmTestPosix, Mapping) { testMapping(); } + +TEST_F(ShmTestSysV, Mapping) { testMapping(); } -TEST_F(ShmTestSysV, Mapping) { testMapping(false); } +TEST_F(ShmTestFile, Mapping) { testMapping(); } -void ShmTest::testMappingAlignment(bool posix) { +void ShmTest::testMappingAlignment() { { // create a segment - ShmSegment s(ShmNew, segmentName, shmSize, posix); + ShmSegment s(ShmNew, segmentName, shmSize, opts); // 0 alignment is wrong. ASSERT_FALSE(s.mapAddress(nullptr, 0)); @@ -171,11 +175,13 @@ void ShmTest::testMappingAlignment(bool posix) { } } -TEST_F(ShmTestPosix, MappingAlignment) { testMappingAlignment(true); } +TEST_F(ShmTestPosix, MappingAlignment) { testMappingAlignment(); } + +TEST_F(ShmTestSysV, MappingAlignment) { testMappingAlignment(); } -TEST_F(ShmTestSysV, MappingAlignment) { testMappingAlignment(false); } +TEST_F(ShmTestFile, MappingAlignment) { testMappingAlignment(); } -void ShmTest::testLifetime(bool posix) { +void ShmTest::testLifetime() { const size_t safeSize = getRandomSize(); const char magicVal = 'x'; ASSERT_NO_THROW({ @@ -184,7 +190,7 @@ void ShmTest::testLifetime(bool posix) { // from address space. this should not actually delete the segment and // we should be able to map it back as long as the object is within the // scope. - ShmSegment s(ShmNew, segmentName, safeSize, posix); + ShmSegment s(ShmNew, segmentName, safeSize, opts); s.mapAddress(nullptr); auto m = s.getCurrentMapping(); writeToMemory(m.addr, m.size, magicVal); @@ -200,14 +206,14 @@ void ShmTest::testLifetime(bool posix) { // should be able to create a new segment with same segmentName after the // previous scope exit destroys the segment. const size_t newSize = getRandomSize(); - ShmSegment s(ShmNew, segmentName, newSize, posix); + ShmSegment s(ShmNew, segmentName, newSize, opts); s.mapAddress(nullptr); auto m = s.getCurrentMapping(); checkMemory(m.addr, m.size, 0); writeToMemory(m.addr, m.size, magicVal); } // attaching should have the same behavior. - ShmSegment s(ShmAttach, segmentName, posix); + ShmSegment s(ShmAttach, segmentName, opts); s.mapAddress(nullptr); s.markForRemoval(); ASSERT_TRUE(s.isMarkedForRemoval()); @@ -218,5 +224,6 @@ void ShmTest::testLifetime(bool posix) { }); } -TEST_F(ShmTestPosix, Lifetime) { testLifetime(true); } -TEST_F(ShmTestSysV, Lifetime) { testLifetime(false); } +TEST_F(ShmTestPosix, Lifetime) { testLifetime(); } +TEST_F(ShmTestSysV, Lifetime) { testLifetime(); } +TEST_F(ShmTestFile, Lifetime) { testLifetime(); } diff --git a/cachelib/shm/tests/test_shm_death_style.cpp b/cachelib/shm/tests/test_shm_death_style.cpp index 2b132c53aa..263df19914 100644 --- a/cachelib/shm/tests/test_shm_death_style.cpp +++ b/cachelib/shm/tests/test_shm_death_style.cpp @@ -26,22 +26,24 @@ using namespace facebook::cachelib::tests; using facebook::cachelib::detail::isPageAlignedSize; -void ShmTest::testAttachReadOnly(bool posix) { +void ShmTest::testAttachReadOnly() { unsigned char magicVal = 'd'; ShmSegmentOpts ropts{PageSizeT::NORMAL, true /* read Only */}; + ropts.typeOpts = opts.typeOpts; ShmSegmentOpts rwopts{PageSizeT::NORMAL, false /* read Only */}; + rwopts.typeOpts = opts.typeOpts; { // attaching to something that does not exist should fail in read only // mode. ASSERT_TRUE(isPageAlignedSize(shmSize)); - ASSERT_THROW(ShmSegment(ShmAttach, segmentName, posix, ropts), + ASSERT_THROW(ShmSegment(ShmAttach, segmentName, ropts), std::system_error); } // create a new segment { - ShmSegment s(ShmNew, segmentName, shmSize, posix, rwopts); + ShmSegment s(ShmNew, segmentName, shmSize, rwopts); ASSERT_EQ(s.getSize(), shmSize); ASSERT_TRUE(s.mapAddress(nullptr)); ASSERT_TRUE(s.isMapped()); @@ -51,7 +53,7 @@ void ShmTest::testAttachReadOnly(bool posix) { } ASSERT_NO_THROW({ - ShmSegment s(ShmAttach, segmentName, posix, rwopts); + ShmSegment s(ShmAttach, segmentName, rwopts); ASSERT_EQ(s.getSize(), shmSize); ASSERT_TRUE(s.mapAddress(nullptr)); void* addr = s.getCurrentMapping().addr; @@ -65,8 +67,8 @@ void ShmTest::testAttachReadOnly(bool posix) { // reading in read only mode should work fine. while another one is // attached. ASSERT_NO_THROW({ - ShmSegment s(ShmAttach, segmentName, posix, ropts); - ShmSegment s2(ShmAttach, segmentName, posix, rwopts); + ShmSegment s(ShmAttach, segmentName, ropts); + ShmSegment s2(ShmAttach, segmentName, rwopts); ASSERT_EQ(s.getSize(), shmSize); ASSERT_TRUE(s.mapAddress(nullptr)); void* addr = s.getCurrentMapping().addr; @@ -89,7 +91,7 @@ void ShmTest::testAttachReadOnly(bool posix) { // detached. segment should be present after it. ASSERT_DEATH( { - ShmSegment s(ShmAttach, segmentName, posix, ropts); + ShmSegment s(ShmAttach, segmentName, ropts); ASSERT_EQ(s.getSize(), shmSize); ASSERT_TRUE(s.mapAddress(nullptr)); void* addr = s.getCurrentMapping().addr; @@ -101,12 +103,14 @@ void ShmTest::testAttachReadOnly(bool posix) { }, ".*"); - ASSERT_NO_THROW(ShmSegment s(ShmAttach, segmentName, posix, ropts)); + ASSERT_NO_THROW(ShmSegment s(ShmAttach, segmentName, ropts)); } -TEST_F(ShmTestPosix, AttachReadOnlyDeathTest) { testAttachReadOnly(true); } +TEST_F(ShmTestPosix, AttachReadOnlyDeathTest) { testAttachReadOnly(); } -TEST_F(ShmTestSysV, AttachReadOnlyDeathTest) { testAttachReadOnly(false); } +TEST_F(ShmTestSysV, AttachReadOnlyDeathTest) { testAttachReadOnly(); } + +TEST_F(ShmTestFile, AttachReadOnlyDeathTest) { testAttachReadOnly(); } int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); diff --git a/cachelib/shm/tests/test_shm_manager.cpp b/cachelib/shm/tests/test_shm_manager.cpp index bc72bb1184..26f8686975 100644 --- a/cachelib/shm/tests/test_shm_manager.cpp +++ b/cachelib/shm/tests/test_shm_manager.cpp @@ -31,6 +31,10 @@ static const std::string namePrefix = "shm-test"; using namespace facebook::cachelib::tests; using facebook::cachelib::ShmManager; +using facebook::cachelib::ShmSegmentOpts; +using facebook::cachelib::ShmTypeOpts; +using facebook::cachelib::PosixSysVSegmentOpts; +using facebook::cachelib::FileShmSegmentOpts; using ShutDownRes = typename facebook::cachelib::ShmManager::ShutDownRes; @@ -39,9 +43,10 @@ class ShmManagerTest : public ShmTestBase { ShmManagerTest() : cacheDir(dirPrefix + std::to_string(::getpid())) {} const std::string cacheDir{}; - std::vector segmentsToDestroy{}; protected: + std::vector> segmentsToDestroy{}; + void SetUp() final { // make sure nothing exists at the start facebook::cachelib::util::removePath(cacheDir); @@ -62,8 +67,18 @@ class ShmManagerTest : public ShmTestBase { } } + virtual std::pair makeSegmentImpl( + std::string name) = 0; virtual void clearAllSegments() = 0; + std::pair makeSegment(std::string name, + bool addToDestroy = true) { + auto val = makeSegmentImpl(name); + if (addToDestroy) + segmentsToDestroy.push_back(val); + return val; + } + /* * We define the generic test here that can be run by the appropriate * specification of the test fixture by their shm type @@ -88,18 +103,48 @@ class ShmManagerTest : public ShmTestBase { class ShmManagerTestSysV : public ShmManagerTest { public: + virtual std::pair makeSegmentImpl(std::string name) + override { + ShmSegmentOpts opts; + opts.typeOpts = PosixSysVSegmentOpts{false}; + return std::pair{name, opts}; + } + void clearAllSegments() override { for (const auto& seg : segmentsToDestroy) { - ShmManager::removeByName(cacheDir, seg, false); + ShmManager::removeByName(cacheDir, seg.first, seg.second.typeOpts); } } }; class ShmManagerTestPosix : public ShmManagerTest { public: + virtual std::pair makeSegmentImpl(std::string name) + override { + ShmSegmentOpts opts; + opts.typeOpts = PosixSysVSegmentOpts{true}; + return std::pair{name, opts}; + } + void clearAllSegments() override { for (const auto& seg : segmentsToDestroy) { - ShmManager::removeByName(cacheDir, seg, true); + ShmManager::removeByName(cacheDir, seg.first, seg.second.typeOpts); + } + } +}; + +class ShmManagerTestFile : public ShmManagerTest { + public: + virtual std::pair makeSegmentImpl(std::string name) + override { + ShmSegmentOpts opts; + opts.typeOpts = FileShmSegmentOpts{"/tmp/" + name}; + return std::pair{name, opts}; + } + + void clearAllSegments() override { + for (const auto& seg : segmentsToDestroy) { + ShmManager::removeByName(cacheDir, seg.first, seg.second.typeOpts); } } }; @@ -107,17 +152,22 @@ class ShmManagerTestPosix : public ShmManagerTest { const std::string ShmManagerTest::dirPrefix = "/tmp/shm-test"; void ShmManagerTest::testMetaFileDeletion(bool posix) { - const std::string segmentName = std::to_string(::getpid()); - const std::string segmentName2 = segmentName + "-2"; - segmentsToDestroy.push_back(segmentName); - segmentsToDestroy.push_back(segmentName2); + int num = 0; + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; + const size_t size = getRandomSize(); const unsigned char magicVal = 'g'; // start the session with the first type and create some segments. auto addr = getNewUnmappedAddr(); { ShmManager s(cacheDir, posix); - auto m = s.createShm(segmentName, size, addr); + auto m = s.createShm(seg1, size, addr, seg1Opt); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -136,8 +186,9 @@ void ShmManagerTest::testMetaFileDeletion(bool posix) { // now try to attach and that should fail. { ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(segmentName), std::invalid_argument); - auto m = s.createShm(segmentName, size, addr); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), + std::invalid_argument); + auto m = s.createShm(seg1, size, addr, seg1Opt); checkMemory(m.addr, m.size, 0); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -153,8 +204,9 @@ void ShmManagerTest::testMetaFileDeletion(bool posix) { // now try to attach and that should fail. { ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(segmentName), std::invalid_argument); - auto m = s.createShm(segmentName, size, addr); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), + std::invalid_argument); + auto m = s.createShm(seg1, size, addr, seg1Opt); checkMemory(m.addr, m.size, 0); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -166,23 +218,24 @@ void ShmManagerTest::testMetaFileDeletion(bool posix) { { ShmManager s(cacheDir, posix); ASSERT_NO_THROW({ - const auto m = s.attachShm(segmentName, addr); + const auto m = s.attachShm(seg1, addr, seg1Opt); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); }); ASSERT_NO_THROW({ - const auto m2 = s.createShm(segmentName2, size, nullptr); + const auto m2 = s.createShm(seg2, size, nullptr, + seg2Opt); writeToMemory(m2.addr, m2.size, magicVal); checkMemory(m2.addr, m2.size, magicVal); }); // simulate this being destroyed outside of shm manager. - ShmManager::removeByName(cacheDir, segmentName, posix); + ShmManager::removeByName(cacheDir, seg1, seg1Opt.typeOpts); // now detach. This will cause us to have a segment that we managed // disappear beneath us. - s.getShmByName(segmentName).detachCurrentMapping(); + s.getShmByName(seg1).detachCurrentMapping(); // delete the meta file ASSERT_TRUE(facebook::cachelib::util::pathExists(cacheDir + "/metadata")); @@ -199,23 +252,23 @@ void ShmManagerTest::testMetaFileDeletion(bool posix) { { ShmManager s(cacheDir, posix); ASSERT_NO_THROW({ - const auto m = s.createShm(segmentName, size, addr); + const auto m = s.createShm(seg1, size, addr, seg1Opt); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); }); ASSERT_NO_THROW({ - const auto m2 = s.createShm(segmentName2, size, nullptr); + const auto m2 = s.createShm(seg2, size, nullptr, seg2Opt); writeToMemory(m2.addr, m2.size, magicVal); checkMemory(m2.addr, m2.size, magicVal); }); // simulate this being destroyed outside of shm manager. - ShmManager::removeByName(cacheDir, segmentName, posix); + ShmManager::removeByName(cacheDir, seg1, seg1Opt.typeOpts); // now detach. This will cause us to have a segment that we managed // disappear beneath us. - s.getShmByName(segmentName).detachCurrentMapping(); + s.getShmByName(seg1).detachCurrentMapping(); // shutdown should work as expected. ASSERT_NO_THROW(ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess)); @@ -226,18 +279,21 @@ TEST_F(ShmManagerTestPosix, MetaFileDeletion) { testMetaFileDeletion(true); } TEST_F(ShmManagerTestSysV, MetaFileDeletion) { testMetaFileDeletion(false); } +TEST_F(ShmManagerTestFile, MetaFileDeletion) { testMetaFileDeletion(false); } + void ShmManagerTest::testDropFile(bool posix) { - const std::string segmentName = std::to_string(::getpid()); - const std::string segmentName2 = segmentName + "-2"; - segmentsToDestroy.push_back(segmentName); - segmentsToDestroy.push_back(segmentName2); + int num = 0; + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg1Opt = segment1.second; const size_t size = getRandomSize(); const unsigned char magicVal = 'g'; // start the session with the first type and create some segments. auto addr = getNewUnmappedAddr(); { ShmManager s(cacheDir, posix); - auto m = s.createShm(segmentName, size, addr); + auto m = s.createShm(seg1, size, addr, seg1Opt); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -254,8 +310,9 @@ void ShmManagerTest::testDropFile(bool posix) { { ShmManager s(cacheDir, posix); ASSERT_FALSE(facebook::cachelib::util::pathExists(cacheDir + "/ColdRoll")); - ASSERT_THROW(s.attachShm(segmentName), std::invalid_argument); - auto m = s.createShm(segmentName, size, addr); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), + std::invalid_argument); + auto m = s.createShm(seg1, size, addr, seg1Opt); checkMemory(m.addr, m.size, 0); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -265,7 +322,7 @@ void ShmManagerTest::testDropFile(bool posix) { // now try to attach and that should succeed. { ShmManager s(cacheDir, posix); - auto m = s.attachShm(segmentName, addr); + auto m = s.attachShm(seg1, addr, seg1Opt); checkMemory(m.addr, m.size, magicVal); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); } @@ -287,7 +344,8 @@ void ShmManagerTest::testDropFile(bool posix) { // now try to attach and that should fail due to previous cold roll { ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(segmentName), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), + std::invalid_argument); } } @@ -295,20 +353,25 @@ TEST_F(ShmManagerTestPosix, DropFile) { testDropFile(true); } TEST_F(ShmManagerTestSysV, DropFile) { testDropFile(false); } +TEST_F(ShmManagerTestFile, DropFile) { testDropFile(false); } + // Tests to ensure that when we shutdown with posix and restart with shm, we // dont mess things up and coming up with the wrong type fails. void ShmManagerTest::testInvalidType(bool posix) { // we ll create the instance with this type and try with the other type + int num = 0; + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg1Opt = segment1.second; - const std::string segmentName = std::to_string(::getpid()); - segmentsToDestroy.push_back(segmentName); const size_t size = getRandomSize(); const unsigned char magicVal = 'g'; // start the sesion with the first type and create some segments. auto addr = getNewUnmappedAddr(); { ShmManager s(cacheDir, posix); - auto m = s.createShm(segmentName, size, addr); + auto m = s.createShm(seg1, size, addr, seg1Opt); writeToMemory(m.addr, m.size, magicVal); checkMemory(m.addr, m.size, magicVal); @@ -323,7 +386,7 @@ void ShmManagerTest::testInvalidType(bool posix) { { ShmManager s(cacheDir, posix); - auto m = s.attachShm(segmentName, addr); + auto m = s.attachShm(seg1, addr, seg1Opt); checkMemory(m.addr, m.size, magicVal); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); @@ -334,19 +397,25 @@ TEST_F(ShmManagerTestPosix, InvalidType) { testInvalidType(true); } TEST_F(ShmManagerTestSysV, InvalidType) { testInvalidType(false); } +TEST_F(ShmManagerTestFile, InvalidType) { testInvalidType(false); } + void ShmManagerTest::testRemove(bool posix) { - const std::string seg1 = std::to_string(::getpid()) + "-0"; - const std::string seg2 = std::to_string(::getpid()) + "-1"; + int num = 0; + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; const size_t size = getRandomSize(); const unsigned char magicVal = 'x'; - segmentsToDestroy.push_back(seg1); - segmentsToDestroy.push_back(seg2); auto addr = getNewUnmappedAddr(); { ShmManager s(cacheDir, posix); - ASSERT_FALSE(s.removeShm(seg1)); - auto m1 = s.createShm(seg1, size, nullptr); - auto m2 = s.createShm(seg2, size, getNewUnmappedAddr()); + ASSERT_FALSE(s.removeShm(seg1, seg1Opt.typeOpts)); + auto m1 = s.createShm(seg1, size, nullptr, seg1Opt); + auto m2 = s.createShm(seg2, size, getNewUnmappedAddr(), seg2Opt); writeToMemory(m1.addr, m1.size, magicVal); writeToMemory(m2.addr, m2.size, magicVal); @@ -357,29 +426,29 @@ void ShmManagerTest::testRemove(bool posix) { { ShmManager s(cacheDir, posix); - auto m1 = s.attachShm(seg1, addr); + auto m1 = s.attachShm(seg1, addr, seg1Opt); auto& shm1 = s.getShmByName(seg1); checkMemory(m1.addr, m1.size, magicVal); - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); checkMemory(m2.addr, m2.size, magicVal); ASSERT_TRUE(shm1.isMapped()); - ASSERT_TRUE(s.removeShm(seg1)); + ASSERT_TRUE(s.removeShm(seg1, seg1Opt.typeOpts)); ASSERT_THROW(s.getShmByName(seg1), std::invalid_argument); // trying to remove now should indicate that the segment does not exist - ASSERT_FALSE(s.removeShm(seg1)); + ASSERT_FALSE(s.removeShm(seg1, seg1Opt.typeOpts)); s.shutDown(); } // attaching after shutdown should reflect the remove { ShmManager s(cacheDir, posix); - auto m1 = s.createShm(seg1, size, addr); + auto m1 = s.createShm(seg1, size, addr, seg1Opt); checkMemory(m1.addr, m1.size, 0); - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); checkMemory(m2.addr, m2.size, magicVal); s.shutDown(); } @@ -387,20 +456,20 @@ void ShmManagerTest::testRemove(bool posix) { // test detachAndRemove { ShmManager s(cacheDir, posix); - auto m1 = s.attachShm(seg1, addr); + auto m1 = s.attachShm(seg1, addr, seg1Opt); checkMemory(m1.addr, m1.size, 0); - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); auto& shm2 = s.getShmByName(seg2); checkMemory(m2.addr, m2.size, magicVal); // call detach and remove with an attached segment - ASSERT_TRUE(s.removeShm(seg1)); + ASSERT_TRUE(s.removeShm(seg1, seg1Opt.typeOpts)); ASSERT_THROW(s.getShmByName(seg1), std::invalid_argument); // call detach and remove with a detached segment shm2.detachCurrentMapping(); - ASSERT_TRUE(s.removeShm(seg2)); + ASSERT_TRUE(s.removeShm(seg2, seg2Opt.typeOpts)); ASSERT_THROW(s.getShmByName(seg2), std::invalid_argument); s.shutDown(); } @@ -416,31 +485,34 @@ TEST_F(ShmManagerTestPosix, Remove) { testRemove(true); } TEST_F(ShmManagerTestSysV, Remove) { testRemove(false); } +TEST_F(ShmManagerTestFile, Remove) { testRemove(false); } + void ShmManagerTest::testStaticCleanup(bool posix) { // pid-X to keep it unique so we dont collude with other tests int num = 0; - const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg1 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg2 = segmentPrefix + "-" + std::to_string(num++); + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; // open an instance and create some segments, write to the memory and // shutdown. ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - segmentsToDestroy.push_back(seg1); - s.createShm(seg1, getRandomSize()); - - segmentsToDestroy.push_back(seg2); - s.createShm(seg2, getRandomSize()); + s.createShm(seg1, getRandomSize(), nullptr, seg1Opt); + s.createShm(seg2, getRandomSize(), nullptr, seg2Opt); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); }); ASSERT_NO_THROW({ - ShmManager::removeByName(cacheDir, seg1, posix); + ShmManager::removeByName(cacheDir, seg1, seg1Opt.typeOpts); ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(seg1), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), std::invalid_argument); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); }); @@ -448,7 +520,7 @@ void ShmManagerTest::testStaticCleanup(bool posix) { ASSERT_NO_THROW({ ShmManager::cleanup(cacheDir, posix); ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(seg2), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg2, nullptr, seg1Opt), std::invalid_argument); }); } @@ -456,6 +528,8 @@ TEST_F(ShmManagerTestPosix, StaticCleanup) { testStaticCleanup(true); } TEST_F(ShmManagerTestSysV, StaticCleanup) { testStaticCleanup(false); } +TEST_F(ShmManagerTestFile, StaticCleanup) { testStaticCleanup(false); } + // test to ensure that if the directory is invalid, things fail void ShmManagerTest::testInvalidCachedDir(bool posix) { std::ofstream f(cacheDir); @@ -481,6 +555,8 @@ TEST_F(ShmManagerTestPosix, InvalidCacheDir) { testInvalidCachedDir(true); } TEST_F(ShmManagerTestSysV, InvalidCacheDir) { testInvalidCachedDir(false); } +TEST_F(ShmManagerTestFile, InvalidCacheDir) { testInvalidCachedDir(false); } + // test to ensure that random contents in the file cause it to fail void ShmManagerTest::testInvalidMetaFile(bool posix) { facebook::cachelib::util::makeDir(cacheDir); @@ -510,6 +586,8 @@ TEST_F(ShmManagerTestPosix, EmptyMetaFile) { testEmptyMetaFile(true); } TEST_F(ShmManagerTestSysV, EmptyMetaFile) { testEmptyMetaFile(false); } +TEST_F(ShmManagerTestFile, EmptyMetaFile) { testEmptyMetaFile(false); } + // test to ensure that segments can be created with a new cache dir, attached // from existing cache dir, segments can be deleted and recreated using the // same cache dir if they have not been attached to already. @@ -518,9 +596,13 @@ void ShmManagerTest::testSegments(bool posix) { const char magicVal2 = 'e'; // pid-X to keep it unique so we dont collude with other tests int num = 0; - const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg1 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg2 = segmentPrefix + "-" + std::to_string(num++); + auto segmentPrefix = std::to_string(::getpid()); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; auto addr = getNewUnmappedAddr(); // open an instance and create some segments, write to the memory and @@ -528,13 +610,11 @@ void ShmManagerTest::testSegments(bool posix) { ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - segmentsToDestroy.push_back(seg1); - auto m1 = s.createShm(seg1, getRandomSize(), addr); + auto m1 = s.createShm(seg1, getRandomSize(), addr, seg1Opt); writeToMemory(m1.addr, m1.size, magicVal1); checkMemory(m1.addr, m1.size, magicVal1); - segmentsToDestroy.push_back(seg2); - auto m2 = s.createShm(seg2, getRandomSize(), getNewUnmappedAddr()); + auto m2 = s.createShm(seg2, getRandomSize(), getNewUnmappedAddr(), seg2Opt); writeToMemory(m2.addr, m2.size, magicVal2); checkMemory(m2.addr, m2.size, magicVal2); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); @@ -545,12 +625,12 @@ void ShmManagerTest::testSegments(bool posix) { ShmManager s(cacheDir, posix); // attach - auto m1 = s.attachShm(seg1, addr); + auto m1 = s.attachShm(seg1, addr, seg1Opt); writeToMemory(m1.addr, m1.size, magicVal1); checkMemory(m1.addr, m1.size, magicVal1); // attach - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); writeToMemory(m2.addr, m2.size, magicVal2); checkMemory(m2.addr, m2.size, magicVal2); // no clean shutdown this time. @@ -560,21 +640,20 @@ void ShmManagerTest::testSegments(bool posix) { { ShmManager s(cacheDir, posix); // try attach, but it should fail. - ASSERT_THROW(s.attachShm(seg1), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), std::invalid_argument); // try attach - ASSERT_THROW(s.attachShm(seg2), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg2, nullptr, seg2Opt), std::invalid_argument); // now create new segments with same name. This should remove the // previous version of the segments with same name. ASSERT_NO_THROW({ - auto m1 = s.createShm(seg1, getRandomSize(), addr); + auto m1 = s.createShm(seg1, getRandomSize(), addr, seg1Opt); checkMemory(m1.addr, m1.size, 0); writeToMemory(m1.addr, m1.size, magicVal1); checkMemory(m1.addr, m1.size, magicVal1); - segmentsToDestroy.push_back(seg2); - auto m2 = s.createShm(seg2, getRandomSize(), getNewUnmappedAddr()); + auto m2 = s.createShm(seg2, getRandomSize(), getNewUnmappedAddr(), seg2Opt); checkMemory(m2.addr, m2.size, 0); writeToMemory(m2.addr, m2.size, magicVal2); checkMemory(m2.addr, m2.size, magicVal2); @@ -587,12 +666,12 @@ void ShmManagerTest::testSegments(bool posix) { // previous versions are removed. ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - auto m1 = s.createShm(seg1, getRandomSize(), addr); + auto m1 = s.createShm(seg1, getRandomSize(), addr, seg1Opt); // ensure its the new one. checkMemory(m1.addr, m1.size, 0); writeToMemory(m1.addr, m1.size, magicVal2); - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); // ensure that we attached to the previous segment. checkMemory(m2.addr, m2.size, magicVal2); writeToMemory(m2.addr, m2.size, magicVal1); @@ -606,11 +685,11 @@ void ShmManagerTest::testSegments(bool posix) { ShmManager s(cacheDir, posix); // attach - auto m1 = s.attachShm(seg1, addr); + auto m1 = s.attachShm(seg1, addr, seg1Opt); checkMemory(m1.addr, m1.size, magicVal2); // attach - auto m2 = s.attachShm(seg2, getNewUnmappedAddr()); + auto m2 = s.attachShm(seg2, getNewUnmappedAddr(), seg2Opt); checkMemory(m2.addr, m2.size, magicVal1); // no clean shutdown this time. }); @@ -620,13 +699,21 @@ TEST_F(ShmManagerTestPosix, Segments) { testSegments(true); } TEST_F(ShmManagerTestSysV, Segments) { testSegments(false); } +TEST_F(ShmManagerTestFile, Segments) { testSegments(false); } + void ShmManagerTest::testShutDown(bool posix) { // pid-X to keep it unique so we dont collude with other tests int num = 0; const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg1 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg2 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg3 = segmentPrefix + "-" + std::to_string(num++); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment3 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg3 = segment3.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; + const auto seg3Opt = segment3.second; size_t seg1Size = 0; size_t seg2Size = 0; size_t seg3Size = 0; @@ -635,21 +722,18 @@ void ShmManagerTest::testShutDown(bool posix) { ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - segmentsToDestroy.push_back(seg1); seg1Size = getRandomSize(); - s.createShm(seg1, seg1Size); + s.createShm(seg1, seg1Size, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - segmentsToDestroy.push_back(seg2); seg2Size = getRandomSize(); - s.createShm(seg2, seg2Size); + s.createShm(seg2, seg2Size, nullptr, seg2Opt); auto& shm2 = s.getShmByName(seg2); ASSERT_EQ(shm2.getSize(), seg2Size); - segmentsToDestroy.push_back(seg3); seg3Size = getRandomSize(); - s.createShm(seg3, seg3Size); + s.createShm(seg3, seg3Size, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); @@ -660,15 +744,15 @@ void ShmManagerTest::testShutDown(bool posix) { ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - s.attachShm(seg1); + s.attachShm(seg1, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - s.attachShm(seg2); + s.attachShm(seg2, nullptr, seg2Opt); auto& shm2 = s.getShmByName(seg2); ASSERT_EQ(shm2.getSize(), seg2Size); - s.attachShm(seg3); + s.attachShm(seg3, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); @@ -680,11 +764,11 @@ void ShmManagerTest::testShutDown(bool posix) { ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - s.attachShm(seg1); + s.attachShm(seg1, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - s.attachShm(seg3); + s.attachShm(seg3, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); @@ -697,20 +781,20 @@ void ShmManagerTest::testShutDown(bool posix) { ShmManager s(cacheDir, posix); ASSERT_NO_THROW({ - s.attachShm(seg1); + s.attachShm(seg1, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - s.attachShm(seg3); + s.attachShm(seg3, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); }); - ASSERT_THROW(s.attachShm(seg2), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg2, nullptr, seg2Opt), std::invalid_argument); // create a new one. this is possible only because the previous one was // destroyed. - ASSERT_NO_THROW(s.createShm(seg2, seg2Size)); + ASSERT_NO_THROW(s.createShm(seg2, seg2Size, nullptr, seg2Opt)); ASSERT_EQ(s.getShmByName(seg2).getSize(), seg2Size); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); @@ -726,19 +810,19 @@ void ShmManagerTest::testShutDown(bool posix) { { ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(seg1), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), std::invalid_argument); - ASSERT_THROW(s.attachShm(seg2), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg2, nullptr, seg2Opt), std::invalid_argument); - ASSERT_THROW(s.attachShm(seg3), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg3, nullptr, seg3Opt), std::invalid_argument); - ASSERT_NO_THROW(s.createShm(seg1, seg1Size)); + ASSERT_NO_THROW(s.createShm(seg1, seg1Size, nullptr, seg1Opt)); ASSERT_EQ(s.getShmByName(seg1).getSize(), seg1Size); - ASSERT_NO_THROW(s.createShm(seg2, seg2Size)); + ASSERT_NO_THROW(s.createShm(seg2, seg2Size, nullptr, seg3Opt)); ASSERT_EQ(s.getShmByName(seg2).getSize(), seg2Size); - ASSERT_NO_THROW(s.createShm(seg3, seg3Size)); + ASSERT_NO_THROW(s.createShm(seg3, seg3Size, nullptr, seg3Opt)); ASSERT_EQ(s.getShmByName(seg3).getSize(), seg3Size); // dont call shutdown @@ -757,13 +841,21 @@ TEST_F(ShmManagerTestPosix, ShutDown) { testShutDown(true); } TEST_F(ShmManagerTestSysV, ShutDown) { testShutDown(false); } +TEST_F(ShmManagerTestFile, ShutDown) { testShutDown(false); } + void ShmManagerTest::testCleanup(bool posix) { // pid-X to keep it unique so we dont collude with other tests int num = 0; const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg1 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg2 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg3 = segmentPrefix + "-" + std::to_string(num++); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment3 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + const auto seg3 = segment3.first; + const auto seg1Opt = segment1.second; + const auto seg2Opt = segment2.second; + const auto seg3Opt = segment3.second; size_t seg1Size = 0; size_t seg2Size = 0; size_t seg3Size = 0; @@ -772,21 +864,18 @@ void ShmManagerTest::testCleanup(bool posix) { ASSERT_NO_THROW({ ShmManager s(cacheDir, posix); - segmentsToDestroy.push_back(seg1); seg1Size = getRandomSize(); - s.createShm(seg1, seg1Size); + s.createShm(seg1, seg1Size, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - segmentsToDestroy.push_back(seg2); seg2Size = getRandomSize(); - s.createShm(seg2, seg2Size); + s.createShm(seg2, seg2Size, nullptr, seg3Opt); auto& shm2 = s.getShmByName(seg2); ASSERT_EQ(shm2.getSize(), seg2Size); - segmentsToDestroy.push_back(seg3); seg3Size = getRandomSize(); - s.createShm(seg3, seg3Size); + s.createShm(seg3, seg3Size, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); @@ -803,22 +892,22 @@ void ShmManagerTest::testCleanup(bool posix) { { ShmManager s(cacheDir, posix); - ASSERT_THROW(s.attachShm(seg1), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg1, nullptr, seg1Opt), std::invalid_argument); - ASSERT_THROW(s.attachShm(seg2), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg2, nullptr, seg2Opt), std::invalid_argument); - ASSERT_THROW(s.attachShm(seg3), std::invalid_argument); + ASSERT_THROW(s.attachShm(seg3, nullptr, seg3Opt), std::invalid_argument); ASSERT_NO_THROW({ - s.createShm(seg1, seg1Size); + s.createShm(seg1, seg1Size, nullptr, seg1Opt); auto& shm1 = s.getShmByName(seg1); ASSERT_EQ(shm1.getSize(), seg1Size); - s.createShm(seg2, seg2Size); + s.createShm(seg2, seg2Size, nullptr, seg2Opt); auto& shm2 = s.getShmByName(seg2); ASSERT_EQ(shm2.getSize(), seg2Size); - s.createShm(seg3, seg3Size); + s.createShm(seg3, seg3Size, nullptr, seg3Opt); auto& shm3 = s.getShmByName(seg3); ASSERT_EQ(shm3.getSize(), seg3Size); }); @@ -830,31 +919,34 @@ TEST_F(ShmManagerTestPosix, Cleanup) { testCleanup(true); } TEST_F(ShmManagerTestSysV, Cleanup) { testCleanup(false); } +TEST_F(ShmManagerTestFile, Cleanup) { testCleanup(false); } + void ShmManagerTest::testAttachReadOnly(bool posix) { // pid-X to keep it unique so we dont collude with other tests int num = 0; const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg = segmentPrefix + "-" + std::to_string(num++); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg = segment1.first; + const auto segOpt = segment1.second; size_t segSize = 0; // open an instance and create segment ShmManager s(cacheDir, posix); - segmentsToDestroy.push_back(seg); segSize = getRandomSize(); - s.createShm(seg, segSize); + s.createShm(seg, segSize, nullptr, segOpt); auto& shm = s.getShmByName(seg); ASSERT_EQ(shm.getSize(), segSize); const unsigned char magicVal = 'd'; writeToMemory(shm.getCurrentMapping().addr, segSize, magicVal); - auto roShm = ShmManager::attachShmReadOnly(cacheDir, seg, posix); + auto roShm = ShmManager::attachShmReadOnly(cacheDir, seg, segOpt.typeOpts); ASSERT_NE(roShm.get(), nullptr); ASSERT_TRUE(roShm->isMapped()); checkMemory(roShm->getCurrentMapping().addr, segSize, magicVal); auto addr = getNewUnmappedAddr(); - roShm = ShmManager::attachShmReadOnly(cacheDir, seg, posix, addr); + roShm = ShmManager::attachShmReadOnly(cacheDir, seg, segOpt.typeOpts, addr); ASSERT_NE(roShm.get(), nullptr); ASSERT_TRUE(roShm->isMapped()); ASSERT_EQ(roShm->getCurrentMapping().addr, addr); @@ -865,6 +957,8 @@ TEST_F(ShmManagerTestPosix, AttachReadOnly) { testAttachReadOnly(true); } TEST_F(ShmManagerTestSysV, AttachReadOnly) { testAttachReadOnly(false); } +TEST_F(ShmManagerTestFile, AttachReadOnly) { testAttachReadOnly(false); } + // test to ensure that segments can be created with a new cache dir, attached // from existing cache dir, segments can be deleted and recreated using the // same cache dir if they have not been attached to already. @@ -872,30 +966,32 @@ void ShmManagerTest::testMappingAlignment(bool posix) { // pid-X to keep it unique so we dont collude with other tests int num = 0; const std::string segmentPrefix = std::to_string(::getpid()); - const std::string seg1 = segmentPrefix + "-" + std::to_string(num++); - const std::string seg2 = segmentPrefix + "-" + std::to_string(num++); + auto segment1 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + auto segment2 = makeSegment(segmentPrefix + "-" + std::to_string(num++)); + const auto seg1 = segment1.first; + const auto seg2 = segment2.first; + auto seg1Opt = segment1.second; + auto seg2Opt = segment2.second; const char magicVal1 = 'f'; const char magicVal2 = 'n'; { ShmManager s(cacheDir, posix); - facebook::cachelib::ShmSegmentOpts opts; - opts.alignment = 1ULL << folly::Random::rand32(0, 18); - segmentsToDestroy.push_back(seg1); - auto m1 = s.createShm(seg1, getRandomSize(), nullptr, opts); - ASSERT_EQ(reinterpret_cast(m1.addr) & (opts.alignment - 1), 0); + seg1Opt.alignment = 1ULL << folly::Random::rand32(0, 18); + auto m1 = s.createShm(seg1, getRandomSize(), nullptr, seg1Opt); + ASSERT_EQ(reinterpret_cast(m1.addr) & (seg1Opt.alignment - 1), 0); writeToMemory(m1.addr, m1.size, magicVal1); checkMemory(m1.addr, m1.size, magicVal1); // invalid alignment should throw - opts.alignment = folly::Random::rand32(1 << 23, 1 << 24); - ASSERT_THROW(s.createShm(seg2, getRandomSize(), nullptr, opts), + seg2Opt.alignment = folly::Random::rand32(1 << 23, 1 << 24); + ASSERT_THROW(s.createShm(seg2, getRandomSize(), nullptr, seg2Opt), std::invalid_argument); ASSERT_THROW(s.getShmByName(seg2), std::invalid_argument); auto addr = getNewUnmappedAddr(); // alignment option is ignored when using explicit address - opts.alignment = folly::Random::rand32(1 << 23, 1 << 24); - auto m2 = s.createShm(seg2, getRandomSize(), addr, opts); + seg2Opt.alignment = folly::Random::rand32(1 << 23, 1 << 24); + auto m2 = s.createShm(seg2, getRandomSize(), addr, seg2Opt); ASSERT_EQ(m2.addr, addr); writeToMemory(m2.addr, m2.size, magicVal2); checkMemory(m2.addr, m2.size, magicVal2); @@ -908,16 +1004,16 @@ void ShmManagerTest::testMappingAlignment(bool posix) { // can choose a different alignemnt facebook::cachelib::ShmSegmentOpts opts; - opts.alignment = 1ULL << folly::Random::rand32(18, 22); + seg1Opt.alignment = 1ULL << folly::Random::rand32(18, 22); // attach - auto m1 = s.attachShm(seg1, nullptr, opts); - ASSERT_EQ(reinterpret_cast(m1.addr) & (opts.alignment - 1), 0); + auto m1 = s.attachShm(seg1, nullptr, seg1Opt); + ASSERT_EQ(reinterpret_cast(m1.addr) & (seg1Opt.alignment - 1), 0); checkMemory(m1.addr, m1.size, magicVal1); // alignment can be enabled on previously explicitly mapped segments - opts.alignment = 1ULL << folly::Random::rand32(1, 22); - auto m2 = s.attachShm(seg2, nullptr, opts); - ASSERT_EQ(reinterpret_cast(m2.addr) & (opts.alignment - 1), 0); + seg2Opt.alignment = 1ULL << folly::Random::rand32(1, 22); + auto m2 = s.attachShm(seg2, nullptr, seg2Opt); + ASSERT_EQ(reinterpret_cast(m2.addr) & (seg2Opt.alignment - 1), 0); checkMemory(m2.addr, m2.size, magicVal2); }; } @@ -928,3 +1024,7 @@ TEST_F(ShmManagerTestPosix, TestMappingAlignment) { TEST_F(ShmManagerTestSysV, TestMappingAlignment) { testMappingAlignment(false); } + +TEST_F(ShmManagerTestFile, TestMappingAlignment) { + testMappingAlignment(false); +} From 14d974e662426ce7813e75dad965793b4e37eb29 Mon Sep 17 00:00:00 2001 From: Sounak Gupta Date: Wed, 27 Oct 2021 10:40:42 -0700 Subject: [PATCH 04/13] Add support for shm opts serialization After introducing file segment type, nameToKey_ does not provide enough information to recover/remove segments on restart. This commit fixes that by replacing nameToKey_ with nameToOpts_. Previously, the Key from nameToKey_ map was only used in a single DCHECK(). --- cachelib/allocator/CacheAllocator-inl.h | 2 +- cachelib/shm/PosixShmSegment.h | 6 +- cachelib/shm/ShmManager.cpp | 115 ++++++++++++++++-------- cachelib/shm/ShmManager.h | 13 ++- cachelib/shm/SysVShmSegment.h | 3 +- cachelib/shm/shm.thrift | 7 +- cachelib/shm/tests/test_shm_manager.cpp | 3 + 7 files changed, 106 insertions(+), 43 deletions(-) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index a6b2ee0b94..ce9dcc3c52 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -3398,7 +3398,7 @@ bool CacheAllocator::stopReaper(std::chrono::seconds timeout) { template bool CacheAllocator::cleanupStrayShmSegments( - const std::string& cacheDir, bool posix /*TODO(SHM_FILE): const std::vector& config */) { + const std::string& cacheDir, bool posix /*TODO(SHM_FILE): const std::vector& config */) { if (util::getStatIfExists(cacheDir, nullptr) && util::isDir(cacheDir)) { try { // cache dir exists. clean up only if there are no other processes diff --git a/cachelib/shm/PosixShmSegment.h b/cachelib/shm/PosixShmSegment.h index da5050a290..6aaeb004e7 100644 --- a/cachelib/shm/PosixShmSegment.h +++ b/cachelib/shm/PosixShmSegment.h @@ -92,13 +92,13 @@ class PosixShmSegment : public ShmBase { // @return true if the segment existed. false otherwise static bool removeByName(const std::string& name); + // returns the key type corresponding to the given name. + static std::string createKeyForName(const std::string& name) noexcept; + private: static int createNewSegment(const std::string& name); static int getExisting(const std::string& name, const ShmSegmentOpts& opts); - // returns the key type corresponding to the given name. - static std::string createKeyForName(const std::string& name) noexcept; - // resize the segment // @param size the new size // @return none diff --git a/cachelib/shm/ShmManager.cpp b/cachelib/shm/ShmManager.cpp index dacdda0670..427062951a 100644 --- a/cachelib/shm/ShmManager.cpp +++ b/cachelib/shm/ShmManager.cpp @@ -22,6 +22,7 @@ #include #include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wconversion" @@ -98,7 +99,7 @@ ShmManager::ShmManager(const std::string& dir, bool usePosix) // if file exists, init from it if needed. const bool reattach = dropSegments ? false : initFromFile(); if (!reattach) { - DCHECK(nameToKey_.empty()); + DCHECK(nameToOpts_.empty()); } // Lock file for exclusive access lockMetadataFile(metaFile); @@ -109,7 +110,7 @@ ShmManager::ShmManager(const std::string& dir, bool usePosix) } bool ShmManager::initFromFile() { - // restore the nameToKey_ map and destroy the contents of the file. + // restore the nameToOpts_ map and destroy the contents of the file. const std::string fileName = pathName(controlDir_, kMetaDataFile); std::ifstream f(fileName); SCOPE_EXIT { f.close(); }; @@ -139,9 +140,16 @@ bool ShmManager::initFromFile() { } for (const auto& kv : *object.nameToKeyMap_ref()) { - nameToKey_.insert({kv.first, kv.second}); + if (kv.second.path == "") { + PosixSysVSegmentOpts type; + type.usePosix = kv.second.usePosix; + nameToOpts_.insert({kv.first, type}); + } else { + FileShmSegmentOpts type; + type.path = kv.second.path; + nameToOpts_.insert({kv.first, type}); + } } - return true; } @@ -157,7 +165,7 @@ typename ShmManager::ShutDownRes ShmManager::writeActiveSegmentsToFile() { return ShutDownRes::kFileDeleted; } - // write the shmtype, nameToKey_ map to the file. + // write the shmtype, nameToOpts_ map to the file. DCHECK(metadataStream_); serialization::ShmManagerObject object; @@ -165,9 +173,20 @@ typename ShmManager::ShutDownRes ShmManager::writeActiveSegmentsToFile() { object.shmVal_ref() = usePosix_ ? static_cast(ShmVal::SHM_POSIX) : static_cast(ShmVal::SHM_SYS_V); - for (const auto& kv : nameToKey_) { + for (const auto& kv : nameToOpts_) { const auto& name = kv.first; - const auto& key = kv.second; + serialization::ShmTypeObject key; + if (const auto* opts = std::get_if(&kv.second)) { + key.path = opts->path; + } else { + try { + const auto& v = std::get(kv.second); + key.usePosix = v.usePosix; + key.path = ""; + } catch(std::bad_variant_access&) { + throw std::invalid_argument(folly::sformat("Not a valid segment")); + } + } const auto it = segments_.find(name); // segment exists and is active. if (it != segments_.end() && it->second->isActive()) { @@ -199,14 +218,14 @@ typename ShmManager::ShutDownRes ShmManager::shutDown() { // clear our data. segments_.clear(); - nameToKey_.clear(); + nameToOpts_.clear(); return ret; } namespace { bool removeSegByName(ShmTypeOpts typeOpts, const std::string& uniqueName) { - if (auto *v = std::get_if(&typeOpts)) { + if (const auto* v = std::get_if(&typeOpts)) { return FileShmSegment::removeByPath(v->path); } @@ -258,22 +277,20 @@ void ShmManager::cleanup(const std::string& dir, bool posix) { } void ShmManager::removeAllSegments() { - // TODO(SHM_FILE): extend this once we have opts stored in nameToKey_ - for (const auto& kv : nameToKey_) { - removeSegByName(usePosix_, uniqueIdForName(kv.first)); + for (const auto& kv : nameToOpts_) { + removeSegByName(kv.second, uniqueIdForName(kv.first)); } - nameToKey_.clear(); + nameToOpts_.clear(); } void ShmManager::removeUnAttachedSegments() { - // TODO(SHM_FILE): extend this once we have opts stored in nameToKey_ - auto it = nameToKey_.begin(); - while (it != nameToKey_.end()) { + auto it = nameToOpts_.begin(); + while (it != nameToOpts_.end()) { const auto name = it->first; // check if the segment is attached. if (segments_.find(name) == segments_.end()) { // not attached - removeSegByName(usePosix_, uniqueIdForName(name)); - it = nameToKey_.erase(it); + removeSegByName(it->second, uniqueIdForName(name)); + it = nameToOpts_.erase(it); } else { ++it; } @@ -292,13 +309,13 @@ ShmAddr ShmManager::createShm(const std::string& shmName, removeShm(shmName, opts.typeOpts); DCHECK(segments_.find(shmName) == segments_.end()); - DCHECK(nameToKey_.find(shmName) == nameToKey_.end()); + DCHECK(nameToOpts_.find(shmName) == nameToOpts_.end()); - if (auto *v = std::get_if(&opts.typeOpts)) { - if (usePosix_ != v->usePosix) - throw std::invalid_argument( - folly::sformat("Expected {} but got {} segment", - usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); + const auto* v = std::get_if(&opts.typeOpts); + if (v && usePosix_ != v->usePosix) { + throw std::invalid_argument( + folly::sformat("Expected {} but got {} segment", + usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); } std::unique_ptr newSeg; @@ -326,24 +343,32 @@ ShmAddr ShmManager::createShm(const std::string& shmName, } auto ret = newSeg->getCurrentMapping(); - nameToKey_.emplace(shmName, newSeg->getKeyStr()); + if (v) { + PosixSysVSegmentOpts opts; + opts.usePosix = v->usePosix; + nameToOpts_.emplace(shmName, opts); + } else { + FileShmSegmentOpts opts; + opts.path = newSeg->getKeyStr(); + nameToOpts_.emplace(shmName, opts); + } segments_.emplace(shmName, std::move(newSeg)); return ret; } void ShmManager::attachNewShm(const std::string& shmName, ShmSegmentOpts opts) { - const auto keyIt = nameToKey_.find(shmName); + const auto keyIt = nameToOpts_.find(shmName); // if key is not known already, there is not much we can do to attach. - if (keyIt == nameToKey_.end()) { + if (keyIt == nameToOpts_.end()) { throw std::invalid_argument( folly::sformat("Unable to find any segment with name {}", shmName)); } - if (auto *v = std::get_if(&opts.typeOpts)) { - if (usePosix_ != v->usePosix) - throw std::invalid_argument( - folly::sformat("Expected {} but got {} segment", - usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); + const auto* v = std::get_if(&opts.typeOpts); + if (v && usePosix_ != v->usePosix) { + throw std::invalid_argument( + folly::sformat("Expected {} but got {} segment", + usePosix_ ? "posix" : "SysV", usePosix_ ? "SysV" : "posix")); } // This means the segment exists and we can try to attach it. @@ -360,7 +385,17 @@ void ShmManager::attachNewShm(const std::string& shmName, ShmSegmentOpts opts) { shmName, e.what())); } DCHECK(segments_.find(shmName) != segments_.end()); - DCHECK_EQ(segments_[shmName]->getKeyStr(), keyIt->second); + if (v) { // If it is a posix shm segment + // Comparison unnecessary since getKeyStr() retuns name_from ShmBase + // createKeyForShm also returns the same variable. + } else { // Else it is a file segment + try { + auto opts = std::get(keyIt->second); + DCHECK_EQ(segments_[shmName]->getKeyStr(), opts.path); + } catch(std::bad_variant_access&) { + throw std::invalid_argument(folly::sformat("Not a valid segment")); + } + } } ShmAddr ShmManager::attachShm(const std::string& shmName, @@ -403,13 +438,13 @@ bool ShmManager::removeShm(const std::string& shmName, ShmTypeOpts typeOpts) { removeSegByName(typeOpts, uniqueIdForName(shmName)); if (!wasPresent) { DCHECK(segments_.end() == segments_.find(shmName)); - DCHECK(nameToKey_.end() == nameToKey_.find(shmName)); + DCHECK(nameToOpts_.end() == nameToOpts_.find(shmName)); return false; } } // not mapped and already removed. segments_.erase(shmName); - nameToKey_.erase(shmName); + nameToOpts_.erase(shmName); return true; } @@ -424,5 +459,15 @@ ShmSegment& ShmManager::getShmByName(const std::string& shmName) { } } +ShmTypeOpts& ShmManager::getShmTypeByName(const std::string& shmName) { + const auto it = nameToOpts_.find(shmName); + if (it != nameToOpts_.end()) { + return it->second; + } else { + throw std::invalid_argument(folly::sformat( + "shared memory segment does not exist: name: {}", shmName)); + } +} + } // namespace cachelib } // namespace facebook diff --git a/cachelib/shm/ShmManager.h b/cachelib/shm/ShmManager.h index 21ad173b3d..2eebbfbf99 100644 --- a/cachelib/shm/ShmManager.h +++ b/cachelib/shm/ShmManager.h @@ -109,6 +109,14 @@ class ShmManager { // it is returned. Otherwise, it throws std::invalid_argument ShmSegment& getShmByName(const std::string& shmName); + // gets a current segment type by the name that is managed by this + // instance. The lifetime of the returned object is same as the + // lifetime of this instance. + // @param name Name of the segment + // @return If a segment of that name, managed by this instance exists, + // it is returned. Otherwise, it throws std::invalid_argument + ShmTypeOpts& getShmTypeByName(const std::string& shmName); + enum class ShutDownRes { kSuccess = 0, kFileDeleted, kFailedWrite }; // persists the metadata information for the current segments managed @@ -223,8 +231,9 @@ class ShmManager { std::unordered_map> segments_{}; // name to key mapping used for reattaching. This is persisted to a - // file and used for attaching to the segment. - std::unordered_map nameToKey_{}; + // file using serialization::ShmSegmentVariant and used for attaching + // to the segment. + std::unordered_map nameToOpts_{}; // file handle for the metadata file. It remains open throughout the lifetime // of the object. diff --git a/cachelib/shm/SysVShmSegment.h b/cachelib/shm/SysVShmSegment.h index bd24f68aaf..fcebe03eb1 100644 --- a/cachelib/shm/SysVShmSegment.h +++ b/cachelib/shm/SysVShmSegment.h @@ -88,10 +88,11 @@ class SysVShmSegment : public ShmBase { // @return true if the segment existed. false otherwise static bool removeByName(const std::string& name); - private: // returns the key identifier for the given name. static KeyType createKeyForName(const std::string& name) noexcept; +private: + static int createNewSegment(key_t key, size_t size, const ShmSegmentOpts& opts); diff --git a/cachelib/shm/shm.thrift b/cachelib/shm/shm.thrift index 4129d1caa3..81dafbdc79 100644 --- a/cachelib/shm/shm.thrift +++ b/cachelib/shm/shm.thrift @@ -16,7 +16,12 @@ namespace cpp2 facebook.cachelib.serialization +struct ShmTypeObject { + 1: required string path, + 2: required bool usePosix, +} + struct ShmManagerObject { 1: required byte shmVal, - 3: required map nameToKeyMap, + 3: required map nameToKeyMap, } diff --git a/cachelib/shm/tests/test_shm_manager.cpp b/cachelib/shm/tests/test_shm_manager.cpp index 26f8686975..014e93d04d 100644 --- a/cachelib/shm/tests/test_shm_manager.cpp +++ b/cachelib/shm/tests/test_shm_manager.cpp @@ -796,6 +796,9 @@ void ShmManagerTest::testShutDown(bool posix) { // destroyed. ASSERT_NO_THROW(s.createShm(seg2, seg2Size, nullptr, seg2Opt)); ASSERT_EQ(s.getShmByName(seg2).getSize(), seg2Size); + auto *v = std::get_if(&s.getShmTypeByName(seg2)); + ASSERT_TRUE(v); + ASSERT_EQ(v->usePosix, posix); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); }; From ba089ef40fa2428559b7c0330296c16daa9fd98e Mon Sep 17 00:00:00 2001 From: victoria-mcgrath Date: Thu, 28 Oct 2021 08:48:05 -0700 Subject: [PATCH 05/13] Initial version of config API extension to support multiple memory tiers * New class MemoryTierCacheConfig allows to configure a memory tier. Setting tier size and location of a file for file-backed memory are supported in this initial implementation; * New member, vector of memory tiers, is added to class CacheAllocatorConfig. * New test suite, chelib/allocator/tests/MemoryTiersTest.cpp, demonstrates the usage of and tests extended config API. --- cachelib/allocator/CMakeLists.txt | 1 + cachelib/allocator/CacheAllocatorConfig.h | 90 +++++++++- cachelib/allocator/MemoryTierCacheConfig.h | 79 ++++++++ cachelib/allocator/tests/MemoryTiersTest.cpp | 180 +++++++++++++++++++ 4 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 cachelib/allocator/MemoryTierCacheConfig.h create mode 100644 cachelib/allocator/tests/MemoryTiersTest.cpp diff --git a/cachelib/allocator/CMakeLists.txt b/cachelib/allocator/CMakeLists.txt index fc5d8610d8..688bf09134 100644 --- a/cachelib/allocator/CMakeLists.txt +++ b/cachelib/allocator/CMakeLists.txt @@ -109,6 +109,7 @@ if (BUILD_TESTS) add_test (tests/ChainedHashTest.cpp) add_test (tests/AllocatorResizeTypeTest.cpp) add_test (tests/AllocatorHitStatsTypeTest.cpp) + add_test (tests/MemoryTiersTest.cpp) add_test (tests/MultiAllocatorTest.cpp) add_test (tests/NvmAdmissionPolicyTest.cpp) add_test (nvmcache/tests/NvmItemTests.cpp) diff --git a/cachelib/allocator/CacheAllocatorConfig.h b/cachelib/allocator/CacheAllocatorConfig.h index 1207036a95..1bd3c75056 100644 --- a/cachelib/allocator/CacheAllocatorConfig.h +++ b/cachelib/allocator/CacheAllocatorConfig.h @@ -25,6 +25,7 @@ #include #include "cachelib/allocator/Cache.h" +#include "cachelib/allocator/MemoryTierCacheConfig.h" #include "cachelib/allocator/MM2Q.h" #include "cachelib/allocator/MemoryMonitor.h" #include "cachelib/allocator/NvmAdmissionPolicy.h" @@ -49,6 +50,7 @@ class CacheAllocatorConfig { using NvmCacheDeviceEncryptor = typename CacheT::NvmCacheT::DeviceEncryptor; using MoveCb = typename CacheT::MoveCb; using NvmCacheConfig = typename CacheT::NvmCacheT::Config; + using MemoryTierConfigs = std::vector; using Key = typename CacheT::Key; using EventTrackerSharedPtr = std::shared_ptr; using Item = typename CacheT::Item; @@ -186,14 +188,23 @@ class CacheAllocatorConfig { // This allows cache to be persisted across restarts. One example use case is // to preserve the cache when releasing a new version of your service. Refer // to our user guide for how to set up cache persistence. + // TODO: get rid of baseAddr or if set make sure all mapping are adjacent? + // We can also make baseAddr a per-tier configuration CacheAllocatorConfig& enableCachePersistence(std::string directory, void* baseAddr = nullptr); - // uses posix shm segments instead of the default sys-v shm segments. - // @throw std::invalid_argument if called without enabling - // cachePersistence() + // Uses posix shm segments instead of the default sys-v shm + // segments. @throw std::invalid_argument if called without enabling + // cachePersistence(). CacheAllocatorConfig& usePosixForShm(); + // Configures cache memory tiers. Accepts vector of MemoryTierCacheConfig. + // Each vector element describes configuration for a single memory cache tier. + CacheAllocatorConfig& configureMemoryTiers(const MemoryTierConfigs& configs); + + // Return reference to MemoryTierCacheConfigs. + const MemoryTierConfigs& getMemoryTierConfigs(); + // This turns on a background worker that periodically scans through the // access container and look for expired items and remove them. CacheAllocatorConfig& enableItemReaperInBackground( @@ -541,6 +552,9 @@ class CacheAllocatorConfig { // cache. uint64_t nvmAdmissionMinTTL{0}; + // Configuration for memory tiers. + MemoryTierConfigs memoryTierConfigs; + friend CacheT; private: @@ -801,6 +815,74 @@ CacheAllocatorConfig& CacheAllocatorConfig::enableItemReaperInBackground( return *this; } +template +CacheAllocatorConfig& CacheAllocatorConfig::configureMemoryTiers( + const MemoryTierConfigs& config) { + memoryTierConfigs = config; + size_t sum_ratios = 0; + size_t sum_sizes = 0; + + for (auto tier_config: memoryTierConfigs) { + auto tier_size = tier_config.getSize(); + auto tier_ratio = tier_config.getRatio(); + if ((!tier_size and !tier_ratio) || (tier_size and tier_ratio)) { + throw std::invalid_argument( + "For each memory tier either size or ratio must be set."); + } + sum_ratios += tier_ratio; + sum_sizes += tier_size; + } + + if (sum_ratios) { + if (!getCacheSize()) { + throw std::invalid_argument( + "Total cache size must be specified when size ratios are \ + used to specify memory tier sizes."); + } else { + if (getCacheSize() < sum_ratios) { + throw std::invalid_argument( + "Sum of all tier size ratios is greater than total cache size."); + } + // Convert ratios to sizes + sum_sizes = 0; + size_t partition_size = getCacheSize() / sum_ratios; + for (auto& tier_config: memoryTierConfigs) { + tier_config.setSize(partition_size * tier_config.getRatio()); + sum_sizes += tier_config.getSize(); + } + if (getCacheSize() != sum_sizes) { + // Adjust capacity of the last tier to account for rounding error + memoryTierConfigs.back().setSize(memoryTierConfigs.back().getSize() + \ + (getCacheSize() - sum_sizes)); + sum_sizes = getCacheSize(); + } + } + } else if (sum_sizes) { + if (getCacheSize() && sum_sizes != getCacheSize()) { + throw std::invalid_argument( + "Sum of tier sizes doesn't match total cache size. \ + Setting of cache total size is not required when per-tier \ + sizes are specified - it is calculated as sum of tier sizes."); + } + } else { + throw std::invalid_argument( + "Either sum of all memory tiers sizes or sum of all ratios \ + must be greater than 0."); + } + + if (sum_sizes && !getCacheSize()) { + setCacheSize(sum_sizes); + } + + return *this; +} + +//const std::vector& CacheAllocatorConfig::getMemoryTierConfigs() { +template +const typename CacheAllocatorConfig::MemoryTierConfigs& CacheAllocatorConfig::getMemoryTierConfigs() { + return memoryTierConfigs; +} + template CacheAllocatorConfig& CacheAllocatorConfig::disableCacheEviction() { disableEviction = true; @@ -970,7 +1052,7 @@ std::map CacheAllocatorConfig::serialize() const { configMap["size"] = std::to_string(size); configMap["cacheDir"] = cacheDir; - configMap["posixShm"] = usePosixShm ? "set" : "empty"; + configMap["posixShm"] = isUsingPosixShm() ? "set" : "empty"; configMap["defaultAllocSizes"] = ""; // Stringify std::set diff --git a/cachelib/allocator/MemoryTierCacheConfig.h b/cachelib/allocator/MemoryTierCacheConfig.h new file mode 100644 index 0000000000..5e3604a0af --- /dev/null +++ b/cachelib/allocator/MemoryTierCacheConfig.h @@ -0,0 +1,79 @@ +/* + * Copyright (c) Intel Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +namespace facebook { +namespace cachelib { +class MemoryTierCacheConfig { +public: + // Creates instance of MemoryTierCacheConfig for file-backed memory. + // @param path to file which CacheLib will use to map memory from. + // TODO: add fromDirectory, fromAnonymousMemory + static MemoryTierCacheConfig fromFile(const std::string& _file) { + MemoryTierCacheConfig config; + config.path = _file; + return config; + } + + // Specifies size of this memory tier. Sizes of tiers must be specified by + // either setting size explicitly or using ratio, mixing of the two is not supported. + MemoryTierCacheConfig& setSize(size_t _size) { + size = _size; + return *this; + } + + // Specifies ratio of this memory tier to other tiers. Absolute size + // of each tier can be calculated as: + // cacheSize * tierRatio / Sum of ratios for all tiers; the difference + // between total cache size and sum of all tier sizes resulted from + // round off error is accounted for when calculating the last tier's + // size to make the totals equal. + MemoryTierCacheConfig& setRatio(double _ratio) { + ratio = _ratio; + return *this; + } + + size_t getRatio() const noexcept { return ratio; } + + size_t getSize() const noexcept { return size; } + + const std::string& getPath() const noexcept { return path; } + + bool isFileBacked() const { + return !path.empty(); + } + + // Size of this memory tiers + size_t size{0}; + + // Ratio is a number of parts of the total cache size to be allocated for this tier. + // E.g. if X is a total cache size, Yi are ratios specified for memory tiers, + // then size of the i-th tier Xi = (X / (Y1 + Y2)) * Yi and X = sum(Xi) + size_t ratio{0}; + + // Path to file for file system-backed memory tier + // TODO: consider using variant to support different + // memory sources + std::string path; + +private: + MemoryTierCacheConfig() = default; +}; +} // namespace cachelib +} // namespace facebook diff --git a/cachelib/allocator/tests/MemoryTiersTest.cpp b/cachelib/allocator/tests/MemoryTiersTest.cpp new file mode 100644 index 0000000000..f578ed3ea3 --- /dev/null +++ b/cachelib/allocator/tests/MemoryTiersTest.cpp @@ -0,0 +1,180 @@ +/* + * Copyright (c) Intel Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include "cachelib/allocator/CacheAllocator.h" +#include "cachelib/allocator/tests/TestBase.h" + +namespace facebook { +namespace cachelib { +namespace tests { + + +using LruAllocatorConfig = CacheAllocatorConfig; +using LruMemoryTierConfigs = LruAllocatorConfig::MemoryTierConfigs; +using Strings = std::vector; +using SizePair = std::tuple; +using SizePairs = std::vector; + +const size_t defaultTotalCacheSize{1 * 1024 * 1024 * 1024}; +const std::string defaultCacheDir{"/var/metadataDir"}; +const std::string defaultPmemPath{"/dev/shm/p1"}; +const std::string defaultDaxPath{"/dev/dax0.0"}; + +template +class MemoryTiersTest: public AllocatorTest { + public: + void basicCheck( + LruAllocatorConfig& actualConfig, + const Strings& expectedPaths = {defaultPmemPath}, + size_t expectedTotalCacheSize = defaultTotalCacheSize, + const std::string& expectedCacheDir = defaultCacheDir) { + EXPECT_EQ(actualConfig.getCacheSize(), expectedTotalCacheSize); + EXPECT_EQ(actualConfig.getMemoryTierConfigs().size(), expectedPaths.size()); + EXPECT_EQ(actualConfig.getCacheDir(), expectedCacheDir); + auto configs = actualConfig.getMemoryTierConfigs(); + + size_t sum_sizes = std::accumulate(configs.begin(), configs.end(), 0, + [](const size_t i, const MemoryTierCacheConfig& config) { return i + config.getSize();}); + size_t sum_ratios = std::accumulate(configs.begin(), configs.end(), 0, + [](const size_t i, const MemoryTierCacheConfig& config) { return i + config.getRatio();}); + + EXPECT_EQ(sum_sizes, expectedTotalCacheSize); + size_t partition_size = 0, remaining_capacity = actualConfig.getCacheSize(); + if (sum_ratios) { + partition_size = actualConfig.getCacheSize() / sum_ratios; + } + + for(auto i = 0; i < configs.size(); ++i) { + EXPECT_EQ(configs[i].getPath(), expectedPaths[i]); + EXPECT_GT(configs[i].getSize(), 0); + if (configs[i].getRatio() && (i < configs.size() - 1)) { + EXPECT_EQ(configs[i].getSize(), partition_size * configs[i].getRatio()); + } + remaining_capacity -= configs[i].getSize(); + } + + EXPECT_EQ(remaining_capacity, 0); + } + + LruAllocatorConfig createTestCacheConfig( + const Strings& tierPaths = {defaultPmemPath}, + const SizePairs& sizePairs = {std::make_tuple(1 /* ratio */, 0 /* size */)}, + bool setPosixForShm = true, + size_t cacheSize = defaultTotalCacheSize, + const std::string& cacheDir = defaultCacheDir) { + LruAllocatorConfig cfg; + cfg.setCacheSize(cacheSize) + .enableCachePersistence(cacheDir); + + if (setPosixForShm) + cfg.usePosixForShm(); + + LruMemoryTierConfigs tierConfigs; + tierConfigs.reserve(tierPaths.size()); + for(auto i = 0; i < tierPaths.size(); ++i) { + tierConfigs.push_back(MemoryTierCacheConfig::fromFile(tierPaths[i]) + .setRatio(std::get<0>(sizePairs[i])) + .setSize(std::get<1>(sizePairs[i]))); + } + cfg.configureMemoryTiers(tierConfigs); + return cfg; + } +}; + +using LruMemoryTiersTest = MemoryTiersTest; + +TEST_F(LruMemoryTiersTest, TestValid1TierPmemRatioConfig) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultPmemPath}).validate(); + basicCheck(cfg); +} + +TEST_F(LruMemoryTiersTest, TestValid1TierDaxRatioConfig) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath}).validate(); + basicCheck(cfg, {defaultDaxPath}); +} + +TEST_F(LruMemoryTiersTest, TestValid1TierDaxSizeConfig) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath}, + {std::make_tuple(0, defaultTotalCacheSize)}, + /* setPosixShm */ true, + /* cacheSize */ 0).validate(); + basicCheck(cfg, {defaultDaxPath}); +} + +TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemConfig) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 0), std::make_tuple(1, 0)}).validate(); + basicCheck(cfg, {defaultDaxPath, defaultPmemPath}); +} + +TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemRatioConfig) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(5, 0), std::make_tuple(2, 0)}).validate(); + basicCheck(cfg, {defaultDaxPath, defaultPmemPath}); +} + +TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemSizeConfig) { + size_t size_1 = 4321, size_2 = 1234; + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(0, size_1), std::make_tuple(0, size_2)}, + true, 0).validate(); + basicCheck(cfg, {defaultDaxPath, defaultPmemPath}, size_1 + size_2); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigPosixShmNotSet) { + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 0), std::make_tuple(1, 0)}, + /* setPosixShm */ false).validate(); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigNumberOfPartitionsTooLarge) { + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}), + std::invalid_argument); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigSizesAndRatiosMixed) { + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 0), std::make_tuple(1, 1)}), + std::invalid_argument); + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 1), std::make_tuple(0, 1)}), + std::invalid_argument); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigSizesAndRatioNotSet) { + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 0), std::make_tuple(0, 0)}), + std::invalid_argument); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigRatiosCacheSizeNotSet) { + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(1, 0), std::make_tuple(1, 0)}, + /* setPosixShm */ true, /* cacheSize */ 0), + std::invalid_argument); +} + +TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigSizesNeCacheSize) { + EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, + {std::make_tuple(0, 1), std::make_tuple(0, 1)}), + std::invalid_argument); +} + +} // namespace tests +} // namespace cachelib +} // namespace facebook From ee63ef37b9227b3affb8ee2d8d4f6247714e3be1 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Fri, 29 Oct 2021 20:23:46 -0400 Subject: [PATCH 06/13] Integrate Memory Tier config API with CacheAllocator. --- cachelib/allocator/CMakeLists.txt | 1 + cachelib/allocator/CacheAllocator-inl.h | 66 +++++++++++++------ cachelib/allocator/CacheAllocator.h | 4 ++ cachelib/allocator/CacheAllocatorConfig.h | 1 - .../tests/AllocatorMemoryTiersTest.cpp | 29 ++++++++ .../tests/AllocatorMemoryTiersTest.h | 47 +++++++++++++ .../allocator/tests/AllocatorTypeTest.cpp | 7 ++ cachelib/allocator/tests/BaseAllocatorTest.h | 4 +- cachelib/shm/ShmCommon.h | 3 +- 9 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp create mode 100644 cachelib/allocator/tests/AllocatorMemoryTiersTest.h diff --git a/cachelib/allocator/CMakeLists.txt b/cachelib/allocator/CMakeLists.txt index 688bf09134..367a02caa3 100644 --- a/cachelib/allocator/CMakeLists.txt +++ b/cachelib/allocator/CMakeLists.txt @@ -109,6 +109,7 @@ if (BUILD_TESTS) add_test (tests/ChainedHashTest.cpp) add_test (tests/AllocatorResizeTypeTest.cpp) add_test (tests/AllocatorHitStatsTypeTest.cpp) + add_test (tests/AllocatorMemoryTiersTest.cpp) add_test (tests/MemoryTiersTest.cpp) add_test (tests/MultiAllocatorTest.cpp) add_test (tests/NvmAdmissionPolicyTest.cpp) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index ce9dcc3c52..d285de2fef 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -24,7 +24,8 @@ namespace cachelib { template CacheAllocator::CacheAllocator(Config config) - : isOnShm_{config.memMonitoringEnabled()}, + : memoryTierConfigs(config.getMemoryTierConfigs()), + isOnShm_{config.memMonitoringEnabled()}, config_(config.validate()), tempShm_(isOnShm_ ? std::make_unique(config_.size) : nullptr), @@ -49,15 +50,21 @@ CacheAllocator::CacheAllocator(Config config) cacheCreationTime_{util::getCurrentTimeSec()}, nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(), config_.isNvmCacheTruncateAllocSizeEnabled()} { + // TODO(MEMORY_TIER) + if (memoryTierConfigs.size()) { + throw std::runtime_error( + "Using custom memory tier is only supported for Shared Memory."); + } initCommon(false); } template CacheAllocator::CacheAllocator(SharedMemNewT, Config config) - : isOnShm_{true}, + : memoryTierConfigs(config.getMemoryTierConfigs()), + isOnShm_{true}, config_(config.validate()), shmManager_( - std::make_unique(config_.cacheDir, config_.usePosixShm)), + std::make_unique(config_.cacheDir, config_.isUsingPosixShm())), allocator_(createNewMemoryAllocator()), compactCacheManager_(std::make_unique(*allocator_)), compressor_(createPtrCompressor()), @@ -69,7 +76,7 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) config_.accessConfig.getNumBuckets()), nullptr, ShmSegmentOpts(config_.accessConfig.getPageSize(), - false, config_.usePosixShm)) + false, config_.isUsingPosixShm())) .addr, compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), @@ -81,7 +88,7 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) config_.chainedItemAccessConfig.getNumBuckets()), nullptr, ShmSegmentOpts(config_.accessConfig.getPageSize(), - false, config_.usePosixShm)) + false, config_.isUsingPosixShm())) .addr, compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), @@ -92,12 +99,13 @@ CacheAllocator::CacheAllocator(SharedMemNewT, Config config) config_.isNvmCacheTruncateAllocSizeEnabled()} { initCommon(false); shmManager_->removeShm(detail::kShmInfoName, - PosixSysVSegmentOpts(config_.usePosixShm)); + PosixSysVSegmentOpts(config_.isUsingPosixShm())); } template CacheAllocator::CacheAllocator(SharedMemAttachT, Config config) - : isOnShm_{true}, + : memoryTierConfigs(config.getMemoryTierConfigs()), + isOnShm_{true}, config_(config.validate()), shmManager_( std::make_unique(config_.cacheDir, config_.usePosixShm)), @@ -111,14 +119,14 @@ CacheAllocator::CacheAllocator(SharedMemAttachT, Config config) deserializer_->deserialize(), config_.accessConfig, shmManager_->attachShm(detail::kShmHashTableName, nullptr, - ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)), + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.isUsingPosixShm())), compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), chainedItemAccessContainer_(std::make_unique( deserializer_->deserialize(), config_.chainedItemAccessConfig, shmManager_->attachShm(detail::kShmChainedItemHashTableName, nullptr, - ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)), + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.isUsingPosixShm())), compressor_, [this](Item* it) -> ItemHandle { return acquire(it); })), chainedItemLocks_(config_.chainedItemsLockPower, @@ -136,7 +144,7 @@ CacheAllocator::CacheAllocator(SharedMemAttachT, Config config) // 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)); + PosixSysVSegmentOpts(config_.isUsingPosixShm())); } template @@ -150,16 +158,35 @@ CacheAllocator::~CacheAllocator() { } template -std::unique_ptr -CacheAllocator::createNewMemoryAllocator() { +ShmSegmentOpts CacheAllocator::createShmCacheOpts() { + if (memoryTierConfigs.size() > 1) { + throw std::invalid_argument("CacheLib only supports a single memory tier"); + } + ShmSegmentOpts opts; opts.alignment = sizeof(Slab); - opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); + + // If memoryTierConfigs is empty, Fallback to Posix/SysV segment + // to keep legacy bahavior + // TODO(MEMORY_TIER) - guarantee there is always at least one mem + // layer inside Config + if (memoryTierConfigs.size()) { + opts.typeOpts = FileShmSegmentOpts(memoryTierConfigs[0].path); + } else { + opts.typeOpts = PosixSysVSegmentOpts(config_.isUsingPosixShm()); + } + + return opts; +} + +template +std::unique_ptr +CacheAllocator::createNewMemoryAllocator() { return std::make_unique( getAllocatorConfig(config_), shmManager_ ->createShm(detail::kShmCacheName, config_.size, - config_.slabMemoryBaseAddr, opts) + config_.slabMemoryBaseAddr, createShmCacheOpts()) .addr, config_.size); } @@ -167,14 +194,11 @@ CacheAllocator::createNewMemoryAllocator() { template std::unique_ptr CacheAllocator::restoreMemoryAllocator() { - ShmSegmentOpts opts; - opts.alignment = sizeof(Slab); - opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); return std::make_unique( deserializer_->deserialize(), shmManager_ - ->attachShm(detail::kShmCacheName, config_.slabMemoryBaseAddr, opts) - .addr, + ->attachShm(detail::kShmCacheName, config_.slabMemoryBaseAddr, + createShmCacheOpts()).addr, config_.size, config_.disableFullCoredump); } @@ -274,7 +298,7 @@ void CacheAllocator::initWorkers() { template std::unique_ptr CacheAllocator::createDeserializer() { auto infoAddr = shmManager_->attachShm(detail::kShmInfoName, nullptr, - ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)); + ShmSegmentOpts(PageSizeT::NORMAL, false, config_.isUsingPosixShm())); return std::make_unique( reinterpret_cast(infoAddr.addr), reinterpret_cast(infoAddr.addr) + infoAddr.size); @@ -3051,7 +3075,7 @@ void CacheAllocator::saveRamCache() { ioBuf->coalesce(); ShmSegmentOpts opts; - opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm); + opts.typeOpts = PosixSysVSegmentOpts(config_.isUsingPosixShm()); void* infoAddr = shmManager_->createShm(detail::kShmInfoName, ioBuf->length(), nullptr, opts).addr; diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index 9b2831a0dd..27cf7b0ca6 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1607,6 +1607,8 @@ class CacheAllocator : public CacheBase { std::unique_ptr& worker, std::chrono::seconds timeout = std::chrono::seconds{0}); + ShmSegmentOpts createShmCacheOpts(); + std::unique_ptr createNewMemoryAllocator(); std::unique_ptr restoreMemoryAllocator(); std::unique_ptr restoreCCacheManager(); @@ -1714,6 +1716,8 @@ class CacheAllocator : public CacheBase { const Config config_{}; + const typename Config::MemoryTierConfigs memoryTierConfigs; + // Manages the temporary shared memory segment for memory allocator that // is not persisted when cache process exits. std::unique_ptr tempShm_; diff --git a/cachelib/allocator/CacheAllocatorConfig.h b/cachelib/allocator/CacheAllocatorConfig.h index 1bd3c75056..a61b1f07b4 100644 --- a/cachelib/allocator/CacheAllocatorConfig.h +++ b/cachelib/allocator/CacheAllocatorConfig.h @@ -877,7 +877,6 @@ CacheAllocatorConfig& CacheAllocatorConfig::configureMemoryTiers( return *this; } -//const std::vector& CacheAllocatorConfig::getMemoryTierConfigs() { template const typename CacheAllocatorConfig::MemoryTierConfigs& CacheAllocatorConfig::getMemoryTierConfigs() { return memoryTierConfigs; diff --git a/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp new file mode 100644 index 0000000000..b784729157 --- /dev/null +++ b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp @@ -0,0 +1,29 @@ +/* + * Copyright (c) Intel Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "cachelib/allocator/tests/AllocatorMemoryTiersTest.h" + +namespace facebook { +namespace cachelib { +namespace tests { + +using LruAllocatorMemoryTiersTest = AllocatorMemoryTiersTest; + +TEST_F(LruAllocatorMemoryTiersTest, MultiTiers) { this->testMultiTiers(); } + +} // end of namespace tests +} // end of namespace cachelib +} // end of namespace facebook diff --git a/cachelib/allocator/tests/AllocatorMemoryTiersTest.h b/cachelib/allocator/tests/AllocatorMemoryTiersTest.h new file mode 100644 index 0000000000..8208c6b19f --- /dev/null +++ b/cachelib/allocator/tests/AllocatorMemoryTiersTest.h @@ -0,0 +1,47 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include "cachelib/allocator/CacheAllocatorConfig.h" +#include "cachelib/allocator/MemoryTierCacheConfig.h" +#include "cachelib/allocator/tests/TestBase.h" + +namespace facebook { +namespace cachelib { +namespace tests { + +template +class AllocatorMemoryTiersTest : public AllocatorTest { + public: + void testMultiTiers() { + typename AllocatorT::Config config; + config.setCacheSize(100 * Slab::kSize); + config.configureMemoryTiers({ + MemoryTierCacheConfig::fromFile("/tmp/a" + std::to_string(::getpid())) + .setRatio(1), + MemoryTierCacheConfig::fromFile("/tmp/b" + std::to_string(::getpid())) + .setRatio(1) + }); + + // More than one tier is not supported + ASSERT_THROW(std::make_unique(AllocatorT::SharedMemNew, config), + std::invalid_argument); + } +}; +} // namespace tests +} // namespace cachelib +} // namespace facebook diff --git a/cachelib/allocator/tests/AllocatorTypeTest.cpp b/cachelib/allocator/tests/AllocatorTypeTest.cpp index f8bb1df9eb..18c4f64044 100644 --- a/cachelib/allocator/tests/AllocatorTypeTest.cpp +++ b/cachelib/allocator/tests/AllocatorTypeTest.cpp @@ -16,6 +16,7 @@ #include "cachelib/allocator/tests/BaseAllocatorTest.h" #include "cachelib/allocator/tests/TestBase.h" +#include "cachelib/allocator/MemoryTierCacheConfig.h" namespace facebook { namespace cachelib { @@ -215,6 +216,12 @@ TYPED_TEST(BaseAllocatorTest, ReaperOutOfBound) { } TYPED_TEST(BaseAllocatorTest, ReaperShutDown) { this->testReaperShutDown(); } +TYPED_TEST(BaseAllocatorTest, ReaperShutDownFile) { + this->testReaperShutDown({ + MemoryTierCacheConfig::fromFile("/tmp/a" + std::to_string(::getpid())) + .setRatio(1) + }); +} TYPED_TEST(BaseAllocatorTest, ShutDownWithActiveHandles) { this->testShutDownWithActiveHandles(); diff --git a/cachelib/allocator/tests/BaseAllocatorTest.h b/cachelib/allocator/tests/BaseAllocatorTest.h index 8bbb380891..c1898d0c0c 100644 --- a/cachelib/allocator/tests/BaseAllocatorTest.h +++ b/cachelib/allocator/tests/BaseAllocatorTest.h @@ -1140,7 +1140,7 @@ class BaseAllocatorTest : public AllocatorTest { this->testLruLength(alloc, poolId, sizes, keyLen, evictedKeys); } - void testReaperShutDown() { + void testReaperShutDown(typename AllocatorT::Config::MemoryTierConfigs cfgs = {}) { const size_t nSlabs = 20; const size_t size = nSlabs * Slab::kSize; @@ -1150,6 +1150,8 @@ class BaseAllocatorTest : public AllocatorTest { config.setAccessConfig({8, 8}); config.enableCachePersistence(this->cacheDir_); config.enableItemReaperInBackground(std::chrono::seconds(1), {}); + if (cfgs.size()) + config.configureMemoryTiers(cfgs); std::vector keys; { AllocatorT alloc(AllocatorT::SharedMemNew, config); diff --git a/cachelib/shm/ShmCommon.h b/cachelib/shm/ShmCommon.h index c3363f4e34..b574c3d0fb 100644 --- a/cachelib/shm/ShmCommon.h +++ b/cachelib/shm/ShmCommon.h @@ -57,7 +57,8 @@ struct ShmSegmentOpts { PageSizeT pageSize{PageSizeT::NORMAL}; bool readOnly{false}; size_t alignment{1}; // alignment for mapping. - ShmTypeOpts typeOpts{}; // opts specific to segment type + // opts specific to segment type + ShmTypeOpts typeOpts{PosixSysVSegmentOpts(false)}; explicit ShmSegmentOpts(PageSizeT p) : pageSize(p) {} explicit ShmSegmentOpts(PageSizeT p, bool ro) : pageSize(p), readOnly(ro) {} From 6b4f46bd8552745566a04073db5d392a22d30fa6 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Fri, 5 Nov 2021 21:03:17 -0400 Subject: [PATCH 07/13] Add MemoryTierCacheConfig::fromShm() to allow using new configureMemoryTiers() API with legacy behavior. Move validation code for memory tiers to validate() method and convert ratios to sizes lazily (on get).. --- cachelib/allocator/CacheAllocator-inl.h | 30 ++-- cachelib/allocator/CacheAllocatorConfig.h | 166 ++++++++++++------ cachelib/allocator/MemoryTierCacheConfig.h | 23 +-- .../tests/AllocatorMemoryTiersTest.cpp | 1 + cachelib/allocator/tests/BaseAllocatorTest.h | 6 +- cachelib/allocator/tests/MemoryTiersTest.cpp | 27 +-- cachelib/shm/ShmCommon.h | 1 - 7 files changed, 159 insertions(+), 95 deletions(-) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index d285de2fef..fc485c2ae9 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -27,14 +27,16 @@ CacheAllocator::CacheAllocator(Config config) : memoryTierConfigs(config.getMemoryTierConfigs()), isOnShm_{config.memMonitoringEnabled()}, config_(config.validate()), - tempShm_(isOnShm_ ? std::make_unique(config_.size) + tempShm_(isOnShm_ ? std::make_unique( + config_.getCacheSize()) : nullptr), allocator_(isOnShm_ ? std::make_unique( getAllocatorConfig(config_), tempShm_->getAddr(), - config_.size) + config_.getCacheSize()) : std::make_unique( - getAllocatorConfig(config_), config_.size)), + getAllocatorConfig(config_), + config_.getCacheSize())), compactCacheManager_(std::make_unique(*allocator_)), compressor_(createPtrCompressor()), accessContainer_(std::make_unique( @@ -51,7 +53,8 @@ CacheAllocator::CacheAllocator(Config config) nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(), config_.isNvmCacheTruncateAllocSizeEnabled()} { // TODO(MEMORY_TIER) - if (memoryTierConfigs.size()) { + if (std::holds_alternative( + memoryTierConfigs[0].getShmTypeOpts())) { throw std::runtime_error( "Using custom memory tier is only supported for Shared Memory."); } @@ -165,16 +168,7 @@ ShmSegmentOpts CacheAllocator::createShmCacheOpts() { ShmSegmentOpts opts; opts.alignment = sizeof(Slab); - - // If memoryTierConfigs is empty, Fallback to Posix/SysV segment - // to keep legacy bahavior - // TODO(MEMORY_TIER) - guarantee there is always at least one mem - // layer inside Config - if (memoryTierConfigs.size()) { - opts.typeOpts = FileShmSegmentOpts(memoryTierConfigs[0].path); - } else { - opts.typeOpts = PosixSysVSegmentOpts(config_.isUsingPosixShm()); - } + opts.typeOpts = memoryTierConfigs[0].getShmTypeOpts(); return opts; } @@ -185,10 +179,10 @@ CacheAllocator::createNewMemoryAllocator() { return std::make_unique( getAllocatorConfig(config_), shmManager_ - ->createShm(detail::kShmCacheName, config_.size, + ->createShm(detail::kShmCacheName, config_.getCacheSize(), config_.slabMemoryBaseAddr, createShmCacheOpts()) .addr, - config_.size); + config_.getCacheSize()); } template @@ -199,7 +193,7 @@ CacheAllocator::restoreMemoryAllocator() { shmManager_ ->attachShm(detail::kShmCacheName, config_.slabMemoryBaseAddr, createShmCacheOpts()).addr, - config_.size, + config_.getCacheSize(), config_.disableFullCoredump); } @@ -2216,7 +2210,7 @@ PoolEvictionAgeStats CacheAllocator::getPoolEvictionAgeStats( template CacheMetadata CacheAllocator::getCacheMetadata() const noexcept { return CacheMetadata{kCachelibVersion, kCacheRamFormatVersion, - kCacheNvmFormatVersion, config_.size}; + kCacheNvmFormatVersion, config_.getCacheSize()}; } template diff --git a/cachelib/allocator/CacheAllocatorConfig.h b/cachelib/allocator/CacheAllocatorConfig.h index a61b1f07b4..cb578717cb 100644 --- a/cachelib/allocator/CacheAllocatorConfig.h +++ b/cachelib/allocator/CacheAllocatorConfig.h @@ -200,10 +200,13 @@ class CacheAllocatorConfig { // Configures cache memory tiers. Accepts vector of MemoryTierCacheConfig. // Each vector element describes configuration for a single memory cache tier. + // @throw std::invalid_argument if: + // - the size of configs is 0 + // - memory tiers use both size and ratio parameters CacheAllocatorConfig& configureMemoryTiers(const MemoryTierConfigs& configs); - // Return reference to MemoryTierCacheConfigs. - const MemoryTierConfigs& getMemoryTierConfigs(); + // Return vector of memory tier configs. + MemoryTierConfigs getMemoryTierConfigs() const; // This turns on a background worker that periodically scans through the // access container and look for expired items and remove them. @@ -334,7 +337,7 @@ class CacheAllocatorConfig { const std::string& getCacheName() const noexcept { return cacheName; } - size_t getCacheSize() const noexcept { return size; } + size_t getCacheSize() const noexcept; bool isUsingPosixShm() const noexcept { return usePosixShm; } @@ -552,12 +555,16 @@ class CacheAllocatorConfig { // cache. uint64_t nvmAdmissionMinTTL{0}; - // Configuration for memory tiers. - MemoryTierConfigs memoryTierConfigs; - friend CacheT; private: + void validateMemoryTiersWithSize(const MemoryTierConfigs&, size_t) const; + + // Configuration for memory tiers. + MemoryTierConfigs memoryTierConfigs{ + {MemoryTierCacheConfig::fromShm().setRatio(1)} + }; + void mergeWithPrefix( std::map& configMap, const std::map& configMapToMerge, @@ -576,6 +583,8 @@ CacheAllocatorConfig& CacheAllocatorConfig::setCacheName( template CacheAllocatorConfig& CacheAllocatorConfig::setCacheSize(size_t _size) { + validateMemoryTiersWithSize(this->memoryTierConfigs, _size); + size = _size; constexpr size_t maxCacheSizeWithCoredump = 64'424'509'440; // 60GB if (size <= maxCacheSizeWithCoredump) { @@ -818,68 +827,62 @@ CacheAllocatorConfig& CacheAllocatorConfig::enableItemReaperInBackground( template CacheAllocatorConfig& CacheAllocatorConfig::configureMemoryTiers( const MemoryTierConfigs& config) { - memoryTierConfigs = config; - size_t sum_ratios = 0; - size_t sum_sizes = 0; + if (!config.size()) { + throw std::invalid_argument("There must be at least one memory tier."); + } - for (auto tier_config: memoryTierConfigs) { + for (auto tier_config: config) { auto tier_size = tier_config.getSize(); auto tier_ratio = tier_config.getRatio(); if ((!tier_size and !tier_ratio) || (tier_size and tier_ratio)) { throw std::invalid_argument( "For each memory tier either size or ratio must be set."); } - sum_ratios += tier_ratio; - sum_sizes += tier_size; } - if (sum_ratios) { - if (!getCacheSize()) { - throw std::invalid_argument( - "Total cache size must be specified when size ratios are \ - used to specify memory tier sizes."); - } else { - if (getCacheSize() < sum_ratios) { - throw std::invalid_argument( - "Sum of all tier size ratios is greater than total cache size."); - } - // Convert ratios to sizes - sum_sizes = 0; - size_t partition_size = getCacheSize() / sum_ratios; - for (auto& tier_config: memoryTierConfigs) { - tier_config.setSize(partition_size * tier_config.getRatio()); - sum_sizes += tier_config.getSize(); - } - if (getCacheSize() != sum_sizes) { - // Adjust capacity of the last tier to account for rounding error - memoryTierConfigs.back().setSize(memoryTierConfigs.back().getSize() + \ - (getCacheSize() - sum_sizes)); - sum_sizes = getCacheSize(); - } - } - } else if (sum_sizes) { - if (getCacheSize() && sum_sizes != getCacheSize()) { - throw std::invalid_argument( - "Sum of tier sizes doesn't match total cache size. \ - Setting of cache total size is not required when per-tier \ - sizes are specified - it is calculated as sum of tier sizes."); - } - } else { - throw std::invalid_argument( - "Either sum of all memory tiers sizes or sum of all ratios \ - must be greater than 0."); - } + validateMemoryTiersWithSize(config, this->size); - if (sum_sizes && !getCacheSize()) { - setCacheSize(sum_sizes); - } + memoryTierConfigs = config; return *this; } template -const typename CacheAllocatorConfig::MemoryTierConfigs& CacheAllocatorConfig::getMemoryTierConfigs() { - return memoryTierConfigs; +typename CacheAllocatorConfig::MemoryTierConfigs +CacheAllocatorConfig::getMemoryTierConfigs() const { + MemoryTierConfigs config = memoryTierConfigs; + size_t sum_ratios = 0; + + for (auto &tier_config: config) { + if (auto *v = std::get_if(&tier_config.shmOpts)) { + v->usePosix = usePosixShm; + } + + sum_ratios += tier_config.getRatio(); + } + + if (sum_ratios == 0) + return config; + + // if ratios are used, size must be specified + XDCHECK(size); + + // Convert ratios to sizes, size must be non-zero + size_t sum_sizes = 0; + size_t partition_size = size / sum_ratios; + for (auto& tier_config: config) { + tier_config.setSize(partition_size * tier_config.getRatio()); + tier_config.setRatio(0); + sum_sizes += tier_config.getSize(); + } + + if (size != sum_sizes) { + // Adjust capacity of the last tier to account for rounding error + config.back().setSize( + config.back().getSize() + (getCacheSize() - sum_sizes)); + } + + return config; } template @@ -997,6 +1000,46 @@ CacheAllocatorConfig& CacheAllocatorConfig::setNvmAdmissionMinTTL( return *this; } +template +size_t CacheAllocatorConfig::getCacheSize() const noexcept { + if (size) + return size; + + size_t sum_sizes = 0; + for (const auto &tier_config : getMemoryTierConfigs()) { + sum_sizes += tier_config.getSize(); + } + + return sum_sizes; +} + +template +void CacheAllocatorConfig::validateMemoryTiersWithSize( + const MemoryTierConfigs &config, size_t size) const { + size_t sum_ratios = 0; + size_t sum_sizes = 0; + + for (const auto &tier_config: config) { + sum_ratios += tier_config.getRatio(); + sum_sizes += tier_config.getSize(); + } + + if (sum_ratios && sum_sizes) { + throw std::invalid_argument("Cannot mix ratios and sizes."); + } else if (sum_sizes) { + if (size && sum_sizes != size) { + throw std::invalid_argument( + "Sum of tier sizes doesn't match total cache size. " + "Setting of cache total size is not required when per-tier " + "sizes are specified - it is calculated as sum of tier sizes."); + } + } else if (!sum_ratios && !sum_sizes) { + throw std::invalid_argument( + "Either sum of all memory tiers sizes or sum of all ratios " + "must be greater than 0."); + } +} + template const CacheAllocatorConfig& CacheAllocatorConfig::validate() const { // we can track tail hits only if MMType is MM2Q @@ -1018,6 +1061,23 @@ const CacheAllocatorConfig& CacheAllocatorConfig::validate() const { size, maxCacheSize)); } + + size_t sum_ratios = 0; + for (auto tier_config: memoryTierConfigs) { + sum_ratios += tier_config.getRatio(); + } + + if (sum_ratios) { + if (!size) { + throw std::invalid_argument( + "Total cache size must be specified when size ratios are " + "used to specify memory tier sizes."); + } else if (size < sum_ratios) { + throw std::invalid_argument( + "Sum of all tier size ratios is greater than total cache size."); + } + } + return *this; } diff --git a/cachelib/allocator/MemoryTierCacheConfig.h b/cachelib/allocator/MemoryTierCacheConfig.h index 5e3604a0af..12fd2c91f0 100644 --- a/cachelib/allocator/MemoryTierCacheConfig.h +++ b/cachelib/allocator/MemoryTierCacheConfig.h @@ -18,6 +18,8 @@ #include +#include "cachelib/shm/ShmCommon.h" + namespace facebook { namespace cachelib { class MemoryTierCacheConfig { @@ -27,7 +29,14 @@ class MemoryTierCacheConfig { // TODO: add fromDirectory, fromAnonymousMemory static MemoryTierCacheConfig fromFile(const std::string& _file) { MemoryTierCacheConfig config; - config.path = _file; + config.shmOpts = FileShmSegmentOpts(_file); + return config; + } + + // Creates instance of MemoryTierCacheConfig for Posix/SysV Shared memory. + static MemoryTierCacheConfig fromShm() { + MemoryTierCacheConfig config; + config.shmOpts = PosixSysVSegmentOpts(); return config; } @@ -53,11 +62,7 @@ class MemoryTierCacheConfig { size_t getSize() const noexcept { return size; } - const std::string& getPath() const noexcept { return path; } - - bool isFileBacked() const { - return !path.empty(); - } + const ShmTypeOpts& getShmTypeOpts() const noexcept { return shmOpts; } // Size of this memory tiers size_t size{0}; @@ -67,10 +72,8 @@ class MemoryTierCacheConfig { // then size of the i-th tier Xi = (X / (Y1 + Y2)) * Yi and X = sum(Xi) size_t ratio{0}; - // Path to file for file system-backed memory tier - // TODO: consider using variant to support different - // memory sources - std::string path; + // Options specific to shm type + ShmTypeOpts shmOpts; private: MemoryTierCacheConfig() = default; diff --git a/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp index b784729157..b6db9ce168 100644 --- a/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp +++ b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp @@ -22,6 +22,7 @@ namespace tests { using LruAllocatorMemoryTiersTest = AllocatorMemoryTiersTest; +// TODO(MEMORY_TIER): add more tests with different eviction policies TEST_F(LruAllocatorMemoryTiersTest, MultiTiers) { this->testMultiTiers(); } } // end of namespace tests diff --git a/cachelib/allocator/tests/BaseAllocatorTest.h b/cachelib/allocator/tests/BaseAllocatorTest.h index c1898d0c0c..dce17f7ceb 100644 --- a/cachelib/allocator/tests/BaseAllocatorTest.h +++ b/cachelib/allocator/tests/BaseAllocatorTest.h @@ -1140,7 +1140,8 @@ class BaseAllocatorTest : public AllocatorTest { this->testLruLength(alloc, poolId, sizes, keyLen, evictedKeys); } - void testReaperShutDown(typename AllocatorT::Config::MemoryTierConfigs cfgs = {}) { + void testReaperShutDown(typename AllocatorT::Config::MemoryTierConfigs cfgs = + {MemoryTierCacheConfig::fromShm().setRatio(1)}) { const size_t nSlabs = 20; const size_t size = nSlabs * Slab::kSize; @@ -1150,8 +1151,7 @@ class BaseAllocatorTest : public AllocatorTest { config.setAccessConfig({8, 8}); config.enableCachePersistence(this->cacheDir_); config.enableItemReaperInBackground(std::chrono::seconds(1), {}); - if (cfgs.size()) - config.configureMemoryTiers(cfgs); + config.configureMemoryTiers(cfgs); std::vector keys; { AllocatorT alloc(AllocatorT::SharedMemNew, config); diff --git a/cachelib/allocator/tests/MemoryTiersTest.cpp b/cachelib/allocator/tests/MemoryTiersTest.cpp index f578ed3ea3..6e5616fcdb 100644 --- a/cachelib/allocator/tests/MemoryTiersTest.cpp +++ b/cachelib/allocator/tests/MemoryTiersTest.cpp @@ -59,7 +59,8 @@ class MemoryTiersTest: public AllocatorTest { } for(auto i = 0; i < configs.size(); ++i) { - EXPECT_EQ(configs[i].getPath(), expectedPaths[i]); + auto &opt = std::get(configs[i].getShmTypeOpts()); + EXPECT_EQ(opt.path, expectedPaths[i]); EXPECT_GT(configs[i].getSize(), 0); if (configs[i].getRatio() && (i < configs.size() - 1)) { EXPECT_EQ(configs[i].getSize(), partition_size * configs[i].getRatio()); @@ -98,12 +99,12 @@ class MemoryTiersTest: public AllocatorTest { using LruMemoryTiersTest = MemoryTiersTest; TEST_F(LruMemoryTiersTest, TestValid1TierPmemRatioConfig) { - LruAllocatorConfig cfg = createTestCacheConfig({defaultPmemPath}).validate(); + LruAllocatorConfig cfg = createTestCacheConfig({defaultPmemPath}); basicCheck(cfg); } TEST_F(LruMemoryTiersTest, TestValid1TierDaxRatioConfig) { - LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath}).validate(); + LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath}); basicCheck(cfg, {defaultDaxPath}); } @@ -111,19 +112,22 @@ TEST_F(LruMemoryTiersTest, TestValid1TierDaxSizeConfig) { LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath}, {std::make_tuple(0, defaultTotalCacheSize)}, /* setPosixShm */ true, - /* cacheSize */ 0).validate(); + /* cacheSize */ 0); basicCheck(cfg, {defaultDaxPath}); + + // Setting size after conifguringMemoryTiers with sizes is not allowed. + EXPECT_THROW(cfg.setCacheSize(defaultTotalCacheSize + 1), std::invalid_argument); } TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemConfig) { LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, - {std::make_tuple(1, 0), std::make_tuple(1, 0)}).validate(); + {std::make_tuple(1, 0), std::make_tuple(1, 0)}); basicCheck(cfg, {defaultDaxPath, defaultPmemPath}); } TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemRatioConfig) { LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, - {std::make_tuple(5, 0), std::make_tuple(2, 0)}).validate(); + {std::make_tuple(5, 0), std::make_tuple(2, 0)}); basicCheck(cfg, {defaultDaxPath, defaultPmemPath}); } @@ -131,19 +135,22 @@ TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemSizeConfig) { size_t size_1 = 4321, size_2 = 1234; LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, {std::make_tuple(0, size_1), std::make_tuple(0, size_2)}, - true, 0).validate(); + true, 0); basicCheck(cfg, {defaultDaxPath, defaultPmemPath}, size_1 + size_2); + + // Setting size after conifguringMemoryTiers with sizes is not allowed. + EXPECT_THROW(cfg.setCacheSize(size_1 + size_2 + 1), std::invalid_argument); } TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigPosixShmNotSet) { LruAllocatorConfig cfg = createTestCacheConfig({defaultDaxPath, defaultPmemPath}, {std::make_tuple(1, 0), std::make_tuple(1, 0)}, - /* setPosixShm */ false).validate(); + /* setPosixShm */ false); } TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigNumberOfPartitionsTooLarge) { EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, - {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}), + {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}).validate(), std::invalid_argument); } @@ -165,7 +172,7 @@ TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigSizesAndRatioNotSet) { TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigRatiosCacheSizeNotSet) { EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, {std::make_tuple(1, 0), std::make_tuple(1, 0)}, - /* setPosixShm */ true, /* cacheSize */ 0), + /* setPosixShm */ true, /* cacheSize */ 0).validate(), std::invalid_argument); } diff --git a/cachelib/shm/ShmCommon.h b/cachelib/shm/ShmCommon.h index b574c3d0fb..807237d6f5 100644 --- a/cachelib/shm/ShmCommon.h +++ b/cachelib/shm/ShmCommon.h @@ -40,7 +40,6 @@ enum PageSizeT { constexpr int kInvalidFD = -1; -// TODO(SHM_FILE): maybe we could use this inside the Tier Config class? struct FileShmSegmentOpts { FileShmSegmentOpts(std::string path = ""): path(path) {} std::string path; From f401f176cb42b46295e55cda7555eac7b9530117 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Mon, 8 Nov 2021 19:46:04 -0500 Subject: [PATCH 08/13] Fix test_shm_manager.cpp test It wrongly assumed that the only possible segment type is PosixSysV segment. --- cachelib/shm/tests/test_shm_manager.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cachelib/shm/tests/test_shm_manager.cpp b/cachelib/shm/tests/test_shm_manager.cpp index 014e93d04d..1343c84c77 100644 --- a/cachelib/shm/tests/test_shm_manager.cpp +++ b/cachelib/shm/tests/test_shm_manager.cpp @@ -797,8 +797,8 @@ void ShmManagerTest::testShutDown(bool posix) { ASSERT_NO_THROW(s.createShm(seg2, seg2Size, nullptr, seg2Opt)); ASSERT_EQ(s.getShmByName(seg2).getSize(), seg2Size); auto *v = std::get_if(&s.getShmTypeByName(seg2)); - ASSERT_TRUE(v); - ASSERT_EQ(v->usePosix, posix); + if (v) + ASSERT_EQ(v->usePosix, posix); ASSERT_TRUE(s.shutDown() == ShutDownRes::kSuccess); }; From e101f94ff679faa8277165ec15dd48919972d0eb Mon Sep 17 00:00:00 2001 From: "Chorazewicz, Igor" Date: Fri, 5 Nov 2021 14:23:40 +0100 Subject: [PATCH 09/13] Run tests on CI --- .github/workflows/build-cachelib-centos.yml | 3 +++ .github/workflows/build-cachelib-debian.yml | 3 +++ run_tests.sh | 10 ++++++++++ 3 files changed, 16 insertions(+) create mode 100755 run_tests.sh diff --git a/.github/workflows/build-cachelib-centos.yml b/.github/workflows/build-cachelib-centos.yml index 5cd28db1b6..ab5bf4d2cd 100644 --- a/.github/workflows/build-cachelib-centos.yml +++ b/.github/workflows/build-cachelib-centos.yml @@ -34,3 +34,6 @@ jobs: uses: actions/checkout@v2 - name: "build CacheLib using build script" run: ./contrib/build.sh -j -v -T + - name: "run tests" + timeout-minutes: 60 + run: cd opt/cachelib/tests && ../../../run_tests.sh diff --git a/.github/workflows/build-cachelib-debian.yml b/.github/workflows/build-cachelib-debian.yml index 182759e175..6aeda6e535 100644 --- a/.github/workflows/build-cachelib-debian.yml +++ b/.github/workflows/build-cachelib-debian.yml @@ -38,3 +38,6 @@ jobs: uses: actions/checkout@v2 - name: "build CacheLib using build script" run: ./contrib/build.sh -j -v -T + - name: "run tests" + timeout-minutes: 60 + run: cd opt/cachelib/tests && ../../../run_tests.sh diff --git a/run_tests.sh b/run_tests.sh new file mode 100755 index 0000000000..baa9bfee0a --- /dev/null +++ b/run_tests.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +# Newline separated list of tests to ignore +BLACKLIST="allocator-test-AllocationClassTest +allocator-test-NvmCacheTests +common-test-TimeTests +common-test-UtilTests +shm-test-test_page_size" + +find -type f \( -not -name "*bench*" -and -not -name "navy*" \) -executable | grep -vF "$BLACKLIST" | xargs -n1 bash -c From 788145630401a973b79cf693e0ffb50d6eff83a2 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Tue, 16 Nov 2021 16:41:16 -0500 Subject: [PATCH 10/13] Run long tests (navy/bench) every day on CI --- .../workflows/build-cachelib-centos-long.yml | 39 +++++++++++++++++++ run_tests.sh | 6 ++- 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/build-cachelib-centos-long.yml diff --git a/.github/workflows/build-cachelib-centos-long.yml b/.github/workflows/build-cachelib-centos-long.yml new file mode 100644 index 0000000000..92165f603b --- /dev/null +++ b/.github/workflows/build-cachelib-centos-long.yml @@ -0,0 +1,39 @@ +name: build-cachelib-centos-latest +on: + schedule: + - cron: '0 7 * * *' + +jobs: + build-cachelib-centos8-latest: + name: "CentOS/latest - Build CacheLib with all dependencies" + runs-on: ubuntu-latest + # Docker container image name + container: "centos:latest" + steps: + - name: "update packages" + run: dnf upgrade -y + - name: "install sudo,git" + run: dnf install -y sudo git cmake gcc + - name: "System Information" + run: | + echo === uname === + uname -a + echo === /etc/os-release === + cat /etc/os-release + echo === df -hl === + df -hl + echo === free -h === + free -h + echo === top === + top -b -n1 -1 -Eg || timeout 1 top -b -n1 + echo === env === + env + echo === gcc -v === + gcc -v + - name: "checkout sources" + uses: actions/checkout@v2 + - name: "build CacheLib using build script" + run: ./contrib/build.sh -j -v -T + - name: "run tests" + timeout-minutes: 60 + run: cd opt/cachelib/tests && ../../../run_tests.sh long diff --git a/run_tests.sh b/run_tests.sh index baa9bfee0a..9a54cf442b 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -7,4 +7,8 @@ common-test-TimeTests common-test-UtilTests shm-test-test_page_size" -find -type f \( -not -name "*bench*" -and -not -name "navy*" \) -executable | grep -vF "$BLACKLIST" | xargs -n1 bash -c +if [ "$1" == "long" ]; then + find -type f -executable | grep -vF "$BLACKLIST" | xargs -n1 bash -c +else + find -type f \( -not -name "*bench*" -and -not -name "navy*" \) -executable | grep -vF "$BLACKLIST" | xargs -n1 bash -c +fi From a0d4a7ccaf401b228da1a21f7a9aa075c59af363 Mon Sep 17 00:00:00 2001 From: Sounak Gupta Date: Sat, 6 Nov 2021 17:43:18 -0700 Subject: [PATCH 11/13] Moved common segment code for posix and file shm segments into ShmCommon --- cachelib/shm/FileShmSegment.cpp | 154 ++----------------------------- cachelib/shm/PosixShmSegment.cpp | 152 ++---------------------------- cachelib/shm/ShmCommon.cpp | 131 ++++++++++++++++++++++++++ cachelib/shm/ShmCommon.h | 29 +++++- 4 files changed, 173 insertions(+), 293 deletions(-) diff --git a/cachelib/shm/FileShmSegment.cpp b/cachelib/shm/FileShmSegment.cpp index 40628aebf6..ff78b50cee 100644 --- a/cachelib/shm/FileShmSegment.cpp +++ b/cachelib/shm/FileShmSegment.cpp @@ -27,149 +27,6 @@ namespace facebook { namespace cachelib { -constexpr static mode_t kRWMode = 0666; -typedef struct stat stat_t; - -namespace detail { - -// TODO(SHM_FILE): move those *Impl functions to common file, there are copied -// from PosixShmSegment.cpp -static int openImpl(const char* name, int flags) { - const int fd = open(name, flags); - - if (fd != -1) { - return fd; - } - - switch (errno) { - case EEXIST: - case EMFILE: - case ENFILE: - case EACCES: - util::throwSystemError(errno); - break; - case ENAMETOOLONG: - case EINVAL: - util::throwSystemError(errno, "Invalid segment name"); - break; - case ENOENT: - if (!(flags & O_CREAT)) { - util::throwSystemError(errno); - } else { - XDCHECK(false); - // FIXME: posix says that ENOENT is thrown only when O_CREAT - // is not set. However, it seems to be set even when O_CREAT - // was set and the parent of path name does not exist. - util::throwSystemError(errno, "Invalid errno"); - } - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } - return kInvalidFD; -} - -static void unlinkImpl(const char* const name) { - const int ret = unlink(name); - if (ret == 0) { - return; - } - - switch (errno) { - case ENOENT: - case EACCES: - util::throwSystemError(errno); - break; - case ENAMETOOLONG: - case EINVAL: - util::throwSystemError(errno, "Invalid segment name"); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void ftruncateImpl(int fd, size_t size) { - const int ret = ftruncate(fd, size); - if (ret == 0) { - return; - } - switch (errno) { - case EBADF: - case EINVAL: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void fstatImpl(int fd, stat_t* buf) { - const int ret = fstat(fd, buf); - if (ret == 0) { - return; - } - switch (errno) { - case EBADF: - case ENOMEM: - case EOVERFLOW: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void* mmapImpl( - void* addr, size_t length, int prot, int flags, int fd, off_t offset) { - void* ret = mmap(addr, length, prot, flags, fd, offset); - if (ret != MAP_FAILED) { - return ret; - } - - switch (errno) { - case EACCES: - case EAGAIN: - if (flags & MAP_LOCKED) { - util::throwSystemError(ENOMEM); - break; - } - case EBADF: - case EINVAL: - case ENFILE: - case ENODEV: - case ENOMEM: - case EPERM: - case ETXTBSY: - case EOVERFLOW: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } - return nullptr; -} - -static void munmapImpl(void* addr, size_t length) { - const int ret = munmap(addr, length); - - if (ret == 0) { - return; - } else if (errno == EINVAL) { - util::throwSystemError(errno); - } else { - XDCHECK(false); - util::throwSystemError(EINVAL, "Invalid errno"); - } -} - -} // namespace detail - FileShmSegment::FileShmSegment(ShmAttachT, const std::string& name, ShmSegmentOpts opts) @@ -217,13 +74,15 @@ FileShmSegment::~FileShmSegment() { int FileShmSegment::createNewSegment(const std::string& name) { constexpr static int createFlags = O_RDWR | O_CREAT | O_EXCL; - return detail::openImpl(name.c_str(), createFlags); + detail::open_func_t open_func = std::bind(open, name.c_str(), createFlags); + return detail::openImpl(open_func, createFlags); } int FileShmSegment::getExisting(const std::string& name, const ShmSegmentOpts& opts) { int flags = opts.readOnly ? O_RDONLY : O_RDWR; - return detail::openImpl(name.c_str(), flags); + detail::open_func_t open_func = std::bind(open, name.c_str(), flags); + return detail::openImpl(open_func, flags); } void FileShmSegment::markForRemoval() { @@ -240,7 +99,8 @@ void FileShmSegment::markForRemoval() { bool FileShmSegment::removeByPath(const std::string& path) { try { - detail::unlinkImpl(path.c_str()); + detail::unlink_func_t unlink_func = std::bind(unlink, path.c_str()); + detail::unlinkImpl(unlink_func); return true; } catch (const std::system_error& e) { // unlink is opaque unlike sys-V api where its through the shmid. Hence @@ -263,7 +123,7 @@ size_t FileShmSegment::getSize() const { return buf.st_size; } else { throw std::runtime_error(folly::sformat( - "Trying to get size of segment with name {} in an invalid state", + "Trying to get size of segment with name {} in an invalid state", getName())); } return 0; diff --git a/cachelib/shm/PosixShmSegment.cpp b/cachelib/shm/PosixShmSegment.cpp index 42c9e2ba33..027fee8bb8 100644 --- a/cachelib/shm/PosixShmSegment.cpp +++ b/cachelib/shm/PosixShmSegment.cpp @@ -27,146 +27,7 @@ namespace facebook { namespace cachelib { -constexpr static mode_t kRWMode = 0666; -typedef struct stat stat_t; - -namespace detail { - -static int shmOpenImpl(const char* name, int flags) { - const int fd = shm_open(name, flags, kRWMode); - - if (fd != -1) { - return fd; - } - - switch (errno) { - case EEXIST: - case EMFILE: - case ENFILE: - case EACCES: - util::throwSystemError(errno); - break; - case ENAMETOOLONG: - case EINVAL: - util::throwSystemError(errno, "Invalid segment name"); - break; - case ENOENT: - if (!(flags & O_CREAT)) { - util::throwSystemError(errno); - } else { - XDCHECK(false); - // FIXME: posix says that ENOENT is thrown only when O_CREAT - // is not set. However, it seems to be set even when O_CREAT - // was set and the parent of path name does not exist. - util::throwSystemError(errno, "Invalid errno"); - } - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } - return kInvalidFD; -} - -static void shmUnlinkImpl(const char* const name) { - const int ret = shm_unlink(name); - if (ret == 0) { - return; - } - - switch (errno) { - case ENOENT: - case EACCES: - util::throwSystemError(errno); - break; - case ENAMETOOLONG: - case EINVAL: - util::throwSystemError(errno, "Invalid segment name"); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void ftruncateImpl(int fd, size_t size) { - const int ret = ftruncate(fd, size); - if (ret == 0) { - return; - } - switch (errno) { - case EBADF: - case EINVAL: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void fstatImpl(int fd, stat_t* buf) { - const int ret = fstat(fd, buf); - if (ret == 0) { - return; - } - switch (errno) { - case EBADF: - case ENOMEM: - case EOVERFLOW: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } -} - -static void* mmapImpl( - void* addr, size_t length, int prot, int flags, int fd, off_t offset) { - void* ret = mmap(addr, length, prot, flags, fd, offset); - if (ret != MAP_FAILED) { - return ret; - } - - switch (errno) { - case EACCES: - case EAGAIN: - if (flags & MAP_LOCKED) { - util::throwSystemError(ENOMEM); - break; - } - case EBADF: - case EINVAL: - case ENFILE: - case ENODEV: - case ENOMEM: - case EPERM: - case ETXTBSY: - case EOVERFLOW: - util::throwSystemError(errno); - break; - default: - XDCHECK(false); - util::throwSystemError(errno, "Invalid errno"); - } - return nullptr; -} - -static void munmapImpl(void* addr, size_t length) { - const int ret = munmap(addr, length); - - if (ret == 0) { - return; - } else if (errno == EINVAL) { - util::throwSystemError(errno); - } else { - XDCHECK(false); - util::throwSystemError(EINVAL, "Invalid errno"); - } -} - -} // namespace detail +constexpr mode_t kRWMode = 0666; PosixShmSegment::PosixShmSegment(ShmAttachT, const std::string& name, @@ -215,13 +76,15 @@ PosixShmSegment::~PosixShmSegment() { int PosixShmSegment::createNewSegment(const std::string& name) { constexpr static int createFlags = O_RDWR | O_CREAT | O_EXCL; - return detail::shmOpenImpl(name.c_str(), createFlags); + detail::open_func_t open_func = std::bind(shm_open, name.c_str(), createFlags, kRWMode); + return detail::openImpl(open_func, createFlags); } int PosixShmSegment::getExisting(const std::string& name, const ShmSegmentOpts& opts) { int flags = opts.readOnly ? O_RDONLY : O_RDWR; - return detail::shmOpenImpl(name.c_str(), flags); + detail::open_func_t open_func = std::bind(shm_open, name.c_str(), flags, kRWMode); + return detail::openImpl(open_func, flags); } void PosixShmSegment::markForRemoval() { @@ -239,7 +102,8 @@ void PosixShmSegment::markForRemoval() { bool PosixShmSegment::removeByName(const std::string& segmentName) { try { auto key = createKeyForName(segmentName); - detail::shmUnlinkImpl(key.c_str()); + detail::unlink_func_t unlink_func = std::bind(shm_unlink, key.c_str()); + detail::unlinkImpl(unlink_func); return true; } catch (const std::system_error& e) { // unlink is opaque unlike sys-V api where its through the shmid. Hence @@ -258,7 +122,7 @@ size_t PosixShmSegment::getSize() const { return buf.st_size; } else { throw std::runtime_error(folly::sformat( - "Trying to get size of segment with name {} in an invalid state", + "Trying to get size of segment with name {} in an invalid state", getName())); } return 0; diff --git a/cachelib/shm/ShmCommon.cpp b/cachelib/shm/ShmCommon.cpp index 9e6be122c4..11a753d865 100644 --- a/cachelib/shm/ShmCommon.cpp +++ b/cachelib/shm/ShmCommon.cpp @@ -22,6 +22,7 @@ #include #include #include +#include namespace facebook { namespace cachelib { @@ -157,6 +158,136 @@ PageSizeT getPageSizeInSMap(void* addr) { throw std::invalid_argument("address mapping not found in /proc/self/smaps"); } +int openImpl(open_func_t const& open_func, int flags) { + const int fd = open_func(); + if (fd == kInvalidFD) { + switch (errno) { + case EEXIST: + case EMFILE: + case ENFILE: + case EACCES: + util::throwSystemError(errno); + break; + case ENAMETOOLONG: + case EINVAL: + util::throwSystemError(errno, "Invalid segment name"); + break; + case ENOENT: + if (!(flags & O_CREAT)) { + util::throwSystemError(errno); + } else { + XDCHECK(false); + // FIXME: posix says that ENOENT is thrown only when O_CREAT + // is not set. However, it seems to be set even when O_CREAT + // was set and the parent of path name does not exist. + util::throwSystemError(errno, "Invalid errno"); + } + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } + } + return fd; +} + +void unlinkImpl(unlink_func_t const& unlink_func) { + const int fd = unlink_func(); + if (fd != kInvalidFD) { + return; + } + + switch (errno) { + case ENOENT: + case EACCES: + util::throwSystemError(errno); + break; + case ENAMETOOLONG: + case EINVAL: + util::throwSystemError(errno, "Invalid segment name"); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +void ftruncateImpl(int fd, size_t size) { + const int ret = ftruncate(fd, size); + if (ret == 0) { + return; + } + switch (errno) { + case EBADF: + case EINVAL: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +void fstatImpl(int fd, stat_t* buf) { + const int ret = fstat(fd, buf); + if (ret == 0) { + return; + } + switch (errno) { + case EBADF: + case ENOMEM: + case EOVERFLOW: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } +} + +void* mmapImpl(void* addr, size_t length, int prot, int flags, int fd, off_t offset) { + void* ret = mmap(addr, length, prot, flags, fd, offset); + if (ret != MAP_FAILED) { + return ret; + } + + switch (errno) { + case EACCES: + case EAGAIN: + if (flags & MAP_LOCKED) { + util::throwSystemError(ENOMEM); + break; + } + case EBADF: + case EINVAL: + case ENFILE: + case ENODEV: + case ENOMEM: + case EPERM: + case ETXTBSY: + case EOVERFLOW: + util::throwSystemError(errno); + break; + default: + XDCHECK(false); + util::throwSystemError(errno, "Invalid errno"); + } + return nullptr; +} + +void munmapImpl(void* addr, size_t length) { + const int ret = munmap(addr, length); + + if (ret == 0) { + return; + } else if (errno == EINVAL) { + util::throwSystemError(errno); + } else { + XDCHECK(false); + util::throwSystemError(EINVAL, "Invalid errno"); + } +} + } // namespace detail } // namespace cachelib } // namespace facebook diff --git a/cachelib/shm/ShmCommon.h b/cachelib/shm/ShmCommon.h index 807237d6f5..136842643d 100644 --- a/cachelib/shm/ShmCommon.h +++ b/cachelib/shm/ShmCommon.h @@ -20,6 +20,8 @@ #include #include +#include "cachelib/common/Utils.h" + #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wconversion" #include @@ -29,6 +31,10 @@ namespace facebook { namespace cachelib { +constexpr int kInvalidFD = -1; + +typedef struct stat stat_t; + enum ShmAttachT { ShmAttach }; enum ShmNewT { ShmNew }; @@ -38,8 +44,6 @@ enum PageSizeT { ONE_GB, }; -constexpr int kInvalidFD = -1; - struct FileShmSegmentOpts { FileShmSegmentOpts(std::string path = ""): path(path) {} std::string path; @@ -143,6 +147,27 @@ bool isPageAlignedAddr(void* addr, PageSizeT p = PageSizeT::NORMAL); // // @throw std::invalid_argument if the address mapping is not found. PageSizeT getPageSizeInSMap(void* addr); + +// @throw std::invalid_argument if the segment name is not created +typedef std::function open_func_t; +int openImpl(open_func_t const& open_func, int flags); + +// @throw std::invalid_argument if there is an error +typedef std::function unlink_func_t; +void unlinkImpl(unlink_func_t const& unlink_func); + +// @throw std::invalid_argument if there is an error +void ftruncateImpl(int fd, size_t size); + +// @throw std::invalid_argument if there is an error +void fstatImpl(int fd, stat_t* buf); + +// @throw std::invalid_argument if there is an error +void* mmapImpl(void* addr, size_t length, int prot, int flags, int fd, off_t offset); + +// @throw std::invalid_argument if there is an error +void munmapImpl(void* addr, size_t length); + } // namespace detail } // namespace cachelib } // namespace facebook From a49e9fd94b7092056e0f34b81d79b581b7f2bb29 Mon Sep 17 00:00:00 2001 From: victoria-mcgrath Date: Thu, 18 Nov 2021 14:49:26 -0800 Subject: [PATCH 12/13] Enabled memory tier config API for cachebench. --- cachelib/cachebench/cache/Cache-inl.h | 17 +++++++-- .../test_configs/simple_tiers_test.json | 36 +++++++++++++++++++ cachelib/cachebench/util/CacheConfig.cpp | 20 ++++++++++- cachelib/cachebench/util/CacheConfig.h | 24 +++++++++++++ 4 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 cachelib/cachebench/test_configs/simple_tiers_test.json diff --git a/cachelib/cachebench/cache/Cache-inl.h b/cachelib/cachebench/cache/Cache-inl.h index d9d3a1641a..5ac6ad40ab 100644 --- a/cachelib/cachebench/cache/Cache-inl.h +++ b/cachelib/cachebench/cache/Cache-inl.h @@ -94,6 +94,20 @@ Cache::Cache(const CacheConfig& config, allocatorConfig_.setCacheSize(config_.cacheSizeMB * (MB)); + if (!cacheDir.empty()) { + allocatorConfig_.cacheDir = cacheDir; + } else if (!config_.persistedCacheDir.empty()) { + allocatorConfig_.enableCachePersistence(config_.persistedCacheDir); + } + + if (config_.usePosixShm) { + allocatorConfig_.usePosixForShm(); + } + + if (config_.memoryTierConfigs.size()) { + allocatorConfig_.configureMemoryTiers(config_.memoryTierConfigs); + } + auto cleanupGuard = folly::makeGuard([&] { if (!nvmCacheFilePath_.empty()) { util::removePath(nvmCacheFilePath_); @@ -246,8 +260,7 @@ Cache::Cache(const CacheConfig& config, allocatorConfig_.cacheName = "cachebench"; - if (!cacheDir.empty()) { - allocatorConfig_.cacheDir = cacheDir; + if (!allocatorConfig_.cacheDir.empty()) { cache_ = std::make_unique(Allocator::SharedMemNew, allocatorConfig_); } else { diff --git a/cachelib/cachebench/test_configs/simple_tiers_test.json b/cachelib/cachebench/test_configs/simple_tiers_test.json new file mode 100644 index 0000000000..1a90a4ee51 --- /dev/null +++ b/cachelib/cachebench/test_configs/simple_tiers_test.json @@ -0,0 +1,36 @@ +// @nolint instantiates a small cache and runs a quick run of basic operations. +{ + "cache_config" : { + "cacheSizeMB" : 512, + "usePosixShm" : true, + "persistedCacheDir" : "/tmp/mem-tiers", + "memoryTiers" : [ + { + "ratio": 1, + "file": "/tmp/mem-tiers/memory-mapped-tier" + } + ], + "poolRebalanceIntervalSec" : 1, + "moveOnSlabRelease" : false, + + "numPools" : 2, + "poolSizes" : [0.3, 0.7] + }, + "test_config" : { + "numOps" : 100000, + "numThreads" : 32, + "numKeys" : 1000000, + + "keySizeRange" : [1, 8, 64], + "keySizeRangeProbability" : [0.3, 0.7], + + "valSizeRange" : [1, 32, 10240, 409200], + "valSizeRangeProbability" : [0.1, 0.2, 0.7], + + "getRatio" : 0.15, + "setRatio" : 0.8, + "delRatio" : 0.05, + "keyPoolDistribution": [0.4, 0.6], + "opPoolDistribution" : [0.5, 0.5] + } +} diff --git a/cachelib/cachebench/util/CacheConfig.cpp b/cachelib/cachebench/util/CacheConfig.cpp index 90ab4dd94c..2604744bd9 100644 --- a/cachelib/cachebench/util/CacheConfig.cpp +++ b/cachelib/cachebench/util/CacheConfig.cpp @@ -93,10 +93,18 @@ CacheConfig::CacheConfig(const folly::dynamic& configJson) { JSONSetVal(configJson, enableItemDestructorCheck); JSONSetVal(configJson, enableItemDestructor); + JSONSetVal(configJson, persistedCacheDir); + JSONSetVal(configJson, usePosixShm); + if (configJson.count("memoryTiers")) { + for (auto& it : configJson["memoryTiers"]) { + memoryTierConfigs.push_back(MemoryTierConfig(it).getMemoryTierCacheConfig()); + } + } + // if you added new fields to the configuration, update the JSONSetVal // to make them available for the json configs and increment the size // below - checkCorrectSize(); + checkCorrectSize(); if (numPools != poolSizes.size()) { throw std::invalid_argument(folly::sformat( @@ -125,6 +133,16 @@ std::shared_ptr CacheConfig::getRebalanceStrategy() const { RandomStrategy::Config{static_cast(rebalanceMinSlabs)}); } } + + +MemoryTierConfig::MemoryTierConfig(const folly::dynamic& configJson) { + JSONSetVal(configJson, file); + JSONSetVal(configJson, ratio); + JSONSetVal(configJson, size); + + checkCorrectSize(); +} + } // namespace cachebench } // namespace cachelib } // namespace facebook diff --git a/cachelib/cachebench/util/CacheConfig.h b/cachelib/cachebench/util/CacheConfig.h index e75880d879..c716de0eac 100644 --- a/cachelib/cachebench/util/CacheConfig.h +++ b/cachelib/cachebench/util/CacheConfig.h @@ -41,6 +41,23 @@ class CacheMonitorFactory { virtual std::unique_ptr create(Lru2QAllocator& cache) = 0; }; +struct MemoryTierConfig : public JSONConfig { + MemoryTierConfig() {} + + explicit MemoryTierConfig(const folly::dynamic& configJson); + MemoryTierCacheConfig getMemoryTierCacheConfig() { + if (file.empty()) { + throw std::invalid_argument("Please specify valid path to memory mapped file."); + } + MemoryTierCacheConfig config = MemoryTierCacheConfig::fromFile(file).setSize(size).setRatio(ratio); + return config; + } + + std::string file{""}; + size_t ratio{0}; + size_t size{0}; +}; + struct CacheConfig : public JSONConfig { // by defaullt, lru allocator. can be set to LRU-2Q. std::string allocator{"LRU"}; @@ -194,6 +211,13 @@ struct CacheConfig : public JSONConfig { // Not used when its value is 0. In seconds. uint32_t memoryOnlyTTL{0}; + // Directory for the cache to enable persistence across restarts. + std::string persistedCacheDir{""}; + + bool usePosixShm{false}; + + std::vector memoryTierConfigs{}; + // If enabled, we will use nvm admission policy tuned for ML use cases std::string mlNvmAdmissionPolicy{""}; From a2834aff71bbbdcc717feccae5cf55b98daa20cf Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Tue, 23 Nov 2021 00:30:09 -0500 Subject: [PATCH 13/13] Simplify CacheAllocatorConfig implementation for memory tiers Revert some of the changes made earlier and reimplement them without chaning getMemoryTierConfigs(). After this change, it is no longer possible to call setCacheSize() and configureMemoryTiers() in arbitrary order - setCacheSize() must be called first. --- cachelib/allocator/CacheAllocatorConfig.h | 202 ++++++++----------- cachelib/allocator/tests/MemoryTiersTest.cpp | 12 +- 2 files changed, 95 insertions(+), 119 deletions(-) diff --git a/cachelib/allocator/CacheAllocatorConfig.h b/cachelib/allocator/CacheAllocatorConfig.h index cb578717cb..72026635b8 100644 --- a/cachelib/allocator/CacheAllocatorConfig.h +++ b/cachelib/allocator/CacheAllocatorConfig.h @@ -62,6 +62,8 @@ class CacheAllocatorConfig { // then we will enable full coredump. Otherwise, we will disable it. The // reason we disable full coredump for large cache is because it takes a // long time to dump, and also we might not have enough local storage. + // @throw std::invalid argument if called after configuring multiple memory + // tiers CacheAllocatorConfig& setCacheSize(size_t _size); // Set default allocation sizes for a cache pool @@ -200,13 +202,10 @@ class CacheAllocatorConfig { // Configures cache memory tiers. Accepts vector of MemoryTierCacheConfig. // Each vector element describes configuration for a single memory cache tier. - // @throw std::invalid_argument if: - // - the size of configs is 0 - // - memory tiers use both size and ratio parameters CacheAllocatorConfig& configureMemoryTiers(const MemoryTierConfigs& configs); - // Return vector of memory tier configs. - MemoryTierConfigs getMemoryTierConfigs() const; + // Return reference to MemoryTierCacheConfigs. + const MemoryTierConfigs& getMemoryTierConfigs(); // This turns on a background worker that periodically scans through the // access container and look for expired items and remove them. @@ -337,7 +336,7 @@ class CacheAllocatorConfig { const std::string& getCacheName() const noexcept { return cacheName; } - size_t getCacheSize() const noexcept; + size_t getCacheSize() const noexcept { return size; } bool isUsingPosixShm() const noexcept { return usePosixShm; } @@ -555,16 +554,14 @@ class CacheAllocatorConfig { // cache. uint64_t nvmAdmissionMinTTL{0}; + // Configuration for memory tiers. + MemoryTierConfigs memoryTierConfigs{MemoryTierCacheConfig::fromShm()}; + friend CacheT; private: - void validateMemoryTiersWithSize(const MemoryTierConfigs&, size_t) const; - - // Configuration for memory tiers. - MemoryTierConfigs memoryTierConfigs{ - {MemoryTierCacheConfig::fromShm().setRatio(1)} - }; - + CacheAllocatorConfig& setUsePosixForTiers(); + CacheAllocatorConfig& setCacheSizeImpl(size_t _size); void mergeWithPrefix( std::map& configMap, const std::map& configMapToMerge, @@ -582,9 +579,8 @@ CacheAllocatorConfig& CacheAllocatorConfig::setCacheName( } template -CacheAllocatorConfig& CacheAllocatorConfig::setCacheSize(size_t _size) { - validateMemoryTiersWithSize(this->memoryTierConfigs, _size); - +CacheAllocatorConfig& CacheAllocatorConfig::setCacheSizeImpl( + size_t _size) { size = _size; constexpr size_t maxCacheSizeWithCoredump = 64'424'509'440; // 60GB if (size <= maxCacheSizeWithCoredump) { @@ -593,6 +589,17 @@ CacheAllocatorConfig& CacheAllocatorConfig::setCacheSize(size_t _size) { return *this; } +template +CacheAllocatorConfig& CacheAllocatorConfig::setCacheSize(size_t _size) { + if (memoryTierConfigs.size() == 1) { + memoryTierConfigs[0].setSize(size); + } else { + throw std::invalid_argument("Cannot set cache size after configuring memory tiers."); + } + + return setCacheSizeImpl(_size); +} + template CacheAllocatorConfig& CacheAllocatorConfig::setDefaultAllocSizes( std::set allocSizes) { @@ -813,7 +820,8 @@ CacheAllocatorConfig& CacheAllocatorConfig::usePosixForShm() { "Posix shm can be set only when cache persistence is enabled"); } usePosixShm = true; - return *this; + + return setUsePosixForTiers(); } template @@ -824,65 +832,88 @@ CacheAllocatorConfig& CacheAllocatorConfig::enableItemReaperInBackground( return *this; } +template +CacheAllocatorConfig& CacheAllocatorConfig::setUsePosixForTiers() { + for (auto &tier_config: memoryTierConfigs) { + if (auto *v = std::get_if(&tier_config.shmOpts)) { + v->usePosix = usePosixShm; + } + } + + return *this; +} + template CacheAllocatorConfig& CacheAllocatorConfig::configureMemoryTiers( const MemoryTierConfigs& config) { - if (!config.size()) { - throw std::invalid_argument("There must be at least one memory tier."); - } + memoryTierConfigs = config; + size_t sum_ratios = 0; + size_t sum_sizes = 0; - for (auto tier_config: config) { + for (const auto &tier_config: memoryTierConfigs) { auto tier_size = tier_config.getSize(); auto tier_ratio = tier_config.getRatio(); if ((!tier_size and !tier_ratio) || (tier_size and tier_ratio)) { throw std::invalid_argument( "For each memory tier either size or ratio must be set."); } + sum_ratios += tier_ratio; + sum_sizes += tier_size; } - validateMemoryTiersWithSize(config, this->size); - - memoryTierConfigs = config; - - return *this; -} - -template -typename CacheAllocatorConfig::MemoryTierConfigs -CacheAllocatorConfig::getMemoryTierConfigs() const { - MemoryTierConfigs config = memoryTierConfigs; - size_t sum_ratios = 0; + if (sum_ratios && !getCacheSize()) { + throw std::invalid_argument( + "Total cache size must be specified when size ratios are" + "used to specify memory tier sizes."); + } - for (auto &tier_config: config) { - if (auto *v = std::get_if(&tier_config.shmOpts)) { - v->usePosix = usePosixShm; + if (sum_ratios) { + if (!getCacheSize()) { + throw std::invalid_argument( + "Total cache size must be specified when size ratios are" + "used to specify memory tier sizes."); + } else { + if (getCacheSize() < sum_ratios) { + throw std::invalid_argument( + "Sum of all tier size ratios is greater than total cache size."); + } + // Convert ratios to sizes + sum_sizes = 0; + size_t partition_size = getCacheSize() / sum_ratios; + for (auto& tier_config: memoryTierConfigs) { + tier_config.setSize(partition_size * tier_config.getRatio()); + sum_sizes += tier_config.getSize(); + } + if (getCacheSize() != sum_sizes) { + // Adjust capacity of the last tier to account for rounding error + memoryTierConfigs.back().setSize(memoryTierConfigs.back().getSize() + \ + (getCacheSize() - sum_sizes)); + sum_sizes = getCacheSize(); + } } - - sum_ratios += tier_config.getRatio(); + } else if (sum_sizes) { + if (getCacheSize() && sum_sizes != getCacheSize()) { + throw std::invalid_argument( + "Sum of tier sizes doesn't match total cache size." + "Setting of cache total size is not required when per-tier" + "sizes are specified - it is calculated as sum of tier sizes."); + } + } else { + throw std::invalid_argument( + "Either sum of all memory tiers sizes or sum of all ratios" + "must be greater than 0."); } - if (sum_ratios == 0) - return config; - - // if ratios are used, size must be specified - XDCHECK(size); - - // Convert ratios to sizes, size must be non-zero - size_t sum_sizes = 0; - size_t partition_size = size / sum_ratios; - for (auto& tier_config: config) { - tier_config.setSize(partition_size * tier_config.getRatio()); - tier_config.setRatio(0); - sum_sizes += tier_config.getSize(); + if (sum_sizes && !getCacheSize()) { + setCacheSizeImpl(sum_sizes); } - if (size != sum_sizes) { - // Adjust capacity of the last tier to account for rounding error - config.back().setSize( - config.back().getSize() + (getCacheSize() - sum_sizes)); - } + return setUsePosixForTiers(); +} - return config; +template +const typename CacheAllocatorConfig::MemoryTierConfigs& CacheAllocatorConfig::getMemoryTierConfigs() { + return memoryTierConfigs; } template @@ -1000,46 +1031,6 @@ CacheAllocatorConfig& CacheAllocatorConfig::setNvmAdmissionMinTTL( return *this; } -template -size_t CacheAllocatorConfig::getCacheSize() const noexcept { - if (size) - return size; - - size_t sum_sizes = 0; - for (const auto &tier_config : getMemoryTierConfigs()) { - sum_sizes += tier_config.getSize(); - } - - return sum_sizes; -} - -template -void CacheAllocatorConfig::validateMemoryTiersWithSize( - const MemoryTierConfigs &config, size_t size) const { - size_t sum_ratios = 0; - size_t sum_sizes = 0; - - for (const auto &tier_config: config) { - sum_ratios += tier_config.getRatio(); - sum_sizes += tier_config.getSize(); - } - - if (sum_ratios && sum_sizes) { - throw std::invalid_argument("Cannot mix ratios and sizes."); - } else if (sum_sizes) { - if (size && sum_sizes != size) { - throw std::invalid_argument( - "Sum of tier sizes doesn't match total cache size. " - "Setting of cache total size is not required when per-tier " - "sizes are specified - it is calculated as sum of tier sizes."); - } - } else if (!sum_ratios && !sum_sizes) { - throw std::invalid_argument( - "Either sum of all memory tiers sizes or sum of all ratios " - "must be greater than 0."); - } -} - template const CacheAllocatorConfig& CacheAllocatorConfig::validate() const { // we can track tail hits only if MMType is MM2Q @@ -1061,23 +1052,6 @@ const CacheAllocatorConfig& CacheAllocatorConfig::validate() const { size, maxCacheSize)); } - - size_t sum_ratios = 0; - for (auto tier_config: memoryTierConfigs) { - sum_ratios += tier_config.getRatio(); - } - - if (sum_ratios) { - if (!size) { - throw std::invalid_argument( - "Total cache size must be specified when size ratios are " - "used to specify memory tier sizes."); - } else if (size < sum_ratios) { - throw std::invalid_argument( - "Sum of all tier size ratios is greater than total cache size."); - } - } - return *this; } diff --git a/cachelib/allocator/tests/MemoryTiersTest.cpp b/cachelib/allocator/tests/MemoryTiersTest.cpp index 6e5616fcdb..ed750f84d1 100644 --- a/cachelib/allocator/tests/MemoryTiersTest.cpp +++ b/cachelib/allocator/tests/MemoryTiersTest.cpp @@ -115,8 +115,10 @@ TEST_F(LruMemoryTiersTest, TestValid1TierDaxSizeConfig) { /* cacheSize */ 0); basicCheck(cfg, {defaultDaxPath}); - // Setting size after conifguringMemoryTiers with sizes is not allowed. - EXPECT_THROW(cfg.setCacheSize(defaultTotalCacheSize + 1), std::invalid_argument); + // Setting size after conifguringMemoryTiers with sizes is allowed if there is only + // a single tier + cfg.setCacheSize(defaultTotalCacheSize + 1); + EXPECT_EQ(cfg.getCacheSize(), defaultTotalCacheSize + 1); } TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemConfig) { @@ -138,7 +140,7 @@ TEST_F(LruMemoryTiersTest, TestValid2TierDaxPmemSizeConfig) { true, 0); basicCheck(cfg, {defaultDaxPath, defaultPmemPath}, size_1 + size_2); - // Setting size after conifguringMemoryTiers with sizes is not allowed. + // Setting size after conifguringMemoryTiers with mutiple tiers is not allowed EXPECT_THROW(cfg.setCacheSize(size_1 + size_2 + 1), std::invalid_argument); } @@ -150,7 +152,7 @@ TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigPosixShmNotSet) { TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigNumberOfPartitionsTooLarge) { EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, - {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}).validate(), + {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}), std::invalid_argument); } @@ -172,7 +174,7 @@ TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigSizesAndRatioNotSet) { TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigRatiosCacheSizeNotSet) { EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath}, {std::make_tuple(1, 0), std::make_tuple(1, 0)}, - /* setPosixShm */ true, /* cacheSize */ 0).validate(), + /* setPosixShm */ true, /* cacheSize */ 0), std::invalid_argument); }