From 1736fa4d220266718980676855c3d3ca54d7da44 Mon Sep 17 00:00:00 2001 From: yoav-steinberg Date: Tue, 7 Dec 2021 15:05:51 +0100 Subject: [PATCH] Don't write oom score adj to proc unless we're managing it. (#9904) When disabling redis oom-score-adj managment we restore the base value read before enabling oom-score-adj management. This fixes an issue introduced in #9748 where updating `oom-score-adj-values` while `oom-score-adj` was set to `no` would write the base oom score adj value read on startup to `/proc`. This is a bug since while `oom-score-adj` is disabled we should never write to proc and let external processes manage it. Added appropriate tests. --- src/config.c | 8 ++++-- src/server.c | 46 +++++++++++++++++++++------------ src/server.h | 1 - tests/unit/oom-score-adj.tcl | 49 ++++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 19 deletions(-) diff --git a/src/config.c b/src/config.c index 939a5342c..b73b5df0e 100644 --- a/src/config.c +++ b/src/config.c @@ -2387,6 +2387,7 @@ static sds getConfigClientOutputBufferLimitOption(typeData data) { static int setConfigOOMScoreAdjValuesOption(typeData data, sds *argv, int argc, const char **err) { int i; int values[CONFIG_OOM_COUNT]; + int change = 0; UNUSED(data); if (argc != CONFIG_OOM_COUNT) { @@ -2419,10 +2420,13 @@ static int setConfigOOMScoreAdjValuesOption(typeData data, sds *argv, int argc, } for (i = 0; i < CONFIG_OOM_COUNT; i++) { - server.oom_score_adj_values[i] = values[i]; + if (server.oom_score_adj_values[i] != values[i]) { + server.oom_score_adj_values[i] = values[i]; + change = 1; + } } - return 1; + return change ? 1 : 2; } static sds getConfigOOMScoreAdjValuesOption(typeData data) { diff --git a/src/server.c b/src/server.c index 571eb1b06..545222d5a 100644 --- a/src/server.c +++ b/src/server.c @@ -3869,18 +3869,6 @@ int restartServer(int flags, mstime_t delay) { return C_ERR; /* Never reached. */ } -static void readOOMScoreAdj(void) { -#ifdef HAVE_PROC_OOM_SCORE_ADJ - char buf[64]; - int fd = open("/proc/self/oom_score_adj", O_RDONLY); - - if (fd < 0) return; - if (read(fd, buf, sizeof(buf)) > 0) - server.oom_score_adj_base = atoi(buf); - close(fd); -#endif -} - /* This function will configure the current process's oom_score_adj according * to user specified configuration. This is currently implemented on Linux * only. @@ -3895,18 +3883,45 @@ int setOOMScoreAdj(int process_class) { serverAssert(process_class >= 0 && process_class < CONFIG_OOM_COUNT); #ifdef HAVE_PROC_OOM_SCORE_ADJ + /* The following statics are used to indicate Redis has changed the process's oom score. + * And to save the original score so we can restore it later if needed. + * We need this so when we disabled oom-score-adj (also during configuration rollback + * when another configuration parameter was invalid and causes a rollback after + * applying a new oom-score) we can return to the oom-score value from before our + * adjustments. */ + static int oom_score_adjusted_by_redis = 0; + static int oom_score_adj_base = 0; + int fd; int val; char buf[64]; if (server.oom_score_adj != OOM_SCORE_ADJ_NO) { + if (!oom_score_adjusted_by_redis) { + oom_score_adjusted_by_redis = 1; + /* Backup base value before enabling Redis control over oom score */ + fd = open("/proc/self/oom_score_adj", O_RDONLY); + if (fd < 0 || read(fd, buf, sizeof(buf)) < 0) { + serverLog(LL_WARNING, "Unable to read oom_score_adj: %s", strerror(errno)); + if (fd != -1) close(fd); + return C_ERR; + } + oom_score_adj_base = atoi(buf); + close(fd); + } + val = server.oom_score_adj_values[process_class]; if (server.oom_score_adj == OOM_SCORE_RELATIVE) - val += server.oom_score_adj_base; + val += oom_score_adj_base; if (val > 1000) val = 1000; if (val < -1000) val = -1000; - } else - val = server.oom_score_adj_base; + } else if (oom_score_adjusted_by_redis) { + oom_score_adjusted_by_redis = 0; + val = oom_score_adj_base; + } + else { + return C_OK; + } snprintf(buf, sizeof(buf) - 1, "%d\n", val); @@ -8046,7 +8061,6 @@ int main(int argc, char **argv) { serverLog(LL_WARNING, "Configuration loaded"); } - readOOMScoreAdj(); initServer(); if (background || server.pidfile) createPidFile(); if (server.set_proc_title) redisSetProcTitle(NULL); diff --git a/src/server.h b/src/server.h index 535d21dd9..b4bb96f26 100644 --- a/src/server.h +++ b/src/server.h @@ -1664,7 +1664,6 @@ struct redisServer { int lfu_log_factor; /* LFU logarithmic counter factor. */ int lfu_decay_time; /* LFU counter decay factor. */ long long proto_max_bulk_len; /* Protocol bulk length maximum size. */ - int oom_score_adj_base; /* Base oom_score_adj value, as observed on startup */ int oom_score_adj_values[CONFIG_OOM_COUNT]; /* Linux oom_score_adj configuration */ int oom_score_adj; /* If true, oom_score_adj is managed */ int disable_thp; /* If true, disable THP by syscall */ diff --git a/tests/unit/oom-score-adj.tcl b/tests/unit/oom-score-adj.tcl index 78b383e03..b226a266a 100644 --- a/tests/unit/oom-score-adj.tcl +++ b/tests/unit/oom-score-adj.tcl @@ -14,6 +14,15 @@ if {$system_name eq {linux}} { return $val } + proc set_oom_score_adj {score {pid ""}} { + if {$pid == ""} { + set pid [srv 0 pid] + } + set fd [open "/proc/$pid/oom_score_adj" "w"] + puts $fd $score + close $fd + } + test {CONFIG SET oom-score-adj works as expected} { set base [get_oom_score_adj] @@ -73,5 +82,45 @@ if {$system_name eq {linux}} { assert {[r config get oom-score-adj-values] == {oom-score-adj-values {0 100 100}}} } } + + test {CONFIG SET oom-score-adj-values doesn't touch proc when disabled} { + set orig_osa [get_oom_score_adj] + + set other_val1 [expr $orig_osa + 1] + set other_val2 [expr $orig_osa + 2] + + r config set oom-score-adj no + + set_oom_score_adj $other_val2 + assert_equal [get_oom_score_adj] $other_val2 + + r config set oom-score-adj-values "$other_val1 $other_val1 $other_val1" + + assert_equal [get_oom_score_adj] $other_val2 + } + + test {CONFIG SET oom score restored on disable} { + r config set oom-score-adj no + set_oom_score_adj 22 + assert_equal [get_oom_score_adj] 22 + + r config set oom-score-adj-values "9 9 9" oom-score-adj yes + assert_equal [get_oom_score_adj] [expr 9+22] + + r config set oom-score-adj no + assert_equal [get_oom_score_adj] 22 + } + + test {CONFIG SET oom score relative and absolute} { + set custom_oom 9 + r config set oom-score-adj no + set base_oom [get_oom_score_adj] + + r config set oom-score-adj-values "$custom_oom $custom_oom $custom_oom" oom-score-adj relative + assert_equal [get_oom_score_adj] [expr $base_oom+$custom_oom] + + r config set oom-score-adj absolute + assert_equal [get_oom_score_adj] $custom_oom + } } }