Let's say a user has two services with ManagedOOMMemoryPressure=kill,
perhaps a web server under system.slice and a batch job under
user.slice. Both exceed their pressure limits. On the previous timer
tick, oomd has already queued the web server's candidate for killing,
but the prekill hook has not yet responded, so the kill is still
pending.
In the code, monitor_memory_pressure_contexts_handler() iterates over
all pressure targets that have exceeded their limits. When it reaches
the web server target and calls oomd_cgroup_kill_mark(), which returns 0
because that cgroup is already queued. The code treats this the same as
a successful new kill: it resets the 15 second delay timer and returns
from the function, exiting the loop.
This loop is handled by SET_FOREACH and the iteration order is
hash-dependent. As such, if the web server target happens coincidentally
to be visited first, oomd never evaluates the batch job target at all.
The effect is twofold:
1. oomd stalls for 15 seconds despite not having initiated any new kill.
That can unnecessarily delay further action to stem increases in
memory pressure. The delay exists to let stale pressure counters
settle after a kill, but no kill has happened here.
2. It non-deterministically skips pressure targets that may have
unqueued candidates, dangerously allowing memory pressure to persist
for longer than it should.
Fix this by skipping cgroups that are already queued so the loop
proceeds to try other pressure targets. We should only delay when a new
kill mark is actually created.
Consider a workload slice with several sibling cgroups. Imagine that one
of those cgroups is removed between the moment oomd enumerates the
directory and the moment it reads memory.oom.group. This is actually
relatively plausible under the high memory pressure conditions where
oomd is most needed.
In this case, the failed read prompts us to `return 0`, which exits the
entire enumeration loop in recursively_get_cgroup_context(). As a
result, all remaining sibling cgroups are silently dropped from the
candidate list for that monitoring cycle.
The effect is that oomd can fail to identify and kill the actual
offending cgroup, allowing memory pressure to persist until a subsequent
cycle where the race doesn't occur.
Fix this by instead proceeding to evaluate further sibling cgroups.
Let's say a user has two services with ManagedOOMMemoryPressure=kill,
one for a web server under system.slice, and one for a batch job under
user.slice. The batch job is causing severe memory pressure, whereas the
web server's cgroup has no processes with significant pgscan activity.
In the code, monitor_memory_pressure_contexts_handler() iterates over
all pressure targets that have exceeded their limits. When
oomd_select_by_pgscan_rate() returns 0 (that is, no candidates) for a
target, we return from the entire SET_FOREACH loop instead of moving to
the next target. Since SET_FOREACH iteration order is hash-dependent, if
the web server target happens to be visited first, oomd will find no
kill candidates for it and exit the loop. The batch job target that is
actually slamming the machine will never even be evaluated, and can
continue to wreak havoc without any intervention.
The effect is that oomd non-deterministically and silently fails to kill
cgroups that it should actually kill, allowing memory pressure to
persist and dangerously build up on the machine.
The fix is simple, keep evaluating remaining targets when one does not
match.
Emitting "oom" doesn't mesh with the org.freedesktop.oom1.Manager
Killed() API contract, which defines "memory-used" and "memory-pressure"
as possible reasons. Consumers that key off reason thus will either lose
policy attribution or may reject the unknown value completely.
Plumb the reason through so it is visible to consumers.
oomd_cgroup_kill_mark() currently returns 0 on all non-error paths. But
the manager only logs that it marked for killing on `if (r > 0)`, which
is thus unreachable.
Changing it to `r >= 0` would also be wrong, because then we would log
on no-op paths.
So let's fix this by making the return value express what actually
happened:
- < 0: failure to queue the kill state
- 0: no new mark was created (already queued or dry-run)
- > 0: a new kill state was queued
oomd_cgroup_kill_mark() allocates a temporary OomdKillState and inserts
it into kill_states via set_ensure_put(). This is keyed by cgroup path.
When the same cgroup is already queued, set_ensure_put() dutifully
returns 0.
The function then returns with
_cleanup_(oomd_kill_state_removep) still armed, which eventually calls
oomd_kill_state_free().
oomd_kill_state_free() removes from kill_states by cgroup-path key, so
because this path already exists, it will remove the existing queued
kill state instead of just dropping the temporary object.
This is wrong and results in mistakenly drops the queued kill state on
duplicates.
This can happen when a cgroup is marked multiple times before the first
queued kill state is consumed. The result is lost kill-state tracking
and incorrect prekill/kill sequencing.
Handle r == 0 explicitly by freeing only the temporary object and
leaving the already queued state intact.
While looking at oomd behaviour in production I noticed that I always
get garbage cgroup paths for the Killed event. Looking more closely, I
noticed that while the signature is (string cgroup, string reason), we
currently erroneously pass the `OomdCGroupContext*` pointer itself as
the first argument to sd_bus_emit_signal(), rather than the ctx->path
string it contains.
The in-memory layout on affected machines in my case is:
struct OomdCGroupContext {
unsigned n_ref;
/* padding */
char *path;
/* ... */
}
...which explains the control characters, since they're garbage from
parsing n_ref, the path pointer, and later fields. At runtime, sd-bus
treats ctx as `const char *` and reads struct bytes as string data,
resulting in garbage being sent.
Pass ctx->path correctly so listeners receive the valid cgroup path.
When a cgroup is selected for termination, send varlink messages
to hooks registered in `/run/systemd/oomd.prekill-hooks/`.
oomd waits up to `PreKillHookTimeoutSec=` seconds for response
before proceeding with the kill.
This reverts commit 332bce5bd7.
The revert is needed because with the PreKill hook, oomd_cgroup_kill()
is not goint to really kill processes but it just creates the callbacks.
So the check is deferred to the real kill.
manager_varlink_init_resolve_hook and manager_varlink_init_userdb are very
similar, but one didn't set a description and the other one had an error
message which didn't print the offending path.
Define constants for the paths, and also change other similar constants
to have slightly shorter names. (It's an "address" and "path", but we don't
need to have both parts in the name, esp. that it makes the name very long.)
On OOM event, oomd tries to kill a cgroup until it succeedes.
The kill can fail with EPERM in case a pid is not killed, this leaves
the cgroup with only half of the processed killed.
This is unlikely but theoretically possible in a user namespace,
where systemd run as root inside the container and tries to kill a
cgroup with some PID from the host namespace.
To address this, send the SIG0 signal to all the processes to check
that we have privileges to kill them.
In all cases, everything that we list in 'extract', we also list in
'sources'. We can simplify things by automatically appending the first
list to the second.
In the listings, move 'extract' key right below 'sources', since now
they are both "sources", just with slightly different meanings.
Let's not leak details from src/shared and src/libsystemd into
src/basic, even though you can't actually do anything useful with
just forward declarations from src/shared.
The sd-forward.h header is put in src/libsystemd/sd-common as we
don't have a directory for shared internal headers for libsystemd
yet.
Let's also rename forward.h to basic-forward.h to keep things
self-explanatory.
If the test is ran multiple times in a row, without an ephemeral
scope (eg: non-booted nspawn), then subsequent runs will fail as
the test cgroup is not cleared so the previous xattrs are still
present. Trim the test cgroup before and after the test.
Follow-up for 61ff7397d0
- use DEFINE_TEST_MAIN() macro,
- use ASSERT_OK() and friends,
- drop restriction that the test must run as root,
- replace sleep(2) with loop of shorter sleep and cg_is_empty(),
- applied several coding style cleanups.
This is not a trivial wrapper around user-created-buffer-based
syscall, so do not use _malloc suffix. Most of our functions
return an internally allocated buffer and this one's not special.
Also, let's deflatten the lists of sources in preparation for the
next commit at the same time.
In systemctl, we split out systemctl-main.c to make sure the definition
of main() is in a separate object which allows us to extract the systemctl.c
object and link it in the fuzzer target without getting a multiple definition
error when linking.
Currently, when we want to add unit tests for code that is compiled into
an executable, we either compile the code at least twice (once for the
executable, and once for each test that uses it) or we create a static
library which is then used by both the executable and all the tests.
Both of these options are not ideal, compiling source files more than
once slows down the build for no reason and creating the intermediate
static libraries takes a lot of boilerplate.
Instead, let's use the extract_objects() method that meson exposes on
build targets. This allows us to extract the objects corresponding to
specific source files and use them in other executables. Because we
define all executables upfront into a dictionary, we integrate this into
the dictionary approach by adding two new fields:
- 'extract' takes a list of files for which objects should be extracted.
The extracted objects are stored in a dict keyed by the executable name
from which they were extracted.
- 'objects' takes the name of an executable from which the extracted
objects should be added to the current executable.
One side effect of this approach is that we can't build test executables
anymore without building the main executable, so we stop building test
executables unless we're also building the main executable. This allows
us to switch to using subdir_done() in all of these subdirectories to skip
parsing them if the corresponding component is disabled.
These changes get me down from 2439 => 2403 ninja targets on a full rebuild
from scratch.
Let's move some logic from _DEFINE_MAIN_FUNCTION() and other places
in main-func.h into functions that we implement in main-func.c to
allow moving some included headers from the header to the .c file.
Currently, NOTIFY_READY from daemon-util.h conflicts with NOTIFY_READY
from NotifyState from service.h so let's rename the constants to avoid
the conflict.
- Drop effectively unused "terminator" param, imply whitespace
- Make ret param optional
- Return ENODATA if the requested key is not found, rather than
ENOENT
- Turn ENOENT -> ENOSYS if /proc/ is not mounted
- Don't skip whitespaces before ':', nothing needs this handling
anyways
- Remove the special treatment for all "0"s. We don't actually
use this for capabilities given pidref_get_capability() exists
- Switch away from read_full_virtual_file() - files using "field"
scheme under /proc/ seem all to be "seq_file"s (refer to
da65941c3e for details on file types)
Now that the necessary functions from log.h have been moved to macro.h,
we can stop including log.h in macro.h. This requires modifying source
files all over the tree to include log.h instead.
Let's return the size in a return parameter instead of the return value.
And if NULL is specified this tells us the caller doesn't care about the
size and expects a NUL terminated string. In that case look for an
embedded NUL byte, and refuse in that case.
This should lock things down a bit, as we'll systematically refuse
embedded NUL strings now when we expect strings.
Given we have the generic interface, let's hook it up everywhere.
This doesnt bother with the Reload() call usually, since that's more
involved, but hooks up the other relevant functions where applicable.
Frantisek Sumsal
Chris Down
Daan De Meyer
Matteo Croce
Daan De Meyer
Yu Watanabe
Zbigniew Jędrzejewski-Szmek
Luca Boccassi
Mike Yuan
Jelle van der Waa
Lennart Poettering