staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj

lowmem_count() should only count anon pages when we have swap device.

Signed-off-by Ganesh Mahendran <[email protected]>

---
 drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/staging/android/lowmemorykiller.c +1 -4

index 6da9260..1d8de47 100644
@@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout;
 static unsigned long lowmem_count(struct shrinker *s,
 				  struct shrink_control *sc)
 {
-	return global_page_state(NR_ACTIVE_ANON) +
-		global_page_state(NR_ACTIVE_FILE) +
-		global_page_state(NR_INACTIVE_ANON) +
-		global_page_state(NR_INACTIVE_FILE);
+	unsigned long freeable = global_page_state(NR_ACTIVE_FILE) +

Quoted 2 lines · Ganesh Mahendran

lowmem_count() should only count anon pages when we have swap device.

Why?

Quoted 28 lines

Signed-off-by Ganesh Mahendran <[email protected]>

---
 drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/staging/android/lowmemorykiller.c +8 -4

index 6da9260..1d8de47 100644
@@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout;
 static unsigned long lowmem_count(struct shrinker *s,
 				  struct shrink_control *sc)
 {
-	return global_page_state(NR_ACTIVE_ANON) +
-		global_page_state(NR_ACTIVE_FILE) +
-		global_page_state(NR_INACTIVE_ANON) +
-		global_page_state(NR_INACTIVE_FILE);
+	unsigned long freeable = global_page_state(NR_ACTIVE_FILE) +
+				global_page_state(NR_INACTIVE_FILE);
+
+	if (get_nr_swap_pages() > 0)
+		freeable += global_page_state(NR_ACTIVE_ANON) +
+				global_page_state(NR_INACTIVE_ANON);
+
+	return freeable;
 }
 

drivers/staging/android/lowmemorykiller.c static unsigned long lowmem_deathpending_timeout; · line 86

Quoted 1 line

drivers/staging/android/lowmemorykiller.c +0 -0

 static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)

Shouldn't this be advertising the amount of memory that is freeable by killing the process with the highest priority oom_score_adj? It's not legitimate to say it can free all anon and file memory if nothing is oom killable, so this function is wrong both originally and with your patched version.

Quoted 27 lines

Quoted 27 lines

Signed-off-by Ganesh Mahendran <[email protected]>

---
 drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/staging/android/lowmemorykiller.c +8 -4

index 6da9260..1d8de47 100644
@@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout;
 static unsigned long lowmem_count(struct shrinker *s,
 				  struct shrink_control *sc)
 {
-	return global_page_state(NR_ACTIVE_ANON) +
-		global_page_state(NR_ACTIVE_FILE) +
-		global_page_state(NR_INACTIVE_ANON) +
-		global_page_state(NR_INACTIVE_FILE);
+	unsigned long freeable = global_page_state(NR_ACTIVE_FILE) +
+				global_page_state(NR_INACTIVE_FILE);
+
+	if (get_nr_swap_pages() > 0)
+		freeable += global_page_state(NR_ACTIVE_ANON) +
+				global_page_state(NR_INACTIVE_ANON);
+
+	return freeable;
 }
 

Quoted 7 lines

drivers/staging/android/lowmemorykiller.c static unsigned long lowmem_deathpending_timeout; · line 86

Quoted 1 line

drivers/staging/android/lowmemorykiller.c +0 -0

 static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)

Shouldn't this be advertising the amount of memory that is freeable by killing the process with the highest priority oom_score_adj? It's not legitimate to say it can free all anon and file memory if nothing is oom killable, so this function is wrong both originally and with your patched version.

Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? Then lowmem_scan() will do the real job to scan all the process.

[..]

Quoted 35 lines

Quoted 34 lines

Quoted 18 lines

Signed-off-by Ganesh Mahendran <[email protected]>

---
 drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/staging/android/lowmemorykiller.c +1 -4

index 6da9260..1d8de47 100644
@@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout;
 static unsigned long lowmem_count(struct shrinker *s,
                            struct shrink_control *sc)
 {
- return global_page_state(NR_ACTIVE_ANON) +
-         global_page_state(NR_ACTIVE_FILE) +
-         global_page_state(NR_INACTIVE_ANON) +
-         global_page_state(NR_INACTIVE_FILE);
+ unsigned long freeable = global_page_state(NR_ACTIVE_FILE) +

drivers/staging/android/lowmemorykiller.c static unsigned long lowmem_deathpending_timeout; · line 86

Quoted 10 lines

drivers/staging/android/lowmemorykiller.c +7 -0

+                         global_page_state(NR_INACTIVE_FILE);
+
+ if (get_nr_swap_pages() > 0)
+         freeable += global_page_state(NR_ACTIVE_ANON) +
+                         global_page_state(NR_INACTIVE_ANON);
+
+ return freeable;
 }
 
 static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)

Shouldn't this be advertising the amount of memory that is freeable by killing the process with the highest priority oom_score_adj? It's not legitimate to say it can free all anon and file memory if nothing is oom killable, so this function is wrong both originally and with your patched version.

Quoted 2 lines

Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? Then lowmem_scan() will do the real job to scan all the process.

hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP when it has nothing to free instead of 0?

        -ss

Current task selecting logic in LMK does not fully aware of the memory pressure. It may select the task with maximum score adj, but with least tasksize.

For example, if min_score_adj is 200, and there are 2 tasks in system:
task a: score adj 500, tasksize 200M
task b: score adj 1000, tasksize 1M
Current LMK logic will select *task b*. But now the system already have much memory pressure.

We should select the task with maximum task from all the tasks which score adj >= min_score_adj.

Quoted 10 lines · Ganesh Mahendran

Current task selecting logic in LMK does not fully aware of the memory pressure. It may select the task with maximum score adj, but with least tasksize.

For example, if min_score_adj is 200, and there are 2 tasks in system:
task a: score adj 500, tasksize 200M
task b: score adj 1000, tasksize 1M
Current LMK logic will select *task b*. But now the system already have much memory pressure.

Quoted 3 lines

We should select the task with maximum task from all the tasks which score adj >= min_score_adj.

Unfortunately, I'm not sure that we can get away with this although I agree that it is a better result (kill a large process, avoid lowmem or oom for longer).

It changes the kill order for systems that have already fine-tuned their oom_score_adj settings and can regress because of this change. If systems really want task b to be killed above, this breaks and they have no immediate way of fixing it.

om_adj is deprecated, and in lowmemorykiller module, we use score adj to do the comparing.
---

                oom_score_adj = p->signal->oom_score_adj;
                if (oom_score_adj < min_score_adj) {
                        task_unlock(p);
                        continue;
                }

---

This patch makes the variable name consistent with the usage.

Quoted 10 lines · Ganesh Mahendran

om_adj is deprecated, and in lowmemorykiller module, we use score adj to do the comparing.
---

                oom_score_adj = p->signal->oom_score_adj;
                if (oom_score_adj < min_score_adj) {
                        task_unlock(p);
                        continue;
                }

---

Quoted 2 lines

This patch makes the variable name consistent with the usage.

Umm, I don't think you can just remove a parameter to a module and replace it with something that has a different unit and not think that userspace will break as a result.

From 394872fc1993a04ae471b10d7f971d4544812ec4 Mon Sep 17 00:00:00 2001