NUMA bindings support for Shm Segments by vinser52 · Pull Request #161 · facebook/CacheLib

vinser52 · 2022-09-13T22:56:50Z

This set of changes allows binding particular memory segments to the specified NUMA node(s).

haowu14

Thank you for making this PR. It looks good overall. Mostly just coding style nits.

therealgymmy · 2022-09-28T23:55:38Z

 }

+static void forcePageAllocation(void* addr, size_t size, size_t pageSize) {
+ for(volatile char* curAddr = (char*)addr; curAddr < (char*)addr+size; curAddr += pageSize) {


please use reinterpret_cast instead of c-style casting.

jaesoo-fb · 2022-10-31T18:01:15Z

+ return;
+ }
+
+ switch (errno) {


Do we need a wrapper for handling different error case here just to compile the error message? What would the strerror() have to say here?
Also, it seems that we don't use the any other flag except MPOL_BIND.

strerror() is context independent and too abstract.
Regarding flag variable. Today it is 0 (MPOL_BIND is a mode parameter) when we call mbindImpl(), but it is passed as a parameter to the mbindImpl() function. What if in the future we will set flag with some values?
If we assume that flag is always 0, like in current implementation, the switch block could be simplified to the following:
switch (errno) { case EFAULT: util::throwSystemError(errno); break; case EINVAL: util::throwSystemError(errno, "Invalid parameters when bind segment to NUMA node(s)"); break; case ENOMEM: util::throwSystemError(errno, "Could not bind memory. Insufficient kernel memory was available"); break; default: XDCHECK(false); util::throwSystemError(errno, "Invalid errno"); }
Should we simply the implementation for now?

This is just an error path, which is unlikely or unexpected to be hit. Would just printing errno along with strerror() just work for debugging?

Ok, I agree. I have replaced this switch block with strerror()

jaesoo-fb · 2022-10-31T18:08:33Z

+ if(opts_.memBindNumaNodes.empty()) return;
+
+ struct bitmask *oldNodeMask = numa_allocate_nodemask();
+ auto guard = folly::makeGuard([&] { numa_bitmask_free(oldNodeMask); });


You've already added NumaBitMask wrapping the struct bitmask. Why not use it instead of having this destructor? Also, I think you can override the cast operator to struct bitmask and pass the NumaBitMask instead of struct bitmask

jaesoo-fb

Only minor issues.

jaesoo-fb · 2022-11-18T01:23:53Z

+ return;
+ }
+
+ switch (errno) {


This is just an error path, which is unlikely or unexpected to be hit. Would just printing errno along with strerror() just work for debugging?

jaesoo-fb · 2022-11-18T01:30:59Z

nit. I think it will be fixed automatically when we pull those, but could you remove the trailing whitespaces and tabs?

$ git diff $(git merge-base --fork-point intern/main) --check
cachelib/cachebench/test_configs/simple_tiers_test.json:15: trailing whitespace.
+
cachelib/cachebench/test_configs/simple_tiers_test.json:23: trailing whitespace.
+
cachelib/cachebench/test_configs/simple_tiers_test.json:26: trailing whitespace.
+
cachelib/cachebench/test_configs/simple_tiers_test.json:29: trailing whitespace.
+
cachelib/cachebench/util/CacheConfig.cpp:86: trailing whitespace.
+
cachelib/shm/ShmCommon.h:103: trailing whitespace.
+
cachelib/shm/SysVShmSegment.cpp:193: trailing whitespace.
+ struct bitmask *nodesMask = static_cast<struct bitmask*>(memBindNumaNodes);
cachelib/shm/SysVShmSegment.cpp:194: trailing whitespace.
+
examples/single_tier_cache/main.cpp:83: space before tab in indent.
+ auto res = put(key, value);

jaesoo-fb

Only minor issues

jaesoo-fb

LGTM. Could you resolve Hao's comments?

facebook-github-bot · 2022-12-02T18:21:49Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-14T02:52:36Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-14T03:43:51Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-14T03:47:36Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-14T03:54:26Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jaesoo-fb · 2022-12-14T03:55:11Z

@vinser52 Hi. There are some merge conflicts encountered while importing this (cachebench/util/CacheConfig.cpp). Could you do a rebase on top of main?

jaesoo-fb

While you are doing the rebase, could you add space after for and if?

facebook-github-bot · 2022-12-15T12:40:32Z

@vinser52 has updated the pull request. You must reimport the pull request before landing.

vinser52 · 2022-12-15T12:41:39Z

@vinser52 Hi. There are some merge conflicts encountered while importing this (cachebench/util/CacheConfig.cpp). Could you do a rebase on top of main?

Done

facebook-github-bot · 2022-12-15T18:10:34Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-16T11:05:40Z

@vinser52 has updated the pull request. You must reimport the pull request before landing.

vinser52 · 2022-12-16T11:08:07Z

Hi @jaesoo-fb,

I just fixed one minor issue after the merge. Please re-import this PR again. Sorry for the inconvenience.

facebook-github-bot · 2022-12-16T20:09:46Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jaesoo-fb · 2022-12-16T23:07:30Z

Somehow, I cannot clearly see what has been changed, and also I cannot checkout the intermediate version 3b3a733. But comparing to the rebased version of bda5e07, it seems that below is the change you mentioned.

diff --git a/cachelib/cachebench/util/CacheConfig.cpp b/cachelib/cachebench/util/CacheConfig.cpp
index e08198d0..a3a742d2 100644
--- a/cachelib/cachebench/util/CacheConfig.cpp
+++ b/cachelib/cachebench/util/CacheConfig.cpp
@@ -102,7 +102,7 @@ CacheConfig::CacheConfig(const folly::dynamic& configJson) {
// 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<CacheConfig, 696>();
+ checkCorrectSize<CacheConfig, 728>();
if (numPools != poolSizes.size()) {
throw std::invalid_argument(folly::sformat(

Can you confirm?

Also, can you remove this white space?

$ git diff 519f664f --check
cachelib/cachebench/util/CacheConfig.h:49: trailing whitespace.
+

facebook-github-bot · 2022-12-16T23:22:03Z

@vinser52 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-12-16T23:23:00Z

@jaesoo-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

vinser52 · 2022-12-16T23:25:59Z

Somehow, I cannot clearly see what has been changed, and also I cannot checkout the intermediate version 3b3a733. But comparing to the rebased version of bda5e07, it seems that below is the change you mentioned.

diff --git a/cachelib/cachebench/util/CacheConfig.cpp b/cachelib/cachebench/util/CacheConfig.cpp
index e08198d0..a3a742d2 100644
--- a/cachelib/cachebench/util/CacheConfig.cpp
+++ b/cachelib/cachebench/util/CacheConfig.cpp
@@ -102,7 +102,7 @@ CacheConfig::CacheConfig(const folly::dynamic& configJson) {
// 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<CacheConfig, 696>();
+ checkCorrectSize<CacheConfig, 728>();
if (numPools != poolSizes.size()) {
throw std::invalid_argument(folly::sformat(

Can you confirm?

Also, can you remove this white space?

$ git diff 519f664f --check
cachelib/cachebench/util/CacheConfig.h:49: trailing whitespace.
+

Yeah, I confirm. After rebase checkCorrectSize<CacheConfig, 696>(); was incorrect. I beleieve you cannot see intermediate version because I do squash every time I address the comment from your side (to avoid a lot of small commits). Is it a bad practice?

Trailing whitespace in cachelib/cachebench/util/CacheConfig.h:49 is removed.

jaesoo-fb · 2022-12-16T23:29:46Z

Yeah, I confirm. After rebase checkCorrectSize<CacheConfig, 696>(); was incorrect. I beleieve you cannot see intermediate version because I do squash every time I address the comment from your side (to avoid a lot of small commits). Is it a bad practice?

No, I don't think squashing the change is the problem; I expected the git repository should have the old commit accessible until being GCed. Anyway, no worries for this.

facebook-github-bot · 2022-12-27T20:35:46Z

@jaesoo-fb merged this pull request in 26e02bf.

Summary: Fix OSS builds by adding numa deps to build files. Currently some fail on missing `numa.h`. Context: #161 added the dependencies to the centOS, debian, and ubuntu18 build files. The PR was opened in Sep 2022 but only landed in Dec 2022, and so probably missed out on the fedora, rocky and arch build files which were added in-between those dates. Having had those build actions run on PRs would have caught this (currently, they are only scheduled.) Pull Request resolved: #197 Test Plan: Github Actions builds (ideally, #198 would be landed first.) I've checked that those packages exist for the respective repositories but didn't run them myself. Reviewed By: jaesoo-fb Differential Revision: D43587970 Pulled By: jiayuebao fbshipit-source-id: 8c59e48528042350e576a45ffc3bf2520699f5a9

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 13, 2022

haowu14 requested changes Sep 21, 2022
View reviewed changes

vinser52 force-pushed the upstream_numa_bindings branch 2 times, most recently from 79bbf1a to 21b8cbbCompare September 22, 2022 19:19

therealgymmy reviewed Sep 29, 2022
View reviewed changes

vinser52 force-pushed the upstream_numa_bindings branch from 21b8cbb to 682cc63Compare September 29, 2022 12:16

vinser52 force-pushed the upstream_numa_bindings branch from 682cc63 to 9a6bbb1Compare October 24, 2022 16:58

jaesoo-fb reviewed Oct 31, 2022
View reviewed changes

vinser52 force-pushed the upstream_numa_bindings branch from 9a6bbb1 to edd034dCompare November 4, 2022 13:32

jaesoo-fb reviewed Nov 18, 2022
View reviewed changes

vinser52 force-pushed the upstream_numa_bindings branch from edd034d to 24996c4Compare November 21, 2022 15:05

jaesoo-fb reviewed Nov 28, 2022
View reviewed changes

vinser52 force-pushed the upstream_numa_bindings branch from 24996c4 to 1fbc002Compare November 30, 2022 17:44

jaesoo-fb reviewed Nov 30, 2022
View reviewed changes

Comment threadcachelib/allocator/MemoryTierCacheConfig.h Outdated
Comment threadcachelib/cachebench/test_configs/simple_tiers_test.json Outdated
Comment threadcachelib/shm/ShmCommon.h Outdated
Comment threadcachelib/shm/SysVShmSegment.cpp

vinser52 force-pushed the upstream_numa_bindings branch from 1fbc002 to bda5e07Compare December 2, 2022 17:42

jaesoo-fb reviewed Dec 14, 2022
View reviewed changes

Vinogradov, Sergei added 2 commits December 15, 2022 07:22

Implement NUMA binding support for SysVShmSegment
b6d6656

Implement NUMA binding support for PosixShmSegment
8a3cc53

vinser52 force-pushed the upstream_numa_bindings branch from bda5e07 to 3b3a733Compare December 15, 2022 12:40

vinser52 force-pushed the upstream_numa_bindings branch from 3b3a733 to e041015Compare December 16, 2022 11:05

Vinogradov, Sergei added 2 commits December 16, 2022 18:19

Enabled memory tier config API in cachebench
c97d07a

Single tier cache example with NUMA bindings
b5422cf

vinser52 force-pushed the upstream_numa_bindings branch from e041015 to b5422cfCompare December 16, 2022 23:22

facebook-github-bot closed this in 26e02bfDec 27, 2022

facebook-github-bot added the Merged label Dec 27, 2022

wonglkd mentioned this pull request Feb 24, 2023
Add missing numa deps for fedora, rocky, arch #197
Closed

wonglkd mentioned this pull request Feb 27, 2023
Run Github Actions on pull requests #198
Closed

Uh oh!

Conversation

vinser52 commented Sep 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haowu14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaesoo-fb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaesoo-fb commented Nov 18, 2022

Uh oh!

jaesoo-fb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaesoo-fb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Dec 2, 2022

Uh oh!

facebook-github-bot commented Dec 14, 2022

Uh oh!

facebook-github-bot commented Dec 14, 2022

Uh oh!

facebook-github-bot commented Dec 14, 2022

Uh oh!

facebook-github-bot commented Dec 14, 2022

Uh oh!

jaesoo-fb commented Dec 14, 2022

Uh oh!

jaesoo-fb left a comment

vinser52 commented Sep 13, 2022•
edited
Loading