Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[native] Add LinuxMemoryChecker check/warning to ensure system-mem-limit-gb is reasonably set #24149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[native] Add LinuxMemoryChecker check/warning to ensure system-mem-limit-gb is reasonably set #24149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions presto-native-execution/presto_cpp/main/LinuxMemoryChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,63 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
LOG(INFO) << fmt::format("Changed to using memory stat file {}", statFile_);
}

void start() {
// Set memMaxFile to "/sys/fs/cgroup/memory/memory.limit_in_bytes" for
// cgroup v1 or "sys/fs/cgroup/memory.max" for cgroup v2.
struct stat buffer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to re-declare this as a struct here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried declaring it as stat buffer; and got error:

/root/presto/presto-native-execution/./presto_cpp/main/LinuxMemoryChecker.cpp:30:9: error: expected ‘;’ before ‘buffer’
   30 |     stat buffer;

std::string memMaxFile;
if ((stat(kCgroupV1MaxMemFilePath, &buffer) == 0)) {
memMaxFile = kCgroupV1MaxMemFilePath;
} else if ((stat(kCgroupV2MaxMemFilePath, &buffer) == 0)) {
memMaxFile = kCgroupV2MaxMemFilePath;
} else {
memMaxFile = "None";
}
LOG(INFO) << fmt::format("Checking {}", memMaxFile);

// Check system-memory-gb < system-mem-limit-gb < actual total memory
// capacity.

// For cgroup v1:
// Set actual total memory to be the smaller number between /proc/meminfo
// and memory.limit_in_bytes
int64_t actualTotalMemory = 0;
folly::gen::byLine("/proc/meminfo") |
[&](const folly::StringPiece& line) -> void {
if (actualTotalMemory == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check because we return immediately if it is set on line 79.

actualTotalMemory = static_cast<int64_t>(
extractNumericConfigValueWithRegex(line, kMemTotalRegex) * 1024);
}
if (actualTotalMemory != 0) {
return;
}
};
if (memMaxFile != "None") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to add a comment here as to what the content could be for the maxMemFile. Since we check for the "max" keyword too.

folly::gen::byLine(memMaxFile.c_str()) |
[&](const folly::StringPiece& line) -> void {
if (line == "max") {
return;
}
actualTotalMemory =
std::min(actualTotalMemory, folly::to<int64_t>(line));
return;
};
}

VELOX_CHECK_LT(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should allow less or equal.

config_.systemMemLimitBytes,
actualTotalMemory,
"system-mem-limit-gb is higher than the actual total memory capacity.");

auto* systemConfig = SystemConfig::instance();
if (config_.systemMemLimitBytes < systemConfig->systemMemoryGb()) {
LOG(WARNING) << "system-mem-limit-gb is smaller than system-memory-gb. "
<< "Expected: system-mem-limit-gb >= system-memory-gb.";
}

PeriodicMemoryChecker::start();
}

protected:
// Current memory calculation used is inactive_anon + active_anon
// Our first attempt was using memInfo memTotal - memAvailable.
Expand Down Expand Up @@ -146,6 +203,9 @@ class LinuxMemoryChecker : public PeriodicMemoryChecker {
const boost::regex kMemTotalRegex{R"!(MemTotal:\s*(\d+)\s*kB)!"};
const char* kCgroupV1Path = "/sys/fs/cgroup/memory/memory.stat";
const char* kCgroupV2Path = "/sys/fs/cgroup/memory.stat";
const char* kCgroupV1MaxMemFilePath =
"/sys/fs/cgroup/memory/memory.limit_in_bytes";
const char* kCgroupV2MaxMemFilePath = "/sys/fs/cgroup/memory.max";
std::string statFile_;

size_t extractNumericConfigValueWithRegex(
Expand Down
Loading