Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

default to higher oom scores for user sessions #20893

Merged
merged 5 commits into from Oct 5, 2021

Conversation

poettering
Copy link
Member

An attempt to revive/finish #17426

@poettering
Copy link
Member Author

/cc @benzea

Copy link
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good to me, but not an oomd expert

@bluca
Copy link
Member

bluca commented Sep 30, 2021

would it be possible to add some checks to https://github.com/systemd/systemd/blob/main/test/units/testsuite-55.sh to cover this?

@github-actions github-actions bot added the units label Sep 30, 2021
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor suggestions.

src/basic/process-util.c Outdated Show resolved Hide resolved
src/core/dbus-manager.c Outdated Show resolved Hide resolved
src/core/main.c Outdated Show resolved Hide resolved
@keszybz keszybz requested a review from anitazha October 1, 2021 09:22
@anitazha
Copy link
Member

anitazha commented Oct 4, 2021

The change and feature looks fine to me. I agree that +100 seems conservative enough for kernel OOM killing and likely will not be noticeably harmful.

@keszybz keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks good-to-merge/with-minor-suggestions and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 4, 2021
@keszybz
Copy link
Member

keszybz commented Oct 4, 2021

Some minor issues are outstanding, but let's merge once that's fixed.

…e service manager's by default

Let's make our service managers slightly less likely to be killed by the
OOM killer by adjusting our services' OOM score adjustment to 100 above
ours. Do this conservatively, i.e. only for regular user sessions.
Let's make it slightly more likely that a per-user service manager is
killed than any system service. We use a conservative 100 (from a range
that goes all the way to 1000).

Replaces: systemd#17426

Together with the previous commit this means: system manager and system
services are placed at OOM score adjustment 0 (specifically: they
inherit kernel default of 0). User service manager (both for root and
non-root) are placed at 100. User services for non-root are placed at
200, those for root inherit 100.

Note that processes forked off the user *sessions* (i.e. not forked off
the per-user service manager) remain at 0 (e.g. the shell process
created by a tty or ssh login). This probably should be
addressed too one day (maybe in pam_systemd?), but is not covered here.
@poettering
Copy link
Member Author

made the three changes. nothing else. upgrading green label.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Oct 4, 2021
@DaanDeMeyer
Copy link
Contributor

test-repart timed out but that doesn't seem related to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed pid1 units
Development

Successfully merging this pull request may close these issues.

None yet

5 participants