systemd-oomd #15206

anitazha · 2020-03-23T20:50:29Z

systemd-oomd follows the current oomd model more closely than what was discussed at DevConf.CZ: oomd will periodically ask pid1 over varlink for a list of cgroups to monitor and use that to decide what/when to kill. The candidates for killing are the children of the monitored cgroups: either the recursive cgroups of a cgroup with memory.oom.group = 1, or the leaf cgroups. oomd will do the actual killing and set xattrs appropriately so the system manager can use that to determine if processes were killed by oomd. However, this limits enabling oomd to slices owned by pid1. But it does allow oomd to kill delegated cgroups.

dschatzberg

Looking good - see comments inline

src/oom/oomd-manager.c

src/oom/oomd-util.c

keszybz

I didn't look at the actual business logic at all... Some initial comments.

src/basic/cgroup-util.h

src/shared/psi-util.c

src/shared/psi-util.h

src/test/test-psi-util.c

src/oom/oomd-manager.c

meson.build

src/oom/oomd.c

src/shared/psi-util.c

poettering

heya! sorry for the late review. still working though a lot of stuff queued from when i was in parental leave.

it's not a complete review, but a start.

I generally like a lot what I am seeing here, good work. the stuff i found is generally minor.

src/basic/cgroup-util.c

poettering · 2020-04-20T16:09:04Z

src/basic/cgroup-util.c

+
+        r = safe_atou64(value, &v);
+        if (r < 0)
+                return r;


as extra paranoia step we could fail here with an error if v is CGROUP_LIMIT_MAX here, to avoid ambiguity? dunno?

src/basic/cgroup-util.c

src/shared/psi-util.c

src/oom/oomd-util.c

src/oom/oomd-manager.c

poettering · 2020-04-20T16:40:17Z

oh, i see now that some of those helpers already did get merged? ignore that part of my review then

poettering · 2020-04-20T16:40:54Z

please rebase! in particular as those helpers apparently got merged, after all

anitazha · 2020-04-23T00:18:26Z

Hmm I thought I had already rebase-ed past the prior PR merges but I'll do that again when I make some updates. Thanks for the first round review

anitazha

Working through the comments and should have another update soon but just addressing some comments

src/shared/psi-util.h

anitazha · 2020-05-26T01:47:02Z

man/systemd.slice.xml

+      <citerefentry><refentrytitle>systemd.directives</refentrytitle><manvolnum>7</manvolnum></citerefentry>,
+      <citerefentry><refentrytitle>systemd-oomd.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>
    </para>
  </refsect1>


It's a simplification for this first version of the code since that's how the FB Oomd is typically set up now. We intend to support scopes and services too in a later iteration (though I don't think the code will be different at all).

src/shared/psi-util.c

hixiomh

Pull requested

anitazha · 2020-10-08T02:15:08Z

🤦 CI is greener now

keszybz · 2020-10-13T12:19:47Z

I think we have crossed the point where subsequent versions of this PR are less useful than merging it and doing follow-up PRs.

I would propose the following plan:

merge this before v247-rc1
make oomd opt-in (i.e. default to disabled) and only available with -Dmode=developer.
describe the daemon and tools in NEWS, but specify that it's a preview and should not be used for production and that the options names and API details are subject to change.

Two reasons for this:

we want to release v247 soon and we won't have time for proper testing and responding to feedback,
it seems likely that some adjustments will be needed, and eschewing backwards-compatibility will allow us to make a better daemon in the long term.

I scanned the patches again, and it looks all nice. Unless @poettering has any last-minute requests, let's go ahead with the above.

systemd-oomd can be enabled when in developer mode (-Dmode=developer)

anitazha · 2020-10-15T06:21:19Z

I pushed one commit to disable systemd-oomd by default and allow it to be enabled only in developer mode as suggested. I think we can add a NEWS entry in another PR.

keszybz · 2020-10-15T12:17:47Z

Yay!

Conan-Kudo · 2020-10-15T12:57:14Z

Woo!

poettering · 2020-10-15T12:38:36Z

man/org.freedesktop.systemd1.xml

+      @org.freedesktop.DBus.Property.EmitsChangedSignal("false")
+      readonly s ManagedOOMMemoryPressure = '...';
+      @org.freedesktop.DBus.Property.EmitsChangedSignal("false")
+      readonly s ManagedOOMMemoryPressureLimitPercent = '...';


hmm, so in the other cases where we expose scaled values (i.e. percent) we encode things on the wire in uint64_t values that are normalized to UINT32_MAX as 100%. i.e. 150% is mapped to be 6442450944 on the wire, 100% is 4294967296 and 50% is 2147483648. I think we should follow the same logic here.

See the MemoryMinScale/MemoryLowScale/TaskMaxScale properties for exampe.

hence, please rework this to be uint64_t, also use "Scale" as suffix in the name

Hmm I was looking to switching this to "Scale" but it doesn't seem applicable in this case. For the existing properties that are suffixed with "Scale", the percentages are translated to a final value, i.e. number of bytes or number of tasks. But MemoryPressureLimitPercent= is operated on solely as a percentage. PSI averages are shown as a ratio in percent so there is no translation to a whole number.

hmm, it's not really percent though, is it? it has two digits after the dot, i.e. its in units of 1/10,000th, displayed with a dot in the middle, no?

Indeed the PSI average is displayed as a decimal but it represents a percent of time, per https://www.kernel.org/doc/html/latest/accounting/psi.html.

i am not getting what you are trying to say. the dbus interface as merged now accepts percent ranges 0%…100% as integer, i.e. 1% granularity. The kernel otoh exports 0.00%…100.00% as fixed point decimal integer with 0.01% granularity, correct? Thus the D-Bus API loses two decimals of accuracy, no?

I still think we should expose this as 64bit value, with binary fixed point normalized to 2^32. It's more finegrained than the kernel accuracy, but accurate enough to not lose any accuracy compared to what the kernel exposes.

Oh I see what you mean now. Keep the finer level of granularity. Sure I can do that.

man/org.freedesktop.systemd1.xml

src/core/core-varlink.c

src/core/load-fragment-gperf.gperf.m4

src/core/load-fragment.c

src/core/unit.c

src/oom/oomd-manager.c

units/systemd-oomd.service.in

Suggested post-merge in systemd#15206

Post-merge suggestion from systemd#15206

Another suggestion from systemd#15206

Requested in systemd#15206 (comment), preserve the full granularity for memory pressure limits (permyriad) instead of capping out at percent.

klausenbusk · 2021-04-10T00:38:56Z

man/systemd-oomd.service.xml

+    <para>The system must be running systemd with a full unified cgroup hierarchy for the expected cgroups-v2 features.
+    Furthermore, resource accounting must be turned on for all units monitored by <command>systemd-oomd</command>.
+    The easiest way to turn on resource accounting is by ensuring the values for <varname>DefaultCPUAccounting</varname>,
+    <varname>DefaultIOAccounting</varname>, <varname>DefaultMemoryAccounting</varname>, and
+    <varname>DefaultTasksAccounting</varname> are set to <constant>true</constant> in
+    <citerefentry><refentrytitle>systemd-system.conf</refentrytitle><manvolnum>5</manvolnum></citerefentry>.</para>


Is this accurate? Looking at the code, only the memory controller seems to be used.

Issue opened: #19331

Requested in systemd/systemd#15206 (comment), preserve the full granularity for memory pressure limits (permyriad) instead of capping out at percent.

dschatzberg reviewed Mar 23, 2020

View reviewed changes

src/oom/oomd-manager.c Outdated Show resolved Hide resolved

src/oom/oomd-util.c Outdated Show resolved Hide resolved

src/oom/oomd-util.c Outdated Show resolved Hide resolved

keszybz reviewed Mar 23, 2020

View reviewed changes

mrc0mmand reviewed Mar 23, 2020

View reviewed changes

src/shared/psi-util.c Outdated Show resolved Hide resolved

anitazha force-pushed the systoomd-v0 branch 3 times, most recently from 2e62d95 to 2185da1 Compare March 24, 2020 23:25

poettering requested changes Apr 20, 2020

View reviewed changes

poettering added needs-rebase new-feature reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 20, 2020

anitazha commented May 26, 2020

View reviewed changes

hixiomh approved these changes Jun 10, 2020

View reviewed changes

anitazha force-pushed the systoomd-v0 branch 8 times, most recently from 5a2215d to fa4b4d0 Compare June 23, 2020 01:01

anitazha force-pushed the systoomd-v0 branch 4 times, most recently from ba7859b to e406de7 Compare June 26, 2020 17:28

anitazha force-pushed the systoomd-v0 branch 2 times, most recently from 70d6f60 to a382d1c Compare July 10, 2020 21:38

keszybz removed the needs-rebase label Jul 11, 2020

core: add systemd-oomd implicit dependencies

a2db022

anitazha force-pushed the systoomd-v0 branch from d8d6ff7 to d9a452e Compare October 8, 2020 00:13

anitazha removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Oct 8, 2020

man: document systemd-oomd and related items

cf3e578

anitazha force-pushed the systoomd-v0 branch from d9a452e to cf3e578 Compare October 9, 2020 09:40

meson: disable systemd-oomd by default

c199dd3

systemd-oomd can be enabled when in developer mode (-Dmode=developer)

keszybz merged commit 69c0807 into systemd:master Oct 15, 2020

poettering reviewed Oct 15, 2020

View reviewed changes

units/systemd-oomd.service.in Show resolved Hide resolved

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

core: varlink tweaks

e7aba92

Suggested post-merge in systemd#15206

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

core: move where we send unit change updates to oomd

9b72fea

Post-merge suggestion from systemd#15206

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

units: drop ConditionCapability from systemd-oomd.service

f14f091

Another suggestion from systemd#15206

anitazha mentioned this pull request Oct 19, 2020

oomd fixups #17387

Merged

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

core: varlink tweaks

2801d36

Suggested post-merge in systemd#15206

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

core: move where we send unit change updates to oomd

f561e8c

Post-merge suggestion from systemd#15206

anitazha added a commit to anitazha/systemd that referenced this pull request Oct 19, 2020

units: drop ConditionCapability from systemd-oomd.service

32ae9b1

Another suggestion from systemd#15206

lqb pushed a commit to lqb/systemd that referenced this pull request Oct 29, 2020

core: varlink tweaks

3883903

Suggested post-merge in systemd#15206

lqb pushed a commit to lqb/systemd that referenced this pull request Oct 29, 2020

core: move where we send unit change updates to oomd

fc71316

Post-merge suggestion from systemd#15206

lqb pushed a commit to lqb/systemd that referenced this pull request Oct 29, 2020

units: drop ConditionCapability from systemd-oomd.service

f689fd4

Another suggestion from systemd#15206

anitazha deleted the systoomd-v0 branch November 18, 2020 22:50

anitazha mentioned this pull request Feb 3, 2021

oom: parse properties with 1/10000 precision instead of 1/100 #18444

Merged

klausenbusk reviewed Apr 10, 2021

View reviewed changes

Uh oh!

systemd-oomd #15206

systemd-oomd #15206

Uh oh!

Conversation

anitazha commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dschatzberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poettering commented Apr 20, 2020

Uh oh!

poettering commented Apr 20, 2020

Uh oh!

anitazha commented Apr 23, 2020

Uh oh!

anitazha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hixiomh left a comment

Choose a reason for hiding this comment

Uh oh!

anitazha commented Oct 8, 2020

Uh oh!

keszybz commented Oct 13, 2020

Uh oh!

anitazha commented Oct 15, 2020

Uh oh!

keszybz commented Oct 15, 2020

Uh oh!

Conan-Kudo commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anitazha commented Mar 23, 2020 •

edited

Loading