Merge lp:~thomas-voss/unity-mir/oom_adjust into lp:unity-mir

[Thomas Voß]

Slightly refactor the OOM adjustments to be a pure implementation detail of the TaskManager.

Slightly refactor the OOM adjustments to be a pure implementation detail of the TaskManager.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:121
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/125/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-amd64-ci/67
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/125
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/125/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-i386-ci/125

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity-mir-ci/125/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

+struct OomAdj
+struct OomScoreAdj
Please don't use the "Adj" abbreviation, it's vague. I'd recommend OomAdjuster.

+ static int disableOomKillerValue()
+ {
+ return -17;
+ }
Why not just a const static int disableOomKillerValue = -17; Similarly for the other *Value() methods.

+ static int {min,max}Value()
I'd prefer a more descriptive name like "{least,most}LikelyToKillValue"

+ static QString filePattern()
+ {
+ return QString("/proc/%1/oom_adj");
+ }
Why this in separate public method? It's only used in applyForPid, and is internal detail.

+ throw std::runtime_error("OomScoreAdj created with invalid value.");
This exception is not caught anywhere, as we tend to avoid using exceptions in our Qt code.

+struct OomScoreAdj
+ static OomScoreAdj leastLikelyToBeKilled()
+ {
+ // We could be way more clever here if we knew the distribution
+ // of oom_score_adj values of all app processes. However, we just
+ // make sure that the process is not ignored by the oom killer for now.
+ return OomScoreAdj(minValue()+200);
+ }
I'd rather you move this comment beside minValue(), and have minValue=-800 for now.

focusCallback=....
+ DLOG("startCallback: Could not oom killer probability for process");
Copy/paste typo in the DLOG

TaskController::resume...
+ DLOG("TaskController::suspend: Could not oom killer probability for process");
Again, c/p typo

This is a lot of code to do something relatively simple. While I like the clear structuring and documentation, creating an object which just writes a set of values to a file, and a separate object for slightly different values (but with same meaning) to a different file, is overkill IMO.

A singleton class is enough to do this, which would internally handle both proc files and their respective values, and so would remove this complexity from TaskManager's code:

+ if (!updateOomScoreAdj(pid, OomScoreAdj::leastLikelyToBeKilled()))
+ {
+ if (!updateOomAdj(pid, OomAdj::leastLikelyToBeKilled()))
+ DLOG("startCallback: Could not oom killer probability for process");
+ }

reducing it to just:

if (!OomAdjuster::instance()->set(pid, OomAdjuster::leastLikelyToBeKilled)
    DLOG("startCallback: Could not set OOM killer probability for process");

review: Needs Fixing

[Thomas Voß (thomas-voss)]

> +struct OomAdj
> +struct OomScoreAdj
> Please don't use the "Adj" abbreviation, it's vague. I'd recommend
> OomAdjuster.
>
> + static int disableOomKillerValue()
> + {
> + return -17;
> + }
> Why not just a const static int disableOomKillerValue = -17; Similarly for the
> other *Value() methods.
>

As static constants are less likely to be hit by static initialization fiascos.

> + static int {min,max}Value()
> I'd prefer a more descriptive name like "{least,most}LikelyToKillValue"
>

Hmmm, I intentionally named them min and max, and introduced two more semantic functions to distinguish between what the kernel oom considers min and max and how it is interpreted, respectively.

> + static QString filePattern()
> + {
> + return QString("/proc/%1/oom_adj");
> + }
> Why this in separate public method? It's only used in applyForPid, and is
> internal detail.

yup, fair point.

>
> + throw std::runtime_error("OomScoreAdj created with invalid value.");
> This exception is not caught anywhere, as we tend to avoid using exceptions in
> our Qt code.
>
> +struct OomScoreAdj
> + static OomScoreAdj leastLikelyToBeKilled()
> + {
> + // We could be way more clever here if we knew the distribution
> + // of oom_score_adj values of all app processes. However, we just
> + // make sure that the process is not ignored by the oom killer for now.
> + return OomScoreAdj(minValue()+200);
> + }
> I'd rather you move this comment beside minValue(), and have minValue=-800 for
> now.
>

As explained before, minValue returns the value as defined in linux/oom.h and there is a significant difference between the min value and our interpretation of least likely to be killed.

> focusCallback=....
> + DLOG("startCallback: Could not oom killer probability for process");
> Copy/paste typo in the DLOG
>
> TaskController::resume...
> + DLOG("TaskController::suspend: Could not oom killer probability for
> process");
> Again, c/p typo
>
>
>
> This is a lot of code to do something relatively simple. While I like the
> clear structuring and documentation, creating an object which just writes a
> set of values to a file, and a separate object for slightly different values
> (but with same meaning) to a different file, is overkill IMO.
>
> A singleton class is enough to do this, which would internally handle both
> proc files and their respective values, and so would remove this complexity
> from TaskManager's code:
>

I think we should stop using an anti-pattern like a Singleton _now_. It leads to tight coupling and is increasingly difficult to test. That being said, I do agree that we can further condense updateOom{Score}Adj.

> + if (!updateOomScoreAdj(pid, OomScoreAdj::leastLikelyToBeKilled()))
> + {
> + if (!updateOomAdj(pid, OomAdj::leastLikelyToBeKilled()))
> + DLOG("startCallback: Could not oom killer probability for process");
> + }
>
> reducing it to just:
>
> if (!OomAdjuster::instance()->set(pid, OomAdjuster::leastLikelyToBeKilled)
> DLOG("startCallback: Could not set OOM killer probability for process");

[lp:~thomas-voss/unity-mir/oom_adjust]

122. By Thomas Voß on 2013-10-15

Address reviewer comments.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:122
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/126/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-amd64-ci/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/126
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/126/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-i386-ci/126

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity-mir-ci/126/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

> > Please don't use the "Adj" abbreviation, it's vague. I'd recommend OomAdjuster.
Please address.

> As static constants are less likely to be hit by static initialization fiascos.
I don't understand this, these are integers, not static classes that have some interdependency.

> > + throw std::runtime_error("OomScoreAdj created with invalid value.");
> > This exception is not caught anywhere, as we tend to avoid using exceptions
> > in our Qt code.
I badly phrased this, I wanted you to remove the exception entirely as Qt is not fully exception safe: https://qt-project.org/doc/qt-5.0/qtdoc/exceptionsafety.html and so we avoid their use.

[lp:~thomas-voss/unity-mir/oom_adjust]

123. By Thomas Voß on 2013-10-15

 * Rename Adj -> Adjuster as per review request.
 * Do not throw on construction from invalid value.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:123
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/128/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-amd64-ci/70
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/128
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/128/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-i386-ci/128

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity-mir-ci/128/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

review: Approve

=== modified file 'src/modules/Unity/Application/taskcontroller.cpp'
--- src/modules/Unity/Application/taskcontroller.cpp	2013-09-26 15:36:27 +0000
+++ src/modules/Unity/Application/taskcontroller.cpp	2013-10-15 16:19:00 +0000
@@ -32,6 +32,210 @@
 #include <signal.h>
 #include <unistd.h>
 
+// linux specific
+#include <linux/oom.h>
+
+namespace
+{
+/**
+ * From man proc:
+ *
+ * This file can be used to adjust the score used to select which
+ * process should be killed in an out-of-memory (OOM) situation.  The
+ * kernel uses this value for a bit-shift opera‐ tion of the process's
+ * oom_score value: valid values are in the range -16 to +15, plus the
+ * special value -17, which disables OOM-killing altogether for this
+ * process.  A posi‐ tive score increases the likelihood of this process
+ * being killed by the OOM-killer; a negative score decreases the
+ * likelihood.
+ *
+ * The default value for this file is 0; a new process inherits its
+ * parent's oom_adj setting.  A process must be privileged
+ * (CAP_SYS_RESOURCE) to update this file.
+ 
+ * Since Linux 2.6.36, use of this file is deprecated in favor of
+ * /proc/[pid]/oom_score_adj.
+ */
+struct OomAdjuster
+{
+    static int disableOomKillerValue()
+    {
+        return OOM_DISABLE;
+    }
+
+    static int minValue()
+    {
+        return OOM_ADJUST_MIN;
+    }
+
+    static int maxValue()
+    {
+        return OOM_ADJUST_MAX;
+    }
+
+    static OomAdjuster leastLikelyToBeKilled()
+    {
+        return OomAdjuster(minValue());
+    }
+
+    static OomAdjuster mostLikelyToBeKilled()
+    {
+        return OomAdjuster(maxValue());
+    }
+
+    OomAdjuster(int value) : value(value)
+    {
+    }
+
+    bool isValid() const
+    {
+        return !(value < disableOomKillerValue() || value > maxValue());
+    }
+
+    bool applyForPid(pid_t pid) const
+    {
+        auto fn = QString("/proc/%1/oom_adj").arg(pid);
+        QFile file(fn);
+        
+        if (!file.open(QIODevice::WriteOnly | QIODevice::Text))
+            return false;
+
+        QTextStream out(&file);
+        out << value;
+
+        return true;
+    }
+
+    int value;
+};
+
+/**
+ * From man proc:
+ *
+ *  This file can be used to adjust the badness heuristic used to
+ *  select which process gets killed in out-of-memory conditions.
+ *
+ *  The badness heuristic assigns a value to each candidate
+ *  task ranging from 0 (never kill) to 1000 (always kill)
+ *  to determine which process is targeted.  The units are
+ *  roughly a proportion along that range of allowed memory
+ *  the process may allocate from, based on an estimation of
+ *  its current memory and swap use.  For example, if a task
+ *  is using all allowed memory, its badness score will be
+ *  1000.  If it is using half of its allowed memory, its
+ *  score will be 500.
+ *
+ *  There is an additional factor included in the badness score: root
+ *  processes are given 3% extra memory over other tasks.
+ *
+ *  The amount of "allowed" memory depends on the context in
+ *  which the OOM-killer was called.  If it is due to the
+ *  memory assigned to the allocating task's cpuset being
+ *  exhausted, the allowed memory represents the set of mems
+ *  assigned to that cpuset (see cpuset(7)).  If it is due
+ *  to a mempolicy's node(s) being exhausted, the allowed
+ *  memory represents the set of mempolicy nodes.  If it is
+ *  due to a memory limit (or swap limit) being reached, the
+ *  allowed memory is that configured limit.  Finally, if it
+ *  is due to the entire system being out of memory, the
+ *  allowed memory represents all allocatable resources.
+ *
+ *  The value of oom_score_adj is added to the badness score before it
+ *  is used to determine which task to kill.  Acceptable values range
+ *  from -1000 (OOM_SCORE_ADJ_MIN) to +1000 (OOM_SCORE_ADJ_MAX).  This
+ *  allows user space to control the preference for OOM-killing,
+ *  ranging from always preferring a certain task or completely
+ *  disabling it from OOM- killing.  The lowest possible value, -1000,
+ *  is equivalent to disabling OOM-killing entirely for that task,
+ *  since it will always report a badness score of 0.
+ *
+ *  Consequently, it is very simple for user space to define the amount
+ *  of memory to consider for each task.  Setting a oom_score_adj value
+ *  of +500, for example, is roughly equiv‐ alent to allowing the
+ *  remainder of tasks sharing the same system, cpuset, mempolicy, or
+ *  memory controller resources to use at least 50% more memory.  A
+ *  value of -500, on the other hand, would be roughly equivalent to
+ *  discounting 50% of the task's allowed memory from being considered
+ *  as scoring against the task.
+ *
+ *  For backward compatibility with previous kernels,
+ *  /proc/[pid]/oom_adj can still be used to tune the badness score.
+ *  Its value is scaled linearly with oom_score_adj.
+ *
+ *  Writing to /proc/[pid]/oom_score_adj or
+ *  /proc/[pid]/oom_adj will change the other with its
+ *  scaled value.
+ */
+struct OomScoreAdjuster
+{
+    static int disableOomKillerValue()
+    {
+        return OOM_SCORE_ADJ_MIN;
+    }
+
+    static int minValue()
+    {
+        return OOM_SCORE_ADJ_MIN;
+    }
+
+    static int maxValue()
+    {
+        return OOM_SCORE_ADJ_MAX;
+    }
+
+    static OomScoreAdjuster leastLikelyToBeKilled()
+    {
+        // We could be way more clever here if we knew the distribution
+        // of oom_score_adj values of all app processes. However, we just
+        // make sure that the process is not ignored by the oom killer for now.
+        return OomScoreAdjuster(minValue()+200);
+    }
+
+    static OomScoreAdjuster mostLikelyToBeKilled()
+    {
+        return OomScoreAdjuster(maxValue());
+    }
+
+    OomScoreAdjuster(int value) : value(value)
+    {
+    }
+
+    bool isValid() const
+    {
+        return !(value < disableOomKillerValue() || value > maxValue());
+    }
+
+    bool applyForPid(pid_t pid) const
+    {
+        auto fn = QString("/proc/%1/oom_score_adj").arg(pid);
+        QFile file(fn);
+        
+        if (!file.open(QIODevice::WriteOnly | QIODevice::Text))
+            return false;
+
+        QTextStream out(&file);
+        out << value;
+
+        return true;
+    }
+
+    int value;
+};
+
+void ensureProcessIsUnlikelyToBeKilled(pid_t pid)
+{    
+    if (!OomScoreAdjuster::leastLikelyToBeKilled().applyForPid(pid))
+        if (!OomAdjuster::leastLikelyToBeKilled().applyForPid(pid))
+            DLOG("ensureProcessIsUnlikelyToBeKilled failed");
+}
+
+void ensureProcessIsLikelyToBeKilled(pid_t pid)
+{
+    if (!OomScoreAdjuster::mostLikelyToBeKilled().applyForPid(pid))
+        if (!OomAdjuster::mostLikelyToBeKilled().applyForPid(pid))
+            DLOG("ensureProcessIsLikelyToBeKilled failed");
+}
+}
 
 TaskController* TaskController::m_theTaskController = nullptr;
 
@@ -48,6 +252,8 @@
 {
     startCallback = [](const gchar * appId, gpointer userData) {
         Q_UNUSED(userData)
+        pid_t pid = upstart_app_launch_get_primary_pid(appId);
+        ensureProcessIsUnlikelyToBeKilled(pid);
         Q_EMIT TaskController::singleton()->processStartReport(QString(appId), false);
     };
 
@@ -58,6 +264,8 @@
 
     focusCallback = [](const gchar * appId, gpointer userData) {
         Q_UNUSED(userData)
+        pid_t pid = upstart_app_launch_get_primary_pid(appId);
+        ensureProcessIsUnlikelyToBeKilled(pid);
         Q_EMIT TaskController::singleton()->requestFocus(QString(appId));
     };
 
@@ -137,6 +345,8 @@
     DLOG("TaskController::suspend (this=%p, application=%p)", this, qPrintable(appId));
     pid_t pid = upstart_app_launch_get_primary_pid(appId.toLatin1().constData());
 
+    ensureProcessIsLikelyToBeKilled(pid);
+
     if (pid) {
         kill(pid, SIGSTOP);
         return true;
@@ -150,6 +360,8 @@
     DLOG("TaskController::resume (this=%p, application=%p)", this, qPrintable(appId));
     pid_t pid = upstart_app_launch_get_primary_pid(appId.toLatin1().constData());
 
+    ensureProcessIsUnlikelyToBeKilled(pid);
+
     if (pid) {
         kill(pid, SIGCONT);
         return true;