From 61954951edbda670bfbae8be0147daa64df95f26 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 22 Nov 2020 13:57:56 +0200 Subject: [PATCH] Fix oom-score-adj-values range, abs options, and bug when used in config file (#8046) Fix: When oom-score-adj-values is provided in the config file after oom-score-adj yes, it'll take an immediate action, before readOOMScoreAdj was acquired, resulting in an error (out of range score due to uninitialized value. delay the reaction the real call is made by main(). Since the values are clamped to -1000..1000, and they're applied as an offset from the value at startup (which may be -1000), we need to allow the offsets to reach to +2000 so that a value of +1000 is achievable in case the value at startup was -1000. Adding an option for absolute values rather than relative ones. --- redis.conf | 23 ++++++++++++++--------- src/config.c | 25 +++++++++++++++++++------ src/server.c | 6 ++++-- src/server.h | 5 +++++ 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/redis.conf b/redis.conf index dcb5f6241..b1c8e4f0c 100644 --- a/redis.conf +++ b/redis.conf @@ -1086,21 +1086,26 @@ lazyfree-lazy-user-del no # for all its processes, depending on their role. The default scores will # attempt to have background child processes killed before all others, and # replicas killed before masters. - +# +# Redis supports three options: +# +# no: Don't make changes to oom-score-adj (default). +# yes: Alias to "relative" see below. +# absolute: Values in oom-score-adj-values are written as is to the kernel. +# relative: Values are used relative to the initial value of oom_score_adj when +# the server starts and are then clamped to a range of -1000 to 1000. +# Because typically the initial value is 0, they will often match the +# absolute values. oom-score-adj no # When oom-score-adj is used, this directive controls the specific values used -# for master, replica and background child processes. Values range -1000 to -# 1000 (higher means more likely to be killed). +# for master, replica and background child processes. Values range -2000 to +# 2000 (higher means more likely to be killed). # # Unprivileged processes (not root, and without CAP_SYS_RESOURCE capabilities) # can freely increase their value, but not decrease it below its initial -# settings. -# -# Values are used relative to the initial value of oom_score_adj when the server -# starts. Because typically the initial value is 0, they will often match the -# absolute values. - +# settings. This means that setting oom-score-adj to "relative" and setting the +# oom-score-adj-values to positive values will always succeed. oom-score-adj-values 0 200 800 diff --git a/src/config.c b/src/config.c index 07ed91e5f..b4fd9bee9 100644 --- a/src/config.c +++ b/src/config.c @@ -104,6 +104,15 @@ configEnum tls_auth_clients_enum[] = { {"optional", TLS_CLIENT_AUTH_OPTIONAL}, {NULL, 0} }; + +configEnum oom_score_adj_enum[] = { + {"no", OOM_SCORE_ADJ_NO}, + {"yes", OOM_SCORE_RELATIVE}, + {"relative", OOM_SCORE_RELATIVE}, + {"absolute", OOM_SCORE_ADJ_ABSOLUTE}, + {NULL, 0} +}; + /* Output buffer limits presets. */ clientBufferLimitsConfig clientBufferLimitsDefaults[CLIENT_TYPE_OBUF_COUNT] = { {0, 0, 0}, /* normal */ @@ -293,7 +302,7 @@ void queueLoadModule(sds path, sds *argv, int argc) { * server.oom_score_adj_values if valid. */ -static int updateOOMScoreAdjValues(sds *args, char **err) { +static int updateOOMScoreAdjValues(sds *args, char **err, int apply) { int i; int values[CONFIG_OOM_COUNT]; @@ -301,8 +310,8 @@ static int updateOOMScoreAdjValues(sds *args, char **err) { char *eptr; long long val = strtoll(args[i], &eptr, 10); - if (*eptr != '\0' || val < -1000 || val > 1000) { - if (err) *err = "Invalid oom-score-adj-values, elements must be between -1000 and 1000."; + if (*eptr != '\0' || val < -2000 || val > 2000) { + if (err) *err = "Invalid oom-score-adj-values, elements must be between -2000 and 2000."; return C_ERR; } @@ -326,6 +335,10 @@ static int updateOOMScoreAdjValues(sds *args, char **err) { old_values[i] = server.oom_score_adj_values[i]; server.oom_score_adj_values[i] = values[i]; } + + /* When parsing the config file, we want to apply only when all is done. */ + if (!apply) + return C_OK; /* Update */ if (setOOMScoreAdj(-1) == C_ERR) { @@ -559,7 +572,7 @@ void loadServerConfigFromString(char *config) { server.client_obuf_limits[class].soft_limit_bytes = soft; server.client_obuf_limits[class].soft_limit_seconds = soft_seconds; } else if (!strcasecmp(argv[0],"oom-score-adj-values") && argc == 1 + CONFIG_OOM_COUNT) { - if (updateOOMScoreAdjValues(&argv[1], &err) == C_ERR) goto loaderr; + if (updateOOMScoreAdjValues(&argv[1], &err, 0) == C_ERR) goto loaderr; } else if (!strcasecmp(argv[0],"notify-keyspace-events") && argc == 2) { int flags = keyspaceEventsStringToFlags(argv[1]); @@ -822,7 +835,7 @@ void configSetCommand(client *c) { int success = 1; sds *v = sdssplitlen(o->ptr, sdslen(o->ptr), " ", 1, &vlen); - if (vlen != CONFIG_OOM_COUNT || updateOOMScoreAdjValues(v, &errstr) == C_ERR) + if (vlen != CONFIG_OOM_COUNT || updateOOMScoreAdjValues(v, &errstr, 1) == C_ERR) success = 0; sdsfreesplitres(v, vlen); @@ -2313,7 +2326,6 @@ standardConfig configs[] = { createBoolConfig("crash-log-enabled", NULL, MODIFIABLE_CONFIG, server.crashlog_enabled, 1, NULL, updateSighandlerEnabled), createBoolConfig("crash-memcheck-enabled", NULL, MODIFIABLE_CONFIG, server.memcheck_enabled, 1, NULL, NULL), createBoolConfig("use-exit-on-panic", NULL, MODIFIABLE_CONFIG, server.use_exit_on_panic, 0, NULL, NULL), - createBoolConfig("oom-score-adj", NULL, MODIFIABLE_CONFIG, server.oom_score_adj, 0, NULL, updateOOMScoreAdj), createBoolConfig("disable-thp", NULL, MODIFIABLE_CONFIG, server.disable_thp, 1, NULL, NULL), /* String Configs */ @@ -2339,6 +2351,7 @@ standardConfig configs[] = { createEnumConfig("loglevel", NULL, MODIFIABLE_CONFIG, loglevel_enum, server.verbosity, LL_NOTICE, NULL, NULL), createEnumConfig("maxmemory-policy", NULL, MODIFIABLE_CONFIG, maxmemory_policy_enum, server.maxmemory_policy, MAXMEMORY_NO_EVICTION, NULL, NULL), createEnumConfig("appendfsync", NULL, MODIFIABLE_CONFIG, aof_fsync_enum, server.aof_fsync, AOF_FSYNC_EVERYSEC, NULL, NULL), + createEnumConfig("oom-score-adj", NULL, MODIFIABLE_CONFIG, oom_score_adj_enum, server.oom_score_adj, OOM_SCORE_ADJ_NO, NULL, updateOOMScoreAdj), /* Integer configs */ createIntConfig("databases", NULL, IMMUTABLE_CONFIG, 1, INT_MAX, server.dbnum, 16, INTEGER_CONFIG, NULL, NULL), diff --git a/src/server.c b/src/server.c index ac5b30e90..3c1f1cb6b 100644 --- a/src/server.c +++ b/src/server.c @@ -2667,7 +2667,7 @@ static void readOOMScoreAdj(void) { */ int setOOMScoreAdj(int process_class) { - if (!server.oom_score_adj) return C_OK; + if (server.oom_score_adj == OOM_SCORE_ADJ_NO) return C_OK; if (process_class == -1) process_class = (server.masterhost ? CONFIG_OOM_REPLICA : CONFIG_OOM_MASTER); @@ -2678,7 +2678,9 @@ int setOOMScoreAdj(int process_class) { int val; char buf[64]; - val = server.oom_score_adj_base + server.oom_score_adj_values[process_class]; + val = server.oom_score_adj_values[process_class]; + if (server.oom_score_adj == OOM_SCORE_RELATIVE) + val += server.oom_score_adj_base; if (val > 1000) val = 1000; if (val < -1000) val = -1000; diff --git a/src/server.h b/src/server.h index 53055897a..4690151c0 100644 --- a/src/server.h +++ b/src/server.h @@ -382,6 +382,11 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; #define SET_OP_DIFF 1 #define SET_OP_INTER 2 +/* oom-score-adj defines */ +#define OOM_SCORE_ADJ_NO 0 +#define OOM_SCORE_RELATIVE 1 +#define OOM_SCORE_ADJ_ABSOLUTE 2 + /* Redis maxmemory strategies. Instead of using just incremental number * for this defines, we use a set of flags so that testing for certain * properties common to multiple policies is faster. */