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

@@ -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) {

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

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 */

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
+        }
     }
 }