Replace popen('uname -ap') with uname() syscall in bvar kernel_version by weim0000 · Pull Request #3517 · apache/brpc

weim0000 · 2026-09-03T13:30:41Z

What problem does this PR solve?

Problem Summary:

When a brpc server has allocated a large amount of memory, the first request to the /vars endpoint can cause a significant latency stall. This is because the kernel_version bvar variable is lazily initialized on first access, and its constructor calls popen("uname -ap") to read the kernel version.

Internally, popen() calls fork() 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 /vars request.

This caused a production incident in our environment, where the service appeared to hang when the monitoring system first scraped the /vars endpoint 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 via popen → fork → exec) with the POSIX uname() syscall in src/bvar/default_variables.cpp. The uname() syscall reads the same kernel information directly via struct utsname, without creating any child process. The output format remains equivalent to uname -ap. A unit test is added in test/bvar_variable_unittest.cpp.

Side effects:

Performance effects: Eliminates the fork() overhead entirely. The uname() syscall completes in microseconds regardless of the process's memory usage, whereas the previous popen() approach could stall for 100ms+ on large-memory processes.
Breaking backward compatibility: No. The output format of the kernel_version bvar remains the same.

Copilot

🟡 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") with uname()-based string construction for kernel_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>

Copilot

🟡 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 the kernel_version bvar actually uses this formatting (or that its output stayed stable). Consider asserting bvar::Variable::describe_exposed("kernel_version") matches the expected string built from utsname so 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';


wwbmmm

Good fix that removes the fork()/popen stall on the /vars hot path; however the output is not strictly equivalent to uname -ap, so the no-compatibility-break claim needs to be corrected or the format adjusted.

🤖 This reply was automatically generated by brpc-oncall

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

+        struct utsname buf;
+        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version";
            return;
        }


+TEST_F(VariableTest, uname_returns_valid_kernel_info) {
+    struct utsname buf;
+    ASSERT_EQ(0, uname(&buf));


+    // Build the string the same way ReadVersion does in default_variables.cpp
+#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;
+#if !defined(__APPLE__)
+    oss << " GNU/Linux";
+#endif
+    oss << '\n';


+    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);


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

+        oss << buf.sysname << ' ' << buf.nodename << ' '
+            << buf.release << ' ' << buf.version << ' '
+            << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)


+    // Build the string the same way ReadVersion does in default_variables.cpp
+#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;
+#if !defined(__APPLE__)
+    oss << " GNU/Linux";
+#endif
+    oss << '\n';
+    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);
+    ASSERT_NE(content.find(buf.machine), std::string::npos);
+
+    // On Linux, sysname should be "Linux"; on macOS, "Darwin"
+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");


+        struct utsname buf;
+        if (uname(&buf) != 0) {
            LOG(ERROR) << "Fail to read kernel version";
            return;
        }


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

There is at least one concrete build/correctness issue (missing header for strerror) and a compatibility-risky formatting decision that should be resolved/clarified before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 3
Review effort level: Lite

 #include <unistd.h>                        // getpagesize
 #include <sys/types.h>
 #include <sys/resource.h>                  // getrusage
+#include <sys/utsname.h>                   // uname


+#else
+        const char* processor = buf.machine;
+#endif
+        const char* hardware_platform = buf.machine;
+        std::ostringstream oss;


    ASSERT_TRUE(destructed.load());
 }
+
+TEST_F(VariableTest, uname_returns_valid_kernel_info) {


wwbmmm · 2026-09-07T13:18:39Z

LGTM. The header-only make_kernel_version_string helper is shared by production and tests, the GNU/Linux suffix is correctly gated to Linux (non-Android), the failure log now carries errno, and my earlier unused-variable concern is resolved. No remaining blockers.

🤖 This reply was automatically generated by brpc-oncall

Copilot

🟡 Changes recommended

The new unit test’s Linux-only assertions will fail on Android builds and the newly introduced installed header/function expands the public API surface in a way that likely needs adjustment/confirmation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Lite

+        if (uname(&buf) != 0) {
+            LOG(ERROR) << "Fail to read kernel version, errno=" << errno
+                       << " (" << strerror(errno) << ")";
            return;
        }


+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");
+    ASSERT_NE(content.find("GNU/Linux"), std::string::npos);


+// from unit-test binaries (see BVAR_NOT_LINK_DEFAULT_VARIABLES in
+// variable.cpp), so keeping the formatting logic here lets tests exercise the
+// exact production formatter without depending on that object being linked.
+inline std::string make_kernel_version_string(const struct utsname& buf) {


Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");
+    ASSERT_NE(content.find("GNU/Linux"), std::string::npos);
+#elif defined(__APPLE__)
+    ASSERT_STREQ(buf.sysname, "Darwin");
+    ASSERT_EQ(content.find("GNU/Linux"), std::string::npos);
+#endif


+        struct utsname buf;
+        if (uname(&buf) != 0) {
+            LOG(ERROR) << "Fail to read kernel version, errno=" << errno
+                       << " (" << strerror(errno) << ")";
            return;
        }


+namespace bvar {
+
+// Build the value of the `kernel_version` bvar from a uname(2) result.
+// The field layout mirrors `uname -ap` on the major platforms:
+//   Linux : sysname nodename release version machine processor machine GNU/Linux
+//   macOS : sysname nodename release version machine processor
+//
+// This is intentionally a header-only helper so that it is shared by both
+// default_variables.cpp and the unit tests. default_variables.o is stripped
+// from unit-test binaries (see BVAR_NOT_LINK_DEFAULT_VARIABLES in
+// variable.cpp), so keeping the formatting logic here lets tests exercise the
+// exact production formatter without depending on that object being linked.
+inline std::string make_kernel_version_string(const struct utsname& buf) {


-            LOG(ERROR) << "Fail to read kernel version";
+        struct utsname buf;
+        if (uname(&buf) != 0) {
+            LOG(ERROR) << "Fail to read kernel version, errno=" << errno


Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

+#if defined(__linux__)
+    ASSERT_STREQ(buf.sysname, "Linux");
+    ASSERT_NE(content.find("GNU/Linux"), std::string::npos);
+#elif defined(__APPLE__)
+    ASSERT_STREQ(buf.sysname, "Darwin");
+    ASSERT_EQ(content.find("GNU/Linux"), std::string::npos);
+#endif


+        struct utsname buf;
+        if (uname(&buf) != 0) {
+            LOG(ERROR) << "Fail to read kernel version, errno=" << errno
+                       << " (" << strerror(errno) << ")";
            return;
        }


+// Build the value of the `kernel_version` bvar from a uname(2) result.
+// The field layout mirrors `uname -ap` on the major platforms:
+//   Linux : sysname nodename release version machine processor machine GNU/Linux
+//   macOS : sysname nodename release version machine processor


+// from unit-test binaries (see BVAR_NOT_LINK_DEFAULT_VARIABLES in
+// variable.cpp), so keeping the formatting logic here lets tests exercise the
+// exact production formatter without depending on that object being linked.
+inline std::string make_kernel_version_string(const struct utsname& buf) {


+    std::ostringstream oss;
+    oss << buf.sysname << ' ' << buf.nodename << ' '
+        << buf.release << ' ' << buf.version << ' '
+        << buf.machine << ' ' << processor;
+#if defined(__linux__) && !defined(__ANDROID__)
+    // `uname -a` appends the hardware platform and the operating-system
+    // identifier on Linux; the hardware platform equals `machine` here.
+    oss << ' ' << buf.machine << " GNU/Linux";
+#endif


Replace popen('uname -ap') with uname() syscall in bvar kernel_version

7b6d678

wwbmmm requested a lite review from Copilot September 5, 2026 07:20

Copilot started reviewing on behalf of wwbmmm September 5, 2026 07:20 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

weim0000 and others added 3 commits September 5, 2026 20:43

Update processor variable assignment for Apple platforms

3c83555

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

update unittest add <string.h>

4877659

update unittest for bvar_variable kernel_version

4f67f87

wwbmmm requested a lite review from Copilot September 5, 2026 13:28

Copilot started reviewing on behalf of wwbmmm September 5, 2026 13:29 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Fix kernel_version to match uname -ap output format across platforms

432abde

wwbmmm reviewed Sep 6, 2026

View reviewed changes

Comment thread src/bvar/default_variables.cpp Outdated

weim0000 commented Sep 6, 2026

View reviewed changes

Comment thread src/bvar/default_variables.cpp Outdated

wwbmmm requested a lite review from Copilot September 7, 2026 09:16

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Copilot started reviewing on behalf of wwbmmm September 7, 2026 09:41 View session

Add hardware_platform to system information output

0cb601b

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

wwbmmm requested a lite review from Copilot September 7, 2026 09:51

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Copilot started reviewing on behalf of wwbmmm September 7, 2026 10:10 View session

weim0000 and others added 2 commits September 7, 2026 19:25

Refactor test to use exported bvar for kernel version

f942020

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Improve kernel_version uname() error log and Linux-only OS suffix guard

bc1b991

wwbmmm requested a lite review from Copilot September 7, 2026 12:24

Copilot started reviewing on behalf of wwbmmm September 7, 2026 12:34 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

fix unittest error

74e9332

wwbmmm requested a lite review from Copilot September 7, 2026 13:29

Copilot started reviewing on behalf of wwbmmm September 7, 2026 13:36 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

wwbmmm requested a lite review from Copilot September 7, 2026 14:33

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Copilot started reviewing on behalf of wwbmmm September 7, 2026 14:57 View session

wwbmmm requested a lite review from Copilot September 7, 2026 16:35

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Copilot started reviewing on behalf of wwbmmm September 7, 2026 16:54 View session

Navigation Menu

Uh oh!

Replace popen('uname -ap') with uname() syscall in bvar kernel_version - #3517

Replace popen('uname -ap') with uname() syscall in bvar kernel_version#3517
weim0000 wants to merge 9 commits into
apache:masterfrom
weim0000:fix_uname

weim0000 commented Sep 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

wwbmmm left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

wwbmmm commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

weim0000 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

wwbmmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

wwbmmm commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weim0000 commented Sep 3, 2026 •

edited

Loading