Merge lp:~thomas-voss/unity-mir/less-aggressive-scores into lp:unity-mir

[Thomas Voß]

Ensure that app processes are not assigned an oom score smaller than the one of the shell.

Ensure that app processes are not assigned an oom score smaller than the one of the shell.

[Loïc Minier (lool)]

I'd like to suggest two possible improvements:
1)
I think we want the active / front app to be scored less favorably than shell; shell is currently score -10, but I think we should score all apps positively; so I think all apps we track should e.g. start with 10, and get e.g. 800 when they get in the background, then back to 10 if they get in front etc.

-10 would be same score as current shell/session, and I think we prefer the front app to die rather than the shell.

2) could we perhaps use relative percentage scores in the code? e.g. set the mostLikelyToGetKilled to score 80% of range:
((maxValue() - minValue()) * 80 / 100) + minValue()

in fact this would ideally be computed against score of driving process (unity8) vs. max

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[lp:~thomas-voss/unity-mir/less-aggressive-scores]

130. By Thomas Voß on 2013-10-16

Ensure that apps can never gain a better oom score than the parent process.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

OomAdjuster::minValue() not used any more, remove?

Couple of magic numbers in OomScoreAdjuster::mostLikelyToBeKilled(), they have any particular meaning, or just work?

review: Needs Information

[Gerry Boland (gerboland)]

Works as described, please address my queries, then I can approve

[lp:~thomas-voss/unity-mir/less-aggressive-scores]

131. By Thomas Voß on 2013-10-22

Replace magic numbers with meaningful constants and some documentation.

[Thomas Voß (thomas-voss)]

> OomAdjuster::minValue() not used any more, remove?
>
Not used, keeping it for the sake of consistency between OomAdjuster and OomScoreAdjuster.

> Couple of magic numbers in OomScoreAdjuster::mostLikelyToBeKilled(), they have
> any particular meaning, or just work?

Replaced magic numbers with meaningful constants and some documentation. Hope that helps in understanding and future refactoring.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Thanks for addressing that

review: Approve

=== modified file 'src/modules/Unity/Application/taskcontroller.cpp'
--- src/modules/Unity/Application/taskcontroller.cpp	2013-10-15 16:17:37 +0000
+++ src/modules/Unity/Application/taskcontroller.cpp	2013-10-22 05:49:30 +0000
@@ -25,6 +25,9 @@
 // Qt
 #include <QStringList>
 
+// STL
+#include <mutex>
+
 // glib
 #include <glib.h>
 
@@ -52,7 +55,7 @@
  * 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.
  */
@@ -75,7 +78,12 @@
 
     static OomAdjuster leastLikelyToBeKilled()
     {
-        return OomAdjuster(minValue());
+        // By system default, we set the oom_score_adj of Unity8 to -10 (via lightdm).
+        // As we want to avoid that any app's oom_score_adj or oom_adj is <= Unity8's oom_score_adj,
+        // we choose a default value of -9 for oom_score_adj and 0 for oom_adj.
+        static const int default_value = 0;
+
+        return OomAdjuster(default_value);
     }
 
     static OomAdjuster mostLikelyToBeKilled()
@@ -96,7 +104,7 @@
     {
         auto fn = QString("/proc/%1/oom_adj").arg(pid);
         QFile file(fn);
-        
+
         if (!file.open(QIODevice::WriteOnly | QIODevice::Text))
             return false;
 
@@ -185,15 +193,62 @@
 
     static OomScoreAdjuster leastLikelyToBeKilled()
     {
+        // By system default, we set the oom_score_adj of Unity8 to -10 (via lightdm).
+        // As we want to avoid that any app's oom_score_adj is <= Unity8's oom_score_adj,
+        // we choose a default value of -9, and a default increase of +1.
+        static const int default_value = -9;
+        static const int default_increase = 1;
+        
+
         // 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);
+        return OomScoreAdjuster(
+            OomScoreAdjuster::thisProcess().isValid() ?
+            OomScoreAdjuster::thisProcess().value + default_increase :
+            default_value);
     }
 
     static OomScoreAdjuster mostLikelyToBeKilled()
     {
-        return OomScoreAdjuster(maxValue());
+        // We avoid extremal values for oom_score_adj. For that, we
+        // set it to 80% of the total available range. If we cannot
+        // determine the oom_score_adj of the current process, i.e.,
+        // Unity8, we substract -200 by default.
+        static const float default_percentage = 0.8;
+        static const int default_decrease = -200;
+
+        return OomScoreAdjuster(
+            OomScoreAdjuster::thisProcess().isValid() ?
+            (maxValue() - OomScoreAdjuster::thisProcess().value) * default_percentage + OomScoreAdjuster::thisProcess().value :
+            maxValue() - default_decrease);
+    }
+
+    static const OomScoreAdjuster& thisProcess()
+    {
+        // Initialize as invalid.
+        static OomScoreAdjuster adjusterForThisProcess(minValue()-1);
+
+        static std::once_flag query_once;
+        std::call_once(
+            query_once,
+            []()
+            {
+                pid_t pid = getpid();
+
+                auto fn = QString("/proc/%1/oom_score_adj").arg(pid);
+                QFile file(fn);
+
+                if (!file.open(QIODevice::WriteOnly | QIODevice::Text))
+                    return;
+
+                QTextStream in(&file);
+                int value; in >> value;
+
+                adjusterForThisProcess.value = value;
+            });
+
+        return adjusterForThisProcess;
     }
 
     OomScoreAdjuster(int value) : value(value)
@@ -209,7 +264,7 @@
     {
         auto fn = QString("/proc/%1/oom_score_adj").arg(pid);
         QFile file(fn);
-        
+
         if (!file.open(QIODevice::WriteOnly | QIODevice::Text))
             return false;
 
@@ -223,17 +278,17 @@
 };
 
 void ensureProcessIsUnlikelyToBeKilled(pid_t pid)
-{    
+{
     if (!OomScoreAdjuster::leastLikelyToBeKilled().applyForPid(pid))
         if (!OomAdjuster::leastLikelyToBeKilled().applyForPid(pid))
-            DLOG("ensureProcessIsUnlikelyToBeKilled failed");
+            LOG("ensureProcessIsUnlikelyToBeKilled failed");
 }
 
 void ensureProcessIsLikelyToBeKilled(pid_t pid)
 {
     if (!OomScoreAdjuster::mostLikelyToBeKilled().applyForPid(pid))
         if (!OomAdjuster::mostLikelyToBeKilled().applyForPid(pid))
-            DLOG("ensureProcessIsLikelyToBeKilled failed");
+            LOG("ensureProcessIsLikelyToBeKilled failed");
 }
 }