All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj
@ 2016-06-21  5:30 Ganesh Mahendran
  2016-06-21  5:30 ` [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices Ganesh Mahendran
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-21  5:30 UTC (permalink / raw)
  To: devel, linux-kernel; +Cc: gregkh, arve, riandrews, 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;
                }
---

This patch makes the variable name consistent with the usage.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
 drivers/staging/android/lowmemorykiller.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index 24d2745..6da9260 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -7,9 +7,9 @@
  * /sys/module/lowmemorykiller/parameters/minfree. Both files take a comma
  * separated list of numbers in ascending order.
  *
- * For example, write "0,8" to /sys/module/lowmemorykiller/parameters/adj and
- * "1024,4096" to /sys/module/lowmemorykiller/parameters/minfree to kill
- * processes with a oom_score_adj value of 8 or higher when the free memory
+ * For example, write "0,470" to /sys/module/lowmemorykiller/parameters/score_adj
+ * and "1024,4096" to /sys/module/lowmemorykiller/parameters/minfree to kill
+ * processes with a oom_score_adj value of 470 or higher when the free memory
  * drops below 4096 pages and kill processes with a oom_score_adj value of 0 or
  * higher when the free memory drops below 1024 pages.
  *
@@ -44,14 +44,15 @@
 #include <linux/notifier.h>
 
 static u32 lowmem_debug_level = 1;
-static short lowmem_adj[6] = {
+
+static short lowmem_score_adj[6] = {
 	0,
-	1,
-	6,
-	12,
+	58,
+	352,
+	705,
 };
 
-static int lowmem_adj_size = 4;
+static int lowmem_score_adj_size = 4;
 static int lowmem_minfree[6] = {
 	3 * 512,	/* 6MB */
 	2 * 1024,	/* 8MB */
@@ -89,20 +90,20 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 	int minfree = 0;
 	int selected_tasksize = 0;
 	short selected_oom_score_adj;
-	int array_size = ARRAY_SIZE(lowmem_adj);
+	int array_size = ARRAY_SIZE(lowmem_score_adj);
 	int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages;
 	int other_file = global_page_state(NR_FILE_PAGES) -
 						global_page_state(NR_SHMEM) -
 						total_swapcache_pages();
 
-	if (lowmem_adj_size < array_size)
-		array_size = lowmem_adj_size;
+	if (lowmem_score_adj_size < array_size)
+		array_size = lowmem_score_adj_size;
 	if (lowmem_minfree_size < array_size)
 		array_size = lowmem_minfree_size;
 	for (i = 0; i < array_size; i++) {
 		minfree = lowmem_minfree[i];
 		if (other_free < minfree && other_file < minfree) {
-			min_score_adj = lowmem_adj[i];
+			min_score_adj = lowmem_score_adj[i];
 			break;
 		}
 	}
@@ -165,7 +166,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		if (selected->mm)
 			task_set_lmk_waiting(selected);
 		task_unlock(selected);
-		lowmem_print(1, "Killing '%s' (%d), adj %hd,\n"
+		lowmem_print(1, "Killing '%s' (%d), score adj %hd,\n"
 				 "   to free %ldkB on behalf of '%s' (%d) because\n"
 				 "   cache %ldkB is below limit %ldkB for oom_score_adj %hd\n"
 				 "   Free memory is %ldkB above reserved\n",
@@ -205,7 +206,7 @@ device_initcall(lowmem_init);
  * bootargs behaviour is to continue using module_param here.
  */
 module_param_named(cost, lowmem_shrinker.seeks, int, S_IRUGO | S_IWUSR);
-module_param_array_named(adj, lowmem_adj, short, &lowmem_adj_size,
+module_param_array_named(score_adj, lowmem_score_adj, short, &lowmem_score_adj_size,
 			 S_IRUGO | S_IWUSR);
 module_param_array_named(minfree, lowmem_minfree, uint, &lowmem_minfree_size,
 			 S_IRUGO | S_IWUSR);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
  2016-06-21  5:30 [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj Ganesh Mahendran
@ 2016-06-21  5:30 ` Ganesh Mahendran
  2016-06-21 20:22   ` David Rientjes
  2016-06-21  5:30 ` [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill Ganesh Mahendran
  2016-06-21 20:27 ` [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj David Rientjes
  2 siblings, 1 reply; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-21  5:30 UTC (permalink / raw)
  To: devel, linux-kernel; +Cc: gregkh, arve, riandrews, Ganesh Mahendran

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

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
 drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index 6da9260..1d8de47 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -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;
 }
 
 static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill
  2016-06-21  5:30 [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj Ganesh Mahendran
  2016-06-21  5:30 ` [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices Ganesh Mahendran
@ 2016-06-21  5:30 ` Ganesh Mahendran
  2016-06-21 20:14   ` David Rientjes
  2016-06-21 20:27 ` [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj David Rientjes
  2 siblings, 1 reply; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-21  5:30 UTC (permalink / raw)
  To: devel, linux-kernel; +Cc: gregkh, arve, riandrews, 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.

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

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
 drivers/staging/android/lowmemorykiller.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index 1d8de47..5fcfcfe 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -122,8 +122,6 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		return 0;
 	}
 
-	selected_oom_score_adj = min_score_adj;
-
 	rcu_read_lock();
 	for_each_process(tsk) {
 		struct task_struct *p;
@@ -151,18 +149,19 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		task_unlock(p);
 		if (tasksize <= 0)
 			continue;
-		if (selected) {
-			if (oom_score_adj < selected_oom_score_adj)
-				continue;
-			if (oom_score_adj == selected_oom_score_adj &&
-			    tasksize <= selected_tasksize)
-				continue;
+
+		/*
+		 * From the processes which score adj >= min_score_adj,
+		 * we select the one with the maximum tasksize.
+		 */
+		if (selected_tasksize < tasksize) {
+			selected = p;
+			selected_tasksize = tasksize;
+			selected_oom_score_adj = oom_score_adj;
+
+			lowmem_print(2, "select '%s' (%d), adj %hd, size %d, to kill\n",
+					 p->comm, p->pid, oom_score_adj, tasksize);
 		}
-		selected = p;
-		selected_tasksize = tasksize;
-		selected_oom_score_adj = oom_score_adj;
-		lowmem_print(2, "select '%s' (%d), adj %hd, size %d, to kill\n",
-			     p->comm, p->pid, oom_score_adj, tasksize);
 	}
 	if (selected) {
 		task_lock(selected);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill
  2016-06-21  5:30 ` [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill Ganesh Mahendran
@ 2016-06-21 20:14   ` David Rientjes
  2016-06-22  4:44     ` Ganesh Mahendran
  0 siblings, 1 reply; 11+ messages in thread
From: David Rientjes @ 2016-06-21 20:14 UTC (permalink / raw)
  To: Ganesh Mahendran; +Cc: devel, linux-kernel, gregkh, arve, riandrews

On Tue, 21 Jun 2016, Ganesh Mahendran wrote:

> 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.
> 

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
  2016-06-21  5:30 ` [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices Ganesh Mahendran
@ 2016-06-21 20:22   ` David Rientjes
  2016-06-22  3:27     ` Ganesh Mahendran
  0 siblings, 1 reply; 11+ messages in thread
From: David Rientjes @ 2016-06-21 20:22 UTC (permalink / raw)
  To: Ganesh Mahendran; +Cc: devel, linux-kernel, gregkh, arve, riandrews

On Tue, 21 Jun 2016, Ganesh Mahendran wrote:

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

Why?

> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> ---
>  drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
> index 6da9260..1d8de47 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -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;
>  }
>  
>  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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj
  2016-06-21  5:30 [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj Ganesh Mahendran
  2016-06-21  5:30 ` [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices Ganesh Mahendran
  2016-06-21  5:30 ` [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill Ganesh Mahendran
@ 2016-06-21 20:27 ` David Rientjes
  2016-06-22  3:10   ` Ganesh Mahendran
  2 siblings, 1 reply; 11+ messages in thread
From: David Rientjes @ 2016-06-21 20:27 UTC (permalink / raw)
  To: Ganesh Mahendran; +Cc: devel, linux-kernel, gregkh, arve, riandrews

On Tue, 21 Jun 2016, Ganesh Mahendran wrote:

> 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.
> 

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj
  2016-06-21 20:27 ` [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj David Rientjes
@ 2016-06-22  3:10   ` Ganesh Mahendran
  0 siblings, 0 replies; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-22  3:10 UTC (permalink / raw)
  To: David Rientjes; +Cc: devel, linux-kernel, gregkh, arve, riandrews

Hi, David:

On Tue, Jun 21, 2016 at 01:27:40PM -0700, David Rientjes wrote:
> On Tue, 21 Jun 2016, Ganesh Mahendran wrote:
> 
> > 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.
> > 
> 
> 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.

You are right, this change will break android AMS which will set the LMK
watermark via /sys/module/lowmemorykiller/parameters/adj.

Please help to review below change. Only make the varialbe name consistent
with the variable usage.

------
>From 394872fc1993a04ae471b10d7f971d4544812ec4 Mon Sep 17 00:00:00 2001
From: Ganesh Mahendran <opensource.ganesh@gmail.com>
Date: Wed, 22 Jun 2016 10:53:13 +0800
Subject: [PATCH v2] staging: lowmemorykiller: make variable name consistent with
 the varialbe usage

LMK use oom_score_adj to do the comparing. But the variable name is
*_adj. This patch makes thevarialbe name consistent with the varialb
usage to avoid ambiguity.

*_adj -> *_score_adj

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
v2:
  do not change user API - David
---
 drivers/staging/android/lowmemorykiller.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index 24d2745..6568bbf 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -44,14 +44,15 @@
 #include <linux/notifier.h>
 
 static u32 lowmem_debug_level = 1;
-static short lowmem_adj[6] = {
+
+static short lowmem_score_adj[6] = {
 	0,
-	1,
-	6,
-	12,
+	58,
+	352,
+	705,
 };
 
-static int lowmem_adj_size = 4;
+static int lowmem_score_adj_size = 4;
 static int lowmem_minfree[6] = {
 	3 * 512,	/* 6MB */
 	2 * 1024,	/* 8MB */
@@ -89,20 +90,20 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 	int minfree = 0;
 	int selected_tasksize = 0;
 	short selected_oom_score_adj;
-	int array_size = ARRAY_SIZE(lowmem_adj);
+	int array_size = ARRAY_SIZE(lowmem_score_adj);
 	int other_free = global_page_state(NR_FREE_PAGES) - totalreserve_pages;
 	int other_file = global_page_state(NR_FILE_PAGES) -
 						global_page_state(NR_SHMEM) -
 						total_swapcache_pages();
 
-	if (lowmem_adj_size < array_size)
-		array_size = lowmem_adj_size;
+	if (lowmem_score_adj_size < array_size)
+		array_size = lowmem_score_adj_size;
 	if (lowmem_minfree_size < array_size)
 		array_size = lowmem_minfree_size;
 	for (i = 0; i < array_size; i++) {
 		minfree = lowmem_minfree[i];
 		if (other_free < minfree && other_file < minfree) {
-			min_score_adj = lowmem_adj[i];
+			min_score_adj = lowmem_score_adj[i];
 			break;
 		}
 	}
@@ -165,7 +166,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		if (selected->mm)
 			task_set_lmk_waiting(selected);
 		task_unlock(selected);
-		lowmem_print(1, "Killing '%s' (%d), adj %hd,\n"
+		lowmem_print(1, "Killing '%s' (%d), score adj %hd,\n"
 				 "   to free %ldkB on behalf of '%s' (%d) because\n"
 				 "   cache %ldkB is below limit %ldkB for oom_score_adj %hd\n"
 				 "   Free memory is %ldkB above reserved\n",
@@ -205,7 +206,7 @@ device_initcall(lowmem_init);
  * bootargs behaviour is to continue using module_param here.
  */
 module_param_named(cost, lowmem_shrinker.seeks, int, S_IRUGO | S_IWUSR);
-module_param_array_named(adj, lowmem_adj, short, &lowmem_adj_size,
+module_param_array_named(adj, lowmem_score_adj, short, &lowmem_score_adj_size,
 			 S_IRUGO | S_IWUSR);
 module_param_array_named(minfree, lowmem_minfree, uint, &lowmem_minfree_size,
 			 S_IRUGO | S_IWUSR);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
  2016-06-21 20:22   ` David Rientjes
@ 2016-06-22  3:27     ` Ganesh Mahendran
  2016-06-23  8:42       ` Sergey Senozhatsky
  0 siblings, 1 reply; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-22  3:27 UTC (permalink / raw)
  To: David Rientjes; +Cc: devel, linux-kernel, gregkh, arve, riandrews

Hi, David:

On Tue, Jun 21, 2016 at 01:22:00PM -0700, David Rientjes wrote:
> On Tue, 21 Jun 2016, Ganesh Mahendran wrote:
> 
> > lowmem_count() should only count anon pages when we have swap device.
> > 
> 
> Why?

I make a mistake. I thought lowmem_count will return the shrinkalbe page
of a process.

> 
> > Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> > ---
> >  drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
> > index 6da9260..1d8de47 100644
> > --- a/drivers/staging/android/lowmemorykiller.c
> > +++ b/drivers/staging/android/lowmemorykiller.c
> > @@ -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;
> >  }
> >  
> >  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.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill
  2016-06-21 20:14   ` David Rientjes
@ 2016-06-22  4:44     ` Ganesh Mahendran
  0 siblings, 0 replies; 11+ messages in thread
From: Ganesh Mahendran @ 2016-06-22  4:44 UTC (permalink / raw)
  To: David Rientjes; +Cc: devel, linux-kernel, gregkh, arve, riandrews

Hi, David:

On Tue, Jun 21, 2016 at 01:14:36PM -0700, David Rientjes wrote:
> On Tue, 21 Jun 2016, Ganesh Mahendran wrote:
> 
> > 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.
> > 
> 
> 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).

Yes, from our testing with this patch applied, system works more smoothly,
and user have better experience.

> 
> 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.

I think the processes with score_adj >= min_score_adj are all acceptable
if we kill them. In android products, LMK does the main job to free memory
when system is hard to shrink file/anon pages. If LMK does not free enough memory,
the system will be very slow before OOM is triggered. During this period, user will
have bad experience.

So LMK need to work effectivly to let system running smoothly, as user experience
is very important for android system.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
  2016-06-22  3:27     ` Ganesh Mahendran
@ 2016-06-23  8:42       ` Sergey Senozhatsky
  2016-07-01  2:02         ` Ganesh Mahendran
  0 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-06-23  8:42 UTC (permalink / raw)
  To: Ganesh Mahendran
  Cc: David Rientjes, devel, gregkh, arve, riandrews, linux-kernel,
	sergey.senozhatsky.work, sergey.senozhatsky

On (06/22/16 11:27), Ganesh Mahendran wrote:
[..]
> > > Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> > > ---
> > >  drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
> > > index 6da9260..1d8de47 100644
> > > --- a/drivers/staging/android/lowmemorykiller.c
> > > +++ b/drivers/staging/android/lowmemorykiller.c
> > > @@ -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;
> > >  }
> > >  
> > >  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.

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

	-ss

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
  2016-06-23  8:42       ` Sergey Senozhatsky
@ 2016-07-01  2:02         ` Ganesh Mahendran
  0 siblings, 0 replies; 11+ messages in thread
From: Ganesh Mahendran @ 2016-07-01  2:02 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: David Rientjes, devel, Greg KH, arve, riandrews, linux-kernel,
	Sergey Senozhatsky

2016-06-23 16:42 GMT+08:00 Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com>:
> On (06/22/16 11:27), Ganesh Mahendran wrote:
> [..]
>> > > Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>> > > ---
>> > >  drivers/staging/android/lowmemorykiller.c | 12 ++++++++----
>> > >  1 file changed, 8 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
>> > > index 6da9260..1d8de47 100644
>> > > --- a/drivers/staging/android/lowmemorykiller.c
>> > > +++ b/drivers/staging/android/lowmemorykiller.c
>> > > @@ -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;
>> > >  }
>> > >
>> > >  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.
>
> hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP
> when it has nothing to free instead of 0?

Yes, you are right. It should return SHRINK_STOP.

>
>         -ss

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-07-01  2:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-21  5:30 [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj Ganesh Mahendran
2016-06-21  5:30 ` [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices Ganesh Mahendran
2016-06-21 20:22   ` David Rientjes
2016-06-22  3:27     ` Ganesh Mahendran
2016-06-23  8:42       ` Sergey Senozhatsky
2016-07-01  2:02         ` Ganesh Mahendran
2016-06-21  5:30 ` [PATCH 3/3] staging: lowmemorykiller: select the task with maximum rss to kill Ganesh Mahendran
2016-06-21 20:14   ` David Rientjes
2016-06-22  4:44     ` Ganesh Mahendran
2016-06-21 20:27 ` [PATCH 1/3] staging: lowmemorykiller: change lowmem_adj to lowmem_score_adj David Rientjes
2016-06-22  3:10   ` Ganesh Mahendran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.