Replace popen('uname -ap') with uname() syscall in bvar kernel_version - #3517
Replace popen('uname -ap') with uname() syscall in bvar kernel_version#3517weim0000 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new kernel_version formatting is observably different from uname -ap (newline + processor field) and the added test introduces a missing-header build risk while not exercising the actual bvar path end-to-end.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes the use of popen("uname -ap") (and its fork() overhead) from the lazy initialization of the kernel_version bvar by switching to the uname() syscall, aiming to eliminate first-hit /vars latency spikes in large-memory processes.
Changes:
- Replace
butil::read_command_output(..., "uname -ap")withuname()-based string construction forkernel_version. - Add a unit test intended to validate the
uname()-derived kernel info formatting.
File summaries
| File | Description |
|---|---|
| src/bvar/default_variables.cpp | Switch kernel_version collection from shelling out to uname() syscall. |
| test/bvar_variable_unittest.cpp | Add a new test around uname() output/format assumptions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <pthread.h> // pthread_* | ||
| #include <unistd.h> // usleep | ||
| #include <sys/utsname.h> // uname | ||
|
|
| TEST_F(VariableTest, uname_returns_valid_kernel_info) { | ||
| struct utsname buf; | ||
| ASSERT_EQ(0, uname(&buf)); | ||
|
|
||
| // Each field should be non-empty | ||
| ASSERT_GT(strlen(buf.sysname), 0u); | ||
| ASSERT_GT(strlen(buf.nodename), 0u); | ||
| ASSERT_GT(strlen(buf.release), 0u); | ||
| ASSERT_GT(strlen(buf.version), 0u); | ||
| ASSERT_GT(strlen(buf.machine), 0u); | ||
|
|
||
| // Build the string the same way ReadVersion does in default_variables.cpp | ||
| std::ostringstream oss; | ||
| oss << buf.sysname << ' ' << buf.nodename << ' ' | ||
| << buf.release << ' ' << buf.version << ' ' | ||
| << buf.machine << ' ' << buf.machine; | ||
| std::string content = oss.str(); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new kernel_version formatting is not actually equivalent to uname -ap on Linux and the added test doesn’t currently assert the behavior of the kernel_version bvar itself.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/bvar_variable_unittest.cpp:494
- This new test validates that
uname()returns non-empty fields, but it does not assert that thekernel_versionbvar actually uses this formatting (or that its output stayed stable). Consider assertingbvar::Variable::describe_exposed("kernel_version")matches the expected string built fromutsnameso the test covers the behavior introduced by this PR.
std::string content = oss.str();
// The result should contain all key fields
ASSERT_NE(content.find(buf.sysname), std::string::npos);
ASSERT_NE(content.find(buf.release), std::string::npos);
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| struct utsname buf; | ||
| if (uname(&buf) != 0) { | ||
| LOG(ERROR) << "Fail to read kernel version"; | ||
| return; | ||
| } | ||
| #if defined(__APPLE__) && (defined(__aarch64__) || defined(__arm64__)) | ||
| const char* processor = "arm"; | ||
| #elif defined(__APPLE__) && defined(__x86_64__) | ||
| const char* processor = "i386"; | ||
| #else | ||
| const char* processor = buf.machine; | ||
| #endif | ||
| std::ostringstream oss; | ||
| oss << buf.sysname << ' ' << buf.nodename << ' ' | ||
| << buf.release << ' ' << buf.version << ' ' | ||
| << buf.machine << ' ' << processor << '\n'; |
What problem does this PR solve?
Problem Summary:
When a brpc server has allocated a large amount of memory, the first request to the
/varsendpoint can cause a significant latency stall. This is because thekernel_versionbvar variable is lazily initialized on first access, and its constructor callspopen("uname -ap")to read the kernel version.Internally,
popen()callsfork()to spawn a child process. On Linux,fork()needs to duplicate the parent process's page tables. For a server with a large memory footprint (e.g., tens of GBs), this can take hundreds of milliseconds or even longer, effectively blocking the bthread that handles the/varsrequest.This caused a production incident in our environment, where the service appeared to hang when the monitoring system first scraped the
/varsendpoint after the server had been running for a while with heavy memory usage.What is changed and the side effects?
Changed:
Replace
butil::read_command_output(oss, "uname -ap")(which shells out viapopen→fork→exec) with the POSIXuname()syscall insrc/bvar/default_variables.cpp. Theuname()syscall reads the same kernel information directly viastruct utsname, without creating any child process. The output format remains equivalent touname -ap. A unit test is added intest/bvar_variable_unittest.cpp.Side effects:
Performance effects: Eliminates the
fork()overhead entirely. Theuname()syscall completes in microseconds regardless of the process's memory usage, whereas the previouspopen()approach could stall for 100ms+ on large-memory processes.Breaking backward compatibility: No. The output format of the
kernel_versionbvar remains the same.