From 81d66fab342e112aefe8b5f43bc853ef66c92add Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Wed, 24 Mar 2021 02:17:04 -0700 Subject: [PATCH 1/8] oomd: split swap and mem pressure event timers One thing that came out of the test week is that systoomd needs to poll more frequently so as not to race with the kernel oom killer in situations where memory is eaten quickly. Memory pressure counters are lagging so it isn't worthwhile to change the current read rate; however swap is not lagging and can be checked more frequently. So let's split these into 2 different timer events. As a result, swap now also doesn't have to be subject to the post-action (post-kill) delay that we need for memory pressure events. Addresses some of slowness to kill discussed in https://bugzilla.redhat.com/show_bug.cgi?id=1941340 --- src/oom/oomd-manager.c | 182 ++++++++++++++++++++++++++++------------- src/oom/oomd-manager.h | 9 +- 2 files changed, 131 insertions(+), 60 deletions(-) diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index c3e84aadde..2a683d5469 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -299,7 +299,82 @@ static int acquire_managed_oom_connect(Manager *m) { return 0; } -static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, void *userdata) { +static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void *userdata) { + Manager *m = userdata; + usec_t usec_now; + int r; + + assert(s); + assert(userdata); + + /* Reset timer */ + r = sd_event_now(sd_event_source_get_event(s), CLOCK_MONOTONIC, &usec_now); + if (r < 0) + return log_error_errno(r, "Failed to reset event timer: %m"); + + r = sd_event_source_set_time_relative(s, SWAP_INTERVAL_USEC); + if (r < 0) + return log_error_errno(r, "Failed to set relative time for timer: %m"); + + /* Reconnect if our connection dropped */ + if (!m->varlink) { + r = acquire_managed_oom_connect(m); + if (r < 0) + return log_error_errno(r, "Failed to acquire varlink connection: %m"); + } + + /* We still try to acquire swap information for oomctl even if no units want swap monitoring */ + r = oomd_system_context_acquire("/proc/swaps", &m->system_context); + /* If there are no units depending on swap actions, the only error we exit on is ENOMEM. + * Allow ENOENT in the event that swap is disabled on the system. */ + if (r == -ENOENT) { + zero(m->system_context); + return 0; + } else if (r == -ENOMEM || (r < 0 && !hashmap_isempty(m->monitored_swap_cgroup_contexts))) + return log_error_errno(r, "Failed to acquire system context: %m"); + + /* Return early if nothing is requesting swap monitoring */ + if (hashmap_isempty(m->monitored_swap_cgroup_contexts)) + return 0; + + /* Note that m->monitored_swap_cgroup_contexts does not need to be updated every interval because only the + * system context is used for deciding whether the swap threshold is hit. m->monitored_swap_cgroup_contexts + * is only used to decide which cgroups to kill (and even then only the resource usages of its descendent + * nodes are the ones that matter). */ + + if (oomd_swap_free_below(&m->system_context, 10000 - m->swap_used_limit_permyriad)) { + _cleanup_hashmap_free_ Hashmap *candidates = NULL; + _cleanup_free_ char *selected = NULL; + + log_debug("Swap used (%"PRIu64") / total (%"PRIu64") is more than " PERMYRIAD_AS_PERCENT_FORMAT_STR, + m->system_context.swap_used, m->system_context.swap_total, + PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad)); + + r = get_monitored_cgroup_contexts_candidates(m->monitored_swap_cgroup_contexts, &candidates); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + log_debug_errno(r, "Failed to get monitored swap cgroup candidates, ignoring: %m"); + + r = oomd_kill_by_swap_usage(candidates, m->dry_run, &selected); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + log_notice_errno(r, "Failed to kill any cgroup(s) based on swap: %m"); + else { + if (selected) + log_notice("Killed %s due to swap used (%"PRIu64") / total (%"PRIu64") being more than " + PERMYRIAD_AS_PERCENT_FORMAT_STR, + selected, m->system_context.swap_used, m->system_context.swap_total, + PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad)); + return 0; + } + } + + return 0; +} + +static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t usec, void *userdata) { _cleanup_set_free_ Set *targets = NULL; Manager *m = userdata; usec_t usec_now; @@ -313,7 +388,7 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo if (r < 0) return log_error_errno(r, "Failed to reset event timer: %m"); - r = sd_event_source_set_time_relative(s, INTERVAL_USEC); + r = sd_event_source_set_time_relative(s, MEM_PRESSURE_INTERVAL_USEC); if (r < 0) return log_error_errno(r, "Failed to set relative time for timer: %m"); @@ -324,13 +399,13 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo return log_error_errno(r, "Failed to acquire varlink connection: %m"); } - /* Update the cgroups used for detection/action */ - r = update_monitored_cgroup_contexts(&m->monitored_swap_cgroup_contexts); - if (r == -ENOMEM) - return log_oom(); - if (r < 0) - log_debug_errno(r, "Failed to update monitored swap cgroup contexts, ignoring: %m"); + /* Return early if nothing is requesting memory pressure monitoring */ + if (hashmap_isempty(m->monitored_mem_pressure_cgroup_contexts)) { + hashmap_clear(m->monitored_mem_pressure_cgroup_contexts_candidates); + return 0; + } + /* Update the cgroups used for detection/action */ r = update_monitored_cgroup_contexts(&m->monitored_mem_pressure_cgroup_contexts); if (r == -ENOMEM) return log_oom(); @@ -344,23 +419,16 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo if (r < 0) log_debug_errno(r, "Failed to update monitored memory pressure candidate cgroup contexts, ignoring: %m"); - r = oomd_system_context_acquire("/proc/swaps", &m->system_context); - /* If there aren't units depending on swap actions, the only error we exit on is ENOMEM. - * Allow ENOENT in the event that swap is disabled on the system. */ - if (r == -ENOMEM || (r < 0 && r != -ENOENT && !hashmap_isempty(m->monitored_swap_cgroup_contexts))) - return log_error_errno(r, "Failed to acquire system context: %m"); - else if (r == -ENOENT) - zero(m->system_context); - if (oomd_memory_reclaim(m->monitored_mem_pressure_cgroup_contexts)) m->last_reclaim_at = usec_now; - /* If we're still recovering from a kill, don't try to kill again yet */ - if (m->post_action_delay_start > 0) { - if (m->post_action_delay_start + POST_ACTION_DELAY_USEC > usec_now) + /* Since pressure counters are lagging, we need to wait a bit after a kill to ensure we don't read stale + * values and go on a kill storm. */ + if (m->mem_pressure_post_action_delay_start > 0) { + if (m->mem_pressure_post_action_delay_start + POST_ACTION_DELAY_USEC > usec_now) return 0; else - m->post_action_delay_start = 0; + m->mem_pressure_post_action_delay_start = 0; } r = oomd_pressure_above(m->monitored_mem_pressure_cgroup_contexts, m->default_mem_pressure_duration_usec, &targets); @@ -396,7 +464,7 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo log_notice_errno(r, "Failed to kill any cgroup(s) under %s based on pressure: %m", t->path); else { /* Don't act on all the high pressure cgroups at once; return as soon as we kill one */ - m->post_action_delay_start = usec_now; + m->mem_pressure_post_action_delay_start = usec_now; if (selected) log_notice("Killed %s due to memory pressure for %s being %lu.%02lu%% > %lu.%02lu%%" " for > %s with reclaim activity", @@ -412,47 +480,17 @@ static int monitor_cgroup_contexts_handler(sd_event_source *s, uint64_t usec, vo } } - if (oomd_swap_free_below(&m->system_context, 10000 - m->swap_used_limit_permyriad)) { - _cleanup_hashmap_free_ Hashmap *candidates = NULL; - _cleanup_free_ char *selected = NULL; - - log_debug("Swap used (%"PRIu64") / total (%"PRIu64") is more than " PERMYRIAD_AS_PERCENT_FORMAT_STR, - m->system_context.swap_used, m->system_context.swap_total, - PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad)); - - r = get_monitored_cgroup_contexts_candidates(m->monitored_swap_cgroup_contexts, &candidates); - if (r == -ENOMEM) - return log_oom(); - if (r < 0) - log_debug_errno(r, "Failed to get monitored swap cgroup candidates, ignoring: %m"); - - r = oomd_kill_by_swap_usage(candidates, m->dry_run, &selected); - if (r == -ENOMEM) - return log_oom(); - if (r < 0) - log_notice_errno(r, "Failed to kill any cgroup(s) based on swap: %m"); - else { - m->post_action_delay_start = usec_now; - if (selected) - log_notice("Killed %s due to swap used (%"PRIu64") / total (%"PRIu64") being more than " - PERMYRIAD_AS_PERCENT_FORMAT_STR, - selected, m->system_context.swap_used, m->system_context.swap_total, - PERMYRIAD_AS_PERCENT_FORMAT_VAL(m->swap_used_limit_permyriad)); - return 0; - } - } - return 0; } -static int monitor_cgroup_contexts(Manager *m) { +static int monitor_swap_contexts(Manager *m) { _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL; int r; assert(m); assert(m->event); - r = sd_event_add_time(m->event, &s, CLOCK_MONOTONIC, 0, 0, monitor_cgroup_contexts_handler, m); + r = sd_event_add_time(m->event, &s, CLOCK_MONOTONIC, 0, 0, monitor_swap_contexts_handler, m); if (r < 0) return r; @@ -464,9 +502,34 @@ static int monitor_cgroup_contexts(Manager *m) { if (r < 0) return r; - (void) sd_event_source_set_description(s, "oomd-timer"); + (void) sd_event_source_set_description(s, "oomd-swap-timer"); - m->cgroup_context_event_source = TAKE_PTR(s); + m->swap_context_event_source = TAKE_PTR(s); + return 0; +} + +static int monitor_memory_pressure_contexts(Manager *m) { + _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL; + int r; + + assert(m); + assert(m->event); + + r = sd_event_add_time(m->event, &s, CLOCK_MONOTONIC, 0, 0, monitor_memory_pressure_contexts_handler, m); + if (r < 0) + return r; + + r = sd_event_source_set_exit_on_failure(s, true); + if (r < 0) + return r; + + r = sd_event_source_set_enabled(s, SD_EVENT_ON); + if (r < 0) + return r; + + (void) sd_event_source_set_description(s, "oomd-memory-pressure-timer"); + + m->mem_pressure_context_event_source = TAKE_PTR(s); return 0; } @@ -474,7 +537,8 @@ Manager* manager_free(Manager *m) { assert(m); varlink_close_unref(m->varlink); - sd_event_source_unref(m->cgroup_context_event_source); + sd_event_source_unref(m->swap_context_event_source); + sd_event_source_unref(m->mem_pressure_context_event_source); sd_event_unref(m->event); bus_verify_polkit_async_registry_free(m->polkit_registry); @@ -596,7 +660,11 @@ int manager_start( if (r < 0) return r; - r = monitor_cgroup_contexts(m); + r = monitor_memory_pressure_contexts(m); + if (r < 0) + return r; + + r = monitor_swap_contexts(m); if (r < 0) return r; diff --git a/src/oom/oomd-manager.h b/src/oom/oomd-manager.h index 9c580c8a24..3df7ce94d7 100644 --- a/src/oom/oomd-manager.h +++ b/src/oom/oomd-manager.h @@ -7,7 +7,9 @@ #include "varlink.h" /* Polling interval for monitoring stats */ -#define INTERVAL_USEC (1 * USEC_PER_SEC) +#define SWAP_INTERVAL_USEC 150000 /* 0.15 seconds */ +/* Pressure counters are lagging (~2 seconds) compared to swap so polling too frequently just wastes CPU */ +#define MEM_PRESSURE_INTERVAL_USEC (1 * USEC_PER_SEC) /* Used to weight the averages */ #define AVERAGE_SIZE_DECAY 4 @@ -45,9 +47,10 @@ struct Manager { OomdSystemContext system_context; usec_t last_reclaim_at; - usec_t post_action_delay_start; + usec_t mem_pressure_post_action_delay_start; - sd_event_source *cgroup_context_event_source; + sd_event_source *swap_context_event_source; + sd_event_source *mem_pressure_context_event_source; Varlink *varlink; }; From 37d8020ccc2f54260b581af416e02294c7591cfc Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Thu, 25 Mar 2021 23:54:22 -0700 Subject: [PATCH 2/8] oomd: refactor pgscan_rate calculation into helper --- src/oom/oomd-util.c | 17 +++++++++++++++++ src/oom/oomd-util.h | 24 +++++++----------------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index 894d23a83a..5133b1c060 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -134,6 +134,23 @@ bool oomd_memory_reclaim(Hashmap *h) { return pgscan_of > last_pgscan_of; } +uint64_t oomd_pgscan_rate(const OomdCGroupContext *c) { + uint64_t last_pgscan; + + assert(c); + + /* If last_pgscan > pgscan, assume the cgroup was recreated and reset last_pgscan to zero. + * pgscan is monotonic and in practice should not decrease (except in the recreation case). */ + last_pgscan = c->last_pgscan; + if (c->last_pgscan > c->pgscan) { + log_debug("Last pgscan %"PRIu64" greater than current pgscan %"PRIu64" for %s. Using last pgscan of zero.", + c->last_pgscan, c->pgscan, c->path); + last_pgscan = 0; + } + + return c->pgscan - last_pgscan; +} + bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad) { uint64_t swap_threshold; diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index 51423130d1..af08504c5b 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -64,10 +64,13 @@ bool oomd_memory_reclaim(Hashmap *h); /* Returns true if the amount of swap free is below the permyriad of swap specified by `threshold_permyriad`. */ bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad); +/* Returns pgscan - last_pgscan, accounting for corner cases. */ +uint64_t oomd_pgscan_rate(const OomdCGroupContext *c); + /* The compare functions will sort from largest to smallest, putting all the contexts with "avoid" at the end * (after the smallest values). */ static inline int compare_pgscan_rate_and_memory_usage(OomdCGroupContext * const *c1, OomdCGroupContext * const *c2) { - uint64_t last1, last2; + uint64_t diff1, diff2; int r; assert(c1); @@ -77,22 +80,9 @@ static inline int compare_pgscan_rate_and_memory_usage(OomdCGroupContext * const if (r != 0) return r; - /* If last_pgscan > pgscan, assume the cgroup was recreated and reset last_pgscan to zero. */ - last2 = (*c2)->last_pgscan; - if ((*c2)->last_pgscan > (*c2)->pgscan) { - log_info("Last pgscan %" PRIu64 "greater than current pgscan %" PRIu64 "for %s. Using last pgscan of zero.", - (*c2)->last_pgscan, (*c2)->pgscan, (*c2)->path); - last2 = 0; - } - - last1 = (*c1)->last_pgscan; - if ((*c1)->last_pgscan > (*c1)->pgscan) { - log_info("Last pgscan %" PRIu64 "greater than current pgscan %" PRIu64 "for %s. Using last pgscan of zero.", - (*c1)->last_pgscan, (*c1)->pgscan, (*c1)->path); - last1 = 0; - } - - r = CMP((*c2)->pgscan - last2, (*c1)->pgscan - last1); + diff1 = oomd_pgscan_rate(*c1); + diff2 = oomd_pgscan_rate(*c2); + r = CMP(diff2, diff1); if (r != 0) return r; From df637ede7b4e1d0faf8d620d626a0af230712a9e Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Fri, 26 Mar 2021 00:39:25 -0700 Subject: [PATCH 3/8] oomd: rework memory reclaim detection logic systemd-oomd only monitors and kills within a selected cgroup subtree For memory pressure kills, this means it's unnecessary to get the pgscan rate across all the monitored memory pressure cgroups. The increase will show up whether we do a total sum or not, but since we only care about the increase in the subtree we're about to target for a kill, we can simplify the code a bit by not doing this total sum. --- src/oom/oomd-manager.c | 75 +++++++++++++++++++--------------------- src/oom/oomd-manager.h | 1 - src/oom/oomd-util.c | 38 +++++--------------- src/oom/oomd-util.h | 7 ++-- src/oom/test-oomd-util.c | 58 ++++++------------------------- 5 files changed, 57 insertions(+), 122 deletions(-) diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 2a683d5469..73555d2594 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -419,9 +419,6 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t if (r < 0) log_debug_errno(r, "Failed to update monitored memory pressure candidate cgroup contexts, ignoring: %m"); - if (oomd_memory_reclaim(m->monitored_mem_pressure_cgroup_contexts)) - m->last_reclaim_at = usec_now; - /* Since pressure counters are lagging, we need to wait a bit after a kill to ensure we don't read stale * values and go on a kill storm. */ if (m->mem_pressure_post_action_delay_start > 0) { @@ -437,45 +434,45 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t if (r < 0) log_debug_errno(r, "Failed to check if memory pressure exceeded limits, ignoring: %m"); else if (r == 1) { - /* Check if there was reclaim activity in the given interval. The concern is the following case: - * Pressure climbed, a lot of high-frequency pages were reclaimed, and we killed the offending - * cgroup. Even after this, well-behaved processes will fault in recently resident pages and - * this will cause pressure to remain high. Thus if there isn't any reclaim pressure, no need - * to kill something (it won't help anyways). */ - if ((usec_now - m->last_reclaim_at) <= RECLAIM_DURATION_USEC) { - OomdCGroupContext *t; + OomdCGroupContext *t; + SET_FOREACH(t, targets) { + _cleanup_free_ char *selected = NULL; + char ts[FORMAT_TIMESPAN_MAX]; - SET_FOREACH(t, targets) { - _cleanup_free_ char *selected = NULL; - char ts[FORMAT_TIMESPAN_MAX]; + /* Check if there was reclaim activity in the given interval. The concern is the following case: + * Pressure climbed, a lot of high-frequency pages were reclaimed, and we killed the offending + * cgroup. Even after this, well-behaved processes will fault in recently resident pages and + * this will cause pressure to remain high. Thus if there isn't any reclaim pressure, no need + * to kill something (it won't help anyways). */ + if ((now(CLOCK_MONOTONIC) - t->last_had_mem_reclaim) > RECLAIM_DURATION_USEC) + continue; - log_debug("Memory pressure for %s is %lu.%02lu%% > %lu.%02lu%% for > %s with reclaim activity", - t->path, - LOAD_INT(t->memory_pressure.avg10), LOAD_FRAC(t->memory_pressure.avg10), - LOAD_INT(t->mem_pressure_limit), LOAD_FRAC(t->mem_pressure_limit), - format_timespan(ts, sizeof ts, - m->default_mem_pressure_duration_usec, - USEC_PER_SEC)); + log_debug("Memory pressure for %s is %lu.%02lu%% > %lu.%02lu%% for > %s with reclaim activity", + t->path, + LOAD_INT(t->memory_pressure.avg10), LOAD_FRAC(t->memory_pressure.avg10), + LOAD_INT(t->mem_pressure_limit), LOAD_FRAC(t->mem_pressure_limit), + format_timespan(ts, sizeof ts, + m->default_mem_pressure_duration_usec, + USEC_PER_SEC)); - r = oomd_kill_by_pgscan_rate(m->monitored_mem_pressure_cgroup_contexts_candidates, t->path, m->dry_run, &selected); - if (r == -ENOMEM) - return log_oom(); - if (r < 0) - log_notice_errno(r, "Failed to kill any cgroup(s) under %s based on pressure: %m", t->path); - else { - /* Don't act on all the high pressure cgroups at once; return as soon as we kill one */ - m->mem_pressure_post_action_delay_start = usec_now; - if (selected) - log_notice("Killed %s due to memory pressure for %s being %lu.%02lu%% > %lu.%02lu%%" - " for > %s with reclaim activity", - selected, t->path, - LOAD_INT(t->memory_pressure.avg10), LOAD_FRAC(t->memory_pressure.avg10), - LOAD_INT(t->mem_pressure_limit), LOAD_FRAC(t->mem_pressure_limit), - format_timespan(ts, sizeof ts, - m->default_mem_pressure_duration_usec, - USEC_PER_SEC)); - return 0; - } + r = oomd_kill_by_pgscan_rate(m->monitored_mem_pressure_cgroup_contexts_candidates, t->path, m->dry_run, &selected); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + log_notice_errno(r, "Failed to kill any cgroup(s) under %s based on pressure: %m", t->path); + else { + /* Don't act on all the high pressure cgroups at once; return as soon as we kill one */ + m->mem_pressure_post_action_delay_start = usec_now; + if (selected) + log_notice("Killed %s due to memory pressure for %s being %lu.%02lu%% > %lu.%02lu%%" + " for > %s with reclaim activity", + selected, t->path, + LOAD_INT(t->memory_pressure.avg10), LOAD_FRAC(t->memory_pressure.avg10), + LOAD_INT(t->mem_pressure_limit), LOAD_FRAC(t->mem_pressure_limit), + format_timespan(ts, sizeof ts, + m->default_mem_pressure_duration_usec, + USEC_PER_SEC)); + return 0; } } } diff --git a/src/oom/oomd-manager.h b/src/oom/oomd-manager.h index 3df7ce94d7..d5407003d3 100644 --- a/src/oom/oomd-manager.h +++ b/src/oom/oomd-manager.h @@ -46,7 +46,6 @@ struct Manager { OomdSystemContext system_context; - usec_t last_reclaim_at; usec_t mem_pressure_post_action_delay_start; sd_event_source *swap_context_event_source; diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index 5133b1c060..fcab31830c 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -104,36 +104,6 @@ int oomd_pressure_above(Hashmap *h, usec_t duration, Set **ret) { return 0; } -bool oomd_memory_reclaim(Hashmap *h) { - uint64_t pgscan = 0, pgscan_of = 0, last_pgscan = 0, last_pgscan_of = 0; - OomdCGroupContext *ctx; - - assert(h); - - /* If sum of all the current pgscan values are greater than the sum of all the last_pgscan values, - * there was reclaim activity. Used along with pressure checks to decide whether to take action. */ - - HASHMAP_FOREACH(ctx, h) { - uint64_t sum; - - sum = pgscan + ctx->pgscan; - if (sum < pgscan || sum < ctx->pgscan) - pgscan_of++; /* count overflows */ - pgscan = sum; - - sum = last_pgscan + ctx->last_pgscan; - if (sum < last_pgscan || sum < ctx->last_pgscan) - last_pgscan_of++; /* count overflows */ - last_pgscan = sum; - } - - /* overflow counts are the same, return sums comparison */ - if (last_pgscan_of == pgscan_of) - return pgscan > last_pgscan; - - return pgscan_of > last_pgscan_of; -} - uint64_t oomd_pgscan_rate(const OomdCGroupContext *c) { uint64_t last_pgscan; @@ -448,8 +418,12 @@ int oomd_insert_cgroup_context(Hashmap *old_h, Hashmap *new_h, const char *path) curr_ctx->last_pgscan = old_ctx->pgscan; curr_ctx->mem_pressure_limit = old_ctx->mem_pressure_limit; curr_ctx->last_hit_mem_pressure_limit = old_ctx->last_hit_mem_pressure_limit; + curr_ctx->last_had_mem_reclaim = old_ctx->last_had_mem_reclaim; } + if (oomd_pgscan_rate(curr_ctx) > 0) + curr_ctx->last_had_mem_reclaim = now(CLOCK_MONOTONIC); + r = hashmap_put(new_h, curr_ctx->path, curr_ctx); if (r < 0) return r; @@ -474,6 +448,10 @@ void oomd_update_cgroup_contexts_between_hashmaps(Hashmap *old_h, Hashmap *curr_ ctx->last_pgscan = old_ctx->pgscan; ctx->mem_pressure_limit = old_ctx->mem_pressure_limit; ctx->last_hit_mem_pressure_limit = old_ctx->last_hit_mem_pressure_limit; + ctx->last_had_mem_reclaim = old_ctx->last_had_mem_reclaim; + + if (oomd_pgscan_rate(ctx) > 0) + ctx->last_had_mem_reclaim = now(CLOCK_MONOTONIC); } } diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index af08504c5b..b3b637ba8b 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -32,10 +32,11 @@ struct OomdCGroupContext { ManagedOOMPreference preference; - /* These are only used by oomd_pressure_above for acting on high memory pressure. */ + /* These are only used for acting on high memory pressure. */ loadavg_t mem_pressure_limit; usec_t mem_pressure_duration_usec; usec_t last_hit_mem_pressure_limit; + usec_t last_had_mem_reclaim; }; struct OomdSystemContext { @@ -57,10 +58,6 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(OomdCGroupContext*, oomd_cgroup_context_free); * Returns -ENOMEM for allocation errors. */ int oomd_pressure_above(Hashmap *h, usec_t duration, Set **ret); -/* Sum up current OomdCGroupContexts' pgscan values and last interval's pgscan values in `h`. Returns true if the - * current sum is higher than the last interval's sum (there was some reclaim activity). */ -bool oomd_memory_reclaim(Hashmap *h); - /* Returns true if the amount of swap free is below the permyriad of swap specified by `threshold_permyriad`. */ bool oomd_swap_free_below(const OomdSystemContext *ctx, int threshold_permyriad); diff --git a/src/oom/test-oomd-util.c b/src/oom/test-oomd-util.c index bd1c574ca7..92248aa063 100644 --- a/src/oom/test-oomd-util.c +++ b/src/oom/test-oomd-util.c @@ -153,9 +153,10 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { assert_se(oomd_insert_cgroup_context(NULL, h1, cgroup) == -EEXIST); /* make sure certain values from h1 get updated in h2 */ - c1->pgscan = 5555; + c1->pgscan = UINT64_MAX; c1->mem_pressure_limit = 6789; c1->last_hit_mem_pressure_limit = 42; + c1->last_had_mem_reclaim = 888; assert_se(h2 = hashmap_new(&oomd_cgroup_ctx_hash_ops)); assert_se(oomd_insert_cgroup_context(h1, h2, cgroup) == 0); c1 = hashmap_get(h1, cgroup); @@ -163,9 +164,10 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { assert_se(c1); assert_se(c2); assert_se(c1 != c2); - assert_se(c2->last_pgscan == 5555); + assert_se(c2->last_pgscan == UINT64_MAX); assert_se(c2->mem_pressure_limit == 6789); assert_se(c2->last_hit_mem_pressure_limit == 42); + assert_se(c2->last_had_mem_reclaim == 888); /* assumes the live pgscan is less than UINT64_MAX */ /* Assert that avoid/omit are not set if the cgroup is not owned by root */ if (test_xattrs) { @@ -182,20 +184,22 @@ static void test_oomd_update_cgroup_contexts_between_hashmaps(void) { char **paths = STRV_MAKE("/0.slice", "/1.slice"); - OomdCGroupContext ctx_old[3] = { + OomdCGroupContext ctx_old[2] = { { .path = paths[0], .mem_pressure_limit = 5, .last_hit_mem_pressure_limit = 777, + .last_had_mem_reclaim = 888, .pgscan = 57 }, { .path = paths[1], .mem_pressure_limit = 6, .last_hit_mem_pressure_limit = 888, + .last_had_mem_reclaim = 888, .pgscan = 42 }, }; - OomdCGroupContext ctx_new[3] = { + OomdCGroupContext ctx_new[2] = { { .path = paths[0], - .pgscan = 100 }, + .pgscan = 57 }, { .path = paths[1], .pgscan = 101 }, }; @@ -215,12 +219,14 @@ static void test_oomd_update_cgroup_contexts_between_hashmaps(void) { assert_se(c_old->pgscan == c_new->last_pgscan); assert_se(c_old->mem_pressure_limit == c_new->mem_pressure_limit); assert_se(c_old->last_hit_mem_pressure_limit == c_new->last_hit_mem_pressure_limit); + assert_se(c_old->last_had_mem_reclaim == c_new->last_had_mem_reclaim); assert_se(c_old = hashmap_get(h_old, "/1.slice")); assert_se(c_new = hashmap_get(h_new, "/1.slice")); assert_se(c_old->pgscan == c_new->last_pgscan); assert_se(c_old->mem_pressure_limit == c_new->mem_pressure_limit); assert_se(c_old->last_hit_mem_pressure_limit == c_new->last_hit_mem_pressure_limit); + assert_se(c_new->last_had_mem_reclaim > c_old->last_had_mem_reclaim); } static void test_oomd_system_context_acquire(void) { @@ -304,47 +310,6 @@ static void test_oomd_pressure_above(void) { assert_se(c->last_hit_mem_pressure_limit == 0); } -static void test_oomd_memory_reclaim(void) { - _cleanup_hashmap_free_ Hashmap *h1 = NULL; - char **paths = STRV_MAKE("/0.slice", - "/1.slice", - "/2.slice", - "/3.slice", - "/4.slice"); - - OomdCGroupContext ctx[5] = { - { .path = paths[0], - .last_pgscan = 100, - .pgscan = 100 }, - { .path = paths[1], - .last_pgscan = 100, - .pgscan = 100 }, - { .path = paths[2], - .last_pgscan = 77, - .pgscan = 33 }, - { .path = paths[3], - .last_pgscan = UINT64_MAX, - .pgscan = 100 }, - { .path = paths[4], - .last_pgscan = 100, - .pgscan = UINT64_MAX }, - }; - - assert_se(h1 = hashmap_new(&string_hash_ops)); - assert_se(hashmap_put(h1, paths[0], &ctx[0]) >= 0); - assert_se(hashmap_put(h1, paths[1], &ctx[1]) >= 0); - assert_se(oomd_memory_reclaim(h1) == false); - - assert_se(hashmap_put(h1, paths[2], &ctx[2]) >= 0); - assert_se(oomd_memory_reclaim(h1) == false); - - assert_se(hashmap_put(h1, paths[4], &ctx[4]) >= 0); - assert_se(oomd_memory_reclaim(h1) == true); - - assert_se(hashmap_put(h1, paths[3], &ctx[3]) >= 0); - assert_se(oomd_memory_reclaim(h1) == false); -} - static void test_oomd_swap_free_below(void) { OomdSystemContext ctx = (OomdSystemContext) { .swap_total = 20971512 * 1024U, @@ -461,7 +426,6 @@ int main(void) { test_oomd_update_cgroup_contexts_between_hashmaps(); test_oomd_system_context_acquire(); test_oomd_pressure_above(); - test_oomd_memory_reclaim(); test_oomd_swap_free_below(); test_oomd_sort_cgroups(); From 69c8f0255a0a8127548907028bef09ea66056f9f Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Sat, 27 Mar 2021 02:02:00 -0700 Subject: [PATCH 4/8] oomd: rename last_hit_mem_pressure_limit -> mem_pressure_limit_hit_start Since this is only changed the first time the limit is hit (and remains set as long as the pressure remains over), I changed the name to better reflect that. Keeps consistent with "last_had_mem_reclaim" which is actually updated every time there is reclaim activity. --- src/oom/oomd-util.c | 12 ++++++------ src/oom/oomd-util.h | 4 ++-- src/oom/test-oomd-util.c | 20 ++++++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index fcab31830c..a1813bfd81 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -82,17 +82,17 @@ int oomd_pressure_above(Hashmap *h, usec_t duration, Set **ret) { if (ctx->memory_pressure.avg10 > ctx->mem_pressure_limit) { usec_t diff; - if (ctx->last_hit_mem_pressure_limit == 0) - ctx->last_hit_mem_pressure_limit = now(CLOCK_MONOTONIC); + if (ctx->mem_pressure_limit_hit_start == 0) + ctx->mem_pressure_limit_hit_start = now(CLOCK_MONOTONIC); - diff = now(CLOCK_MONOTONIC) - ctx->last_hit_mem_pressure_limit; + diff = now(CLOCK_MONOTONIC) - ctx->mem_pressure_limit_hit_start; if (diff >= duration) { r = set_put(targets, ctx); if (r < 0) return -ENOMEM; } } else - ctx->last_hit_mem_pressure_limit = 0; + ctx->mem_pressure_limit_hit_start = 0; } if (!set_isempty(targets)) { @@ -417,7 +417,7 @@ int oomd_insert_cgroup_context(Hashmap *old_h, Hashmap *new_h, const char *path) if (old_ctx) { curr_ctx->last_pgscan = old_ctx->pgscan; curr_ctx->mem_pressure_limit = old_ctx->mem_pressure_limit; - curr_ctx->last_hit_mem_pressure_limit = old_ctx->last_hit_mem_pressure_limit; + curr_ctx->mem_pressure_limit_hit_start = old_ctx->mem_pressure_limit_hit_start; curr_ctx->last_had_mem_reclaim = old_ctx->last_had_mem_reclaim; } @@ -447,7 +447,7 @@ void oomd_update_cgroup_contexts_between_hashmaps(Hashmap *old_h, Hashmap *curr_ ctx->last_pgscan = old_ctx->pgscan; ctx->mem_pressure_limit = old_ctx->mem_pressure_limit; - ctx->last_hit_mem_pressure_limit = old_ctx->last_hit_mem_pressure_limit; + ctx->mem_pressure_limit_hit_start = old_ctx->mem_pressure_limit_hit_start; ctx->last_had_mem_reclaim = old_ctx->last_had_mem_reclaim; if (oomd_pgscan_rate(ctx) > 0) diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index b3b637ba8b..7f14df972a 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -35,7 +35,7 @@ struct OomdCGroupContext { /* These are only used for acting on high memory pressure. */ loadavg_t mem_pressure_limit; usec_t mem_pressure_duration_usec; - usec_t last_hit_mem_pressure_limit; + usec_t mem_pressure_limit_hit_start; usec_t last_had_mem_reclaim; }; @@ -52,7 +52,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(OomdCGroupContext*, oomd_cgroup_context_free); /* Scans all the OomdCGroupContexts in `h` and returns 1 and a set of pointers to those OomdCGroupContexts in `ret` * if any of them have exceeded their supplied memory pressure limits for the `duration` length of time. - * `last_hit_mem_pressure_limit` is updated accordingly for each entry when the limit is exceeded, and when it returns + * `mem_pressure_limit_hit_start` is updated accordingly for the first time the limit is exceeded, and when it returns * below the limit. * Returns 0 and sets `ret` to an empty set if no entries exceeded limits for `duration`. * Returns -ENOMEM for allocation errors. */ diff --git a/src/oom/test-oomd-util.c b/src/oom/test-oomd-util.c index 92248aa063..6676f4d29a 100644 --- a/src/oom/test-oomd-util.c +++ b/src/oom/test-oomd-util.c @@ -155,7 +155,7 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { /* make sure certain values from h1 get updated in h2 */ c1->pgscan = UINT64_MAX; c1->mem_pressure_limit = 6789; - c1->last_hit_mem_pressure_limit = 42; + c1->mem_pressure_limit_hit_start = 42; c1->last_had_mem_reclaim = 888; assert_se(h2 = hashmap_new(&oomd_cgroup_ctx_hash_ops)); assert_se(oomd_insert_cgroup_context(h1, h2, cgroup) == 0); @@ -166,7 +166,7 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) { assert_se(c1 != c2); assert_se(c2->last_pgscan == UINT64_MAX); assert_se(c2->mem_pressure_limit == 6789); - assert_se(c2->last_hit_mem_pressure_limit == 42); + assert_se(c2->mem_pressure_limit_hit_start == 42); assert_se(c2->last_had_mem_reclaim == 888); /* assumes the live pgscan is less than UINT64_MAX */ /* Assert that avoid/omit are not set if the cgroup is not owned by root */ @@ -187,12 +187,12 @@ static void test_oomd_update_cgroup_contexts_between_hashmaps(void) { OomdCGroupContext ctx_old[2] = { { .path = paths[0], .mem_pressure_limit = 5, - .last_hit_mem_pressure_limit = 777, + .mem_pressure_limit_hit_start = 777, .last_had_mem_reclaim = 888, .pgscan = 57 }, { .path = paths[1], .mem_pressure_limit = 6, - .last_hit_mem_pressure_limit = 888, + .mem_pressure_limit_hit_start = 888, .last_had_mem_reclaim = 888, .pgscan = 42 }, }; @@ -218,14 +218,14 @@ static void test_oomd_update_cgroup_contexts_between_hashmaps(void) { assert_se(c_new = hashmap_get(h_new, "/0.slice")); assert_se(c_old->pgscan == c_new->last_pgscan); assert_se(c_old->mem_pressure_limit == c_new->mem_pressure_limit); - assert_se(c_old->last_hit_mem_pressure_limit == c_new->last_hit_mem_pressure_limit); + assert_se(c_old->mem_pressure_limit_hit_start == c_new->mem_pressure_limit_hit_start); assert_se(c_old->last_had_mem_reclaim == c_new->last_had_mem_reclaim); assert_se(c_old = hashmap_get(h_old, "/1.slice")); assert_se(c_new = hashmap_get(h_new, "/1.slice")); assert_se(c_old->pgscan == c_new->last_pgscan); assert_se(c_old->mem_pressure_limit == c_new->mem_pressure_limit); - assert_se(c_old->last_hit_mem_pressure_limit == c_new->last_hit_mem_pressure_limit); + assert_se(c_old->mem_pressure_limit_hit_start == c_new->mem_pressure_limit_hit_start); assert_se(c_new->last_had_mem_reclaim > c_old->last_had_mem_reclaim); } @@ -289,7 +289,7 @@ static void test_oomd_pressure_above(void) { assert_se(oomd_pressure_above(h1, 0 /* duration */, &t1) == 1); assert_se(set_contains(t1, &ctx[0]) == true); assert_se(c = hashmap_get(h1, "/herp.slice")); - assert_se(c->last_hit_mem_pressure_limit > 0); + assert_se(c->mem_pressure_limit_hit_start > 0); /* Low memory pressure */ assert_se(h2 = hashmap_new(&string_hash_ops)); @@ -297,7 +297,7 @@ static void test_oomd_pressure_above(void) { assert_se(oomd_pressure_above(h2, 0 /* duration */, &t2) == 0); assert_se(t2 == NULL); assert_se(c = hashmap_get(h2, "/derp.slice")); - assert_se(c->last_hit_mem_pressure_limit == 0); + assert_se(c->mem_pressure_limit_hit_start == 0); /* High memory pressure w/ multiple cgroups */ assert_se(hashmap_put(h1, "/derp.slice", &ctx[1]) >= 0); @@ -305,9 +305,9 @@ static void test_oomd_pressure_above(void) { assert_se(set_contains(t3, &ctx[0]) == true); assert_se(set_size(t3) == 1); assert_se(c = hashmap_get(h1, "/herp.slice")); - assert_se(c->last_hit_mem_pressure_limit > 0); + assert_se(c->mem_pressure_limit_hit_start > 0); assert_se(c = hashmap_get(h1, "/derp.slice")); - assert_se(c->last_hit_mem_pressure_limit == 0); + assert_se(c->mem_pressure_limit_hit_start == 0); } static void test_oomd_swap_free_below(void) { From 14140b35440306d2fc1f564b8a24aed0d65e44ba Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Fri, 26 Mar 2021 00:54:46 -0700 Subject: [PATCH 5/8] oomd: delete unused variables --- src/oom/oomd-manager.h | 3 --- src/oom/oomd-util.h | 1 - 2 files changed, 4 deletions(-) diff --git a/src/oom/oomd-manager.h b/src/oom/oomd-manager.h index d5407003d3..e08ad74701 100644 --- a/src/oom/oomd-manager.h +++ b/src/oom/oomd-manager.h @@ -11,9 +11,6 @@ /* Pressure counters are lagging (~2 seconds) compared to swap so polling too frequently just wastes CPU */ #define MEM_PRESSURE_INTERVAL_USEC (1 * USEC_PER_SEC) -/* Used to weight the averages */ -#define AVERAGE_SIZE_DECAY 4 - /* Take action if 10s of memory pressure > 60 for more than 30s. We use the "full" value from PSI so this is the * percentage of time all tasks were delayed (i.e. unproductive). * Generally 60 or higher might be acceptable for something like system.slice with no memory.high set; processes in diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index 7f14df972a..a8fa090755 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -34,7 +34,6 @@ struct OomdCGroupContext { /* These are only used for acting on high memory pressure. */ loadavg_t mem_pressure_limit; - usec_t mem_pressure_duration_usec; usec_t mem_pressure_limit_hit_start; usec_t last_had_mem_reclaim; }; From a858355e4a7168625ec1b9e5d17fdb6a11dfecb8 Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Fri, 26 Mar 2021 01:19:10 -0700 Subject: [PATCH 6/8] oomd: force DefaultMemoryPressureDurationSec= to be greater than or equal 1 sec --- man/oomd.conf.xml | 2 +- src/oom/oomd.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/man/oomd.conf.xml b/man/oomd.conf.xml index 6156c98fbd..0ae7e109b1 100644 --- a/man/oomd.conf.xml +++ b/man/oomd.conf.xml @@ -81,7 +81,7 @@ Sets the amount of time a unit's control group needs to have exceeded memory pressure limits before systemd-oomd will take action. Memory pressure limits are defined by DefaultMemoryPressureLimit= and ManagedOOMMemoryPressureLimit=. - Defaults to 30 seconds when this property is unset or set to 0. + Must be set to 0, or at least 1 second. Defaults to 30 seconds when unset or 0. diff --git a/src/oom/oomd.c b/src/oom/oomd.c index 6e2a5889d1..deb7b094d5 100644 --- a/src/oom/oomd.c +++ b/src/oom/oomd.c @@ -155,6 +155,9 @@ static int run(int argc, char *argv[]) { assert_se(sigprocmask_many(SIG_BLOCK, NULL, SIGTERM, SIGINT, -1) >= 0); + if (arg_mem_pressure_usec > 0 && arg_mem_pressure_usec < 1 * USEC_PER_SEC) + log_error_errno(SYNTHETIC_ERRNO(EINVAL), "DefaultMemoryPressureDurationSec= must be 0 or at least 1s"); + r = manager_new(&m); if (r < 0) return log_error_errno(r, "Failed to create manager: %m"); From cb13961ada52c1b27f6d6c2c6e37a2901f01ed30 Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Fri, 26 Mar 2021 01:53:15 -0700 Subject: [PATCH 7/8] oomd: don't get pressure candidates on every interval Only start collecting candidates for a memory pressure kill when we're hitting the limit (but before the duration hitting that limit is exceeded). This brings CPU util from ~1% to 0.3%. Addresses CPU util from https://bugzilla.redhat.com/show_bug.cgi?id=1941340 and https://bugzilla.redhat.com/show_bug.cgi?id=1944646 --- src/oom/oomd-manager.c | 48 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 73555d2594..07a5d52d1e 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -374,8 +374,17 @@ static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void return 0; } +static void clear_candidate_hashmapp(Manager **m) { + if (*m) + hashmap_clear((*m)->monitored_mem_pressure_cgroup_contexts_candidates); +} + static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t usec, void *userdata) { + /* Don't want to use stale candidate data. Setting this will clear the candidate hashmap on return unless we + * update the candidate data (in which case clear_candidates will be NULL). */ + _cleanup_(clear_candidate_hashmapp) Manager *clear_candidates = userdata; _cleanup_set_free_ Set *targets = NULL; + bool in_post_action_delay = false; Manager *m = userdata; usec_t usec_now; int r; @@ -400,10 +409,8 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t } /* Return early if nothing is requesting memory pressure monitoring */ - if (hashmap_isempty(m->monitored_mem_pressure_cgroup_contexts)) { - hashmap_clear(m->monitored_mem_pressure_cgroup_contexts_candidates); + if (hashmap_isempty(m->monitored_mem_pressure_cgroup_contexts)) return 0; - } /* Update the cgroups used for detection/action */ r = update_monitored_cgroup_contexts(&m->monitored_mem_pressure_cgroup_contexts); @@ -423,7 +430,7 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t * values and go on a kill storm. */ if (m->mem_pressure_post_action_delay_start > 0) { if (m->mem_pressure_post_action_delay_start + POST_ACTION_DELAY_USEC > usec_now) - return 0; + in_post_action_delay = true; else m->mem_pressure_post_action_delay_start = 0; } @@ -433,7 +440,7 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t return log_oom(); if (r < 0) log_debug_errno(r, "Failed to check if memory pressure exceeded limits, ignoring: %m"); - else if (r == 1) { + else if (r == 1 && !in_post_action_delay) { OomdCGroupContext *t; SET_FOREACH(t, targets) { _cleanup_free_ char *selected = NULL; @@ -455,6 +462,15 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t m->default_mem_pressure_duration_usec, USEC_PER_SEC)); + r = update_monitored_cgroup_contexts_candidates( + m->monitored_mem_pressure_cgroup_contexts, &m->monitored_mem_pressure_cgroup_contexts_candidates); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + log_debug_errno(r, "Failed to update monitored memory pressure candidate cgroup contexts, ignoring: %m"); + else + clear_candidates = NULL; + r = oomd_kill_by_pgscan_rate(m->monitored_mem_pressure_cgroup_contexts_candidates, t->path, m->dry_run, &selected); if (r == -ENOMEM) return log_oom(); @@ -475,6 +491,28 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t return 0; } } + } else { + /* If any monitored cgroup is over their pressure limit, get all the kill candidates for every + * monitored cgroup. This saves CPU cycles from doing it every interval by only doing it when a kill + * might happen. + * Candidate cgroup data will continue to get updated during the post-action delay period in case + * pressure continues to be high after a kill. */ + OomdCGroupContext *c; + HASHMAP_FOREACH(c, m->monitored_mem_pressure_cgroup_contexts) { + if (c->mem_pressure_limit_hit_start == 0) + continue; + + r = update_monitored_cgroup_contexts_candidates( + m->monitored_mem_pressure_cgroup_contexts, &m->monitored_mem_pressure_cgroup_contexts_candidates); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + log_debug_errno(r, "Failed to update monitored memory pressure candidate cgroup contexts, ignoring: %m"); + else { + clear_candidates = NULL; + break; + } + } } return 0; From 685b0985f0faeb349d1449f8e9d87a9c87e1e24f Mon Sep 17 00:00:00 2001 From: Anita Zhang Date: Fri, 26 Mar 2021 02:37:01 -0700 Subject: [PATCH 8/8] oomd: threshold swap kill candidates to usages of more than 5% In some instances, particularly with swap on zram, swap used will be high while there is still a lot of memory available. FB OOMD handles this by thresholding kills to X% of total swap usage. Let's do the same thing here. Anecdotally with these thresholds and my laptop which is exclusively swap on zram I can sit at 0K / 4G swap free with most of memory free and systemd-oomd doesn't kill anything. Partially addresses aggressive kill behavior from https://bugzilla.redhat.com/show_bug.cgi?id=1941170 --- man/oomd.conf.xml | 6 +++--- src/oom/oomd-manager.c | 4 +++- src/oom/oomd-manager.h | 3 +++ src/oom/oomd-util.c | 12 ++++++------ src/oom/oomd-util.h | 2 +- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/man/oomd.conf.xml b/man/oomd.conf.xml index 0ae7e109b1..27914e0699 100644 --- a/man/oomd.conf.xml +++ b/man/oomd.conf.xml @@ -52,9 +52,9 @@ Sets the limit for swap usage on the system before systemd-oomd will take action. If the fraction of swap used on the system is more than what is defined here, - systemd-oomd will act on eligible descendant control groups, starting from the - ones with the highest swap usage to the lowest swap usage. Which control groups are monitored and - what action gets taken depends on what the unit has configured for + systemd-oomd will act on eligible descendant control groups with swap usage greater + than 5% of total swap, starting from the ones with the highest swap usage. Which + control groups are monitored and what action gets taken depends on what the unit has configured for ManagedOOMSwap=. Takes a value specified in percent (when suffixed with "%"), permille ("‰") or permyriad ("‱"), between 0% and 100%, inclusive. Defaults to 90%. diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 07a5d52d1e..49dc5eb26e 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -345,6 +345,7 @@ static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void if (oomd_swap_free_below(&m->system_context, 10000 - m->swap_used_limit_permyriad)) { _cleanup_hashmap_free_ Hashmap *candidates = NULL; _cleanup_free_ char *selected = NULL; + uint64_t threshold; log_debug("Swap used (%"PRIu64") / total (%"PRIu64") is more than " PERMYRIAD_AS_PERCENT_FORMAT_STR, m->system_context.swap_used, m->system_context.swap_total, @@ -356,7 +357,8 @@ static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void if (r < 0) log_debug_errno(r, "Failed to get monitored swap cgroup candidates, ignoring: %m"); - r = oomd_kill_by_swap_usage(candidates, m->dry_run, &selected); + threshold = m->system_context.swap_total * THRESHOLD_SWAP_USED_PERCENT / 100; + r = oomd_kill_by_swap_usage(candidates, threshold, m->dry_run, &selected); if (r == -ENOMEM) return log_oom(); if (r < 0) diff --git a/src/oom/oomd-manager.h b/src/oom/oomd-manager.h index e08ad74701..dc170f2bda 100644 --- a/src/oom/oomd-manager.h +++ b/src/oom/oomd-manager.h @@ -19,6 +19,9 @@ #define DEFAULT_MEM_PRESSURE_LIMIT_PERCENT 60 #define DEFAULT_SWAP_USED_LIMIT_PERCENT 90 +/* Only tackle candidates with large swap usage. */ +#define THRESHOLD_SWAP_USED_PERCENT 5 + #define RECLAIM_DURATION_USEC (30 * USEC_PER_SEC) #define POST_ACTION_DELAY_USEC (15 * USEC_PER_SEC) diff --git a/src/oom/oomd-util.c b/src/oom/oomd-util.c index a1813bfd81..5bf81479c9 100644 --- a/src/oom/oomd-util.c +++ b/src/oom/oomd-util.c @@ -233,7 +233,7 @@ int oomd_kill_by_pgscan_rate(Hashmap *h, const char *prefix, bool dry_run, char return ret; } -int oomd_kill_by_swap_usage(Hashmap *h, bool dry_run, char **ret_selected) { +int oomd_kill_by_swap_usage(Hashmap *h, uint64_t threshold_usage, bool dry_run, char **ret_selected) { _cleanup_free_ OomdCGroupContext **sorted = NULL; int n, r, ret = 0; @@ -244,12 +244,12 @@ int oomd_kill_by_swap_usage(Hashmap *h, bool dry_run, char **ret_selected) { if (n < 0) return n; - /* Try to kill cgroups with non-zero swap usage until we either succeed in - * killing or we get to a cgroup with no swap usage. */ + /* Try to kill cgroups with non-zero swap usage until we either succeed in killing or we get to a cgroup with + * no swap usage. Threshold killing only cgroups with more than threshold swap usage. */ for (int i = 0; i < n; i++) { - /* Skip over cgroups with no resource usage. - * Continue break since there might be "avoid" cgroups at the end. */ - if (sorted[i]->swap_usage == 0) + /* Skip over cgroups with not enough swap usage. Don't break since there might be "avoid" + * cgroups at the end. */ + if (sorted[i]->swap_usage <= threshold_usage) continue; r = oomd_cgroup_kill(sorted[i]->path, true, dry_run); diff --git a/src/oom/oomd-util.h b/src/oom/oomd-util.h index a8fa090755..81fdc5e088 100644 --- a/src/oom/oomd-util.h +++ b/src/oom/oomd-util.h @@ -111,7 +111,7 @@ int oomd_cgroup_kill(const char *path, bool recurse, bool dry_run); * everything in `h` is a candidate. * Returns the killed cgroup in ret_selected. */ int oomd_kill_by_pgscan_rate(Hashmap *h, const char *prefix, bool dry_run, char **ret_selected); -int oomd_kill_by_swap_usage(Hashmap *h, bool dry_run, char **ret_selected); +int oomd_kill_by_swap_usage(Hashmap *h, uint64_t threshold_usage, bool dry_run, char **ret_selected); int oomd_cgroup_context_acquire(const char *path, OomdCGroupContext **ret); int oomd_system_context_acquire(const char *proc_swaps_path, OomdSystemContext *ret);