Bug 32851 - Avoid services inheriting OOM protection from dbus-daemon
Summary: Avoid services inheriting OOM protection from dbus-daemon
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: lowest enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-01-05 09:52 UTC by Simon McVittie
Modified: 2016-06-30 13:56 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch from Kimmo Hämäläinen via dbus/1.4.1-0maemo0 (843 bytes, patch)
2011-01-05 09:54 UTC, Simon McVittie
Details | Splinter Review
activation: set children oom_score_adj to 0 (1.31 KB, patch)
2016-06-08 09:47 UTC, Simon McVittie
Details | Splinter Review
v2: activation: set children oom_score_adj to 0 (1.44 KB, patch)
2016-06-23 07:54 UTC, WaLyong Cho
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-01-05 09:52:30 UTC
Maemo's dbus-daemon has OOM-protection (/proc/self/oom_adj) so that the OOM killer will try to kill less important things first. To avoid every D-Bus service inheriting this OOM-protection, there's a patch to reset OOM protection before exec()ing children.

It's possible that the way we eventually want to solve this upstream is "tell everyone to launch D-Bus services via systemd or upstart", but until then, it might be useful to pull in the Maemo patch, which I'll attach in a moment.
Comment 1 Simon McVittie 2011-01-05 09:54:41 UTC
Created attachment 41676 [details] [review]
Patch from Kimmo Hämäläinen via dbus/1.4.1-0maemo0

Note that I haven't reviewed this patch yet and have no opinion on whether it's the best approach.
Comment 2 Lennart Poettering 2011-03-09 19:39:06 UTC
Looks good, but the oom_adj interface is already obsolete, hence this should probably be updated to oom_score_adj before it is merged.
Comment 3 Lennart Poettering 2011-03-09 19:40:24 UTC
Also, O_CLOEXEC should be used and O_SYNC be dropped, and the whole thing should be in an #ifdef __linux__ block.
Comment 4 Simon McVittie 2016-06-08 09:47:22 UTC
Created attachment 124402 [details] [review]
activation: set children oom_score_adj to 0

From: WaLyong Cho <walyong.cho@samsung.com>

If dbus is running as systemd service, dbus daemon is running with
oom_score_adj -900 by OOMScoreAdjust=-900. And children will also have
same value with dbus daemon.
To avoid this, set the child itself values after fork () to 0.

---

From the mailing list.
Comment 5 Simon McVittie 2016-06-08 10:02:32 UTC
Comment on attachment 124402 [details] [review]
activation: set children oom_score_adj to 0

Review of attachment 124402 [details] [review]:
-----------------------------------------------------------------

This is clearly better than what was in Maemo, and also better than what we have now, assuming it compiles successfully on every Linux platform we care about.

::: dbus/dbus-spawn.c
@@ +1359,5 @@
>  	}
>        else if (grandchild_pid == 0)
>        {
> +#ifdef __linux__
> +          int fd = open ("/proc/self/oom_score_adj", O_WRONLY | O_CLOEXEC);

To be fully portable, this should ideally be something like

    int fd = -1;

#ifdef O_CLOEXEC
    fd = open (..., O_WRONLY | O_CLOEXEC);
#endif

    if (fd < 0)
      {
        fd = open (..., O_WRONLY);
        _dbus_fd_set_close_on_exec (fd);
      }

    if (fd >= 0)
      ... continue, do the write ...

However, perhaps we don't actually care about Linux platforms that don't have O_CLOEXEC any more; it was introduced in Linux 2.6.23. Our policy is that we don't support platforms whose vendor no longer provides security support, so if nobody ships Linux kernels or glibc that lack O_CLOEXEC in a security-supported distribution, we don't need to bother with this.

#if (defined(__linux__) && defined(O_CLOEXEC)) would be a simpler way to make sure this can never break compilation.

@@ +1363,5 @@
> +          int fd = open ("/proc/self/oom_score_adj", O_WRONLY | O_CLOEXEC);
> +
> +          if (fd >= 0)
> +            {
> +              write (fd, "0", sizeof (char));

This is going to provoke compiler warnings, because write() is declared with __attribute__ ((__warn_unused_result__)). Please use

if (write (fd, "0", sizeof (char)) < 0)
  {
    /* ignore error, this is non-critical */
  }

to show that we've thought about the correct error-handling, or replace the comment with some sort of error-handling if necessary. (But I suspect that the correct error-handling here is indeed to ignore it and just carry on.)
Comment 6 WaLyong Cho 2016-06-23 07:54:17 UTC
Created attachment 124676 [details] [review]
v2: activation: set children oom_score_adj to 0
Comment 7 Simon McVittie 2016-06-23 15:43:52 UTC
Comment on attachment 124676 [details] [review]
v2: activation: set children oom_score_adj to 0

Review of attachment 124676 [details] [review]:
-----------------------------------------------------------------

This looks good, thanks. I'll merge it soon.

I can't help wondering whether this should be in child_setup() in bus/activation.c: it's the dbus-daemon that has elevated OOM-killer protection, so it's the dbus-daemon, not the library code, that should take responsibility for resetting that attribute. But it looks as though we only use dbus-spawn in the dbus-daemon and tests anyway, so no big deal.
Comment 8 Simon McVittie 2016-06-30 13:56:00 UTC
Thanks, fixed in git for 1.10.10, 1.11.4


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.