[PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[Chintan Pandya]

The global shrinker will invoke lowmem_shrink in a loop.
The loop will be run (total_scan_pages/batch_size) times.
The default batch_size will be 128 which will make
shrinker invoking 100s of times. LMK does meaningful
work only during first 2-3 times and then rest of the
invocations are just CPU cycle waste. Fix that by returning
to the shrinker with SHRINK_STOP when LMK doesn't find any
more work to do. The deciding factor here is, no process
found in the selected LMK bucket or memory conditions are
sane.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 drivers/staging/android/lowmemorykiller.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index b545d3d..5bf483f 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -110,7 +110,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 	if (min_score_adj == OOM_SCORE_ADJ_MAX + 1) {
 		lowmem_print(5, "lowmem_scan %lu, %x, return 0\n",
 			     sc->nr_to_scan, sc->gfp_mask);
-		return 0;
+		return SHRINK_STOP;
 	}
 
 	selected_oom_score_adj = min_score_adj;
@@ -163,6 +163,9 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		set_tsk_thread_flag(selected, TIF_MEMDIE);
 		send_sig(SIGKILL, selected, 0);
 		rem += selected_tasksize;
+	} else {
+		rcu_read_unlock();
+		return SHRINK_STOP;
 	}
 
 	lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",
-- 
Chintan Pandya

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[Michal Hocko]

On Mon 12-01-15 21:49:14, Chintan Pandya wrote:
> The global shrinker will invoke lowmem_shrink in a loop.
> The loop will be run (total_scan_pages/batch_size) times.
> The default batch_size will be 128 which will make
> shrinker invoking 100s of times. LMK does meaningful
> work only during first 2-3 times and then rest of the
> invocations are just CPU cycle waste. Fix that by returning
> to the shrinker with SHRINK_STOP when LMK doesn't find any
> more work to do. The deciding factor here is, no process
> found in the selected LMK bucket or memory conditions are
> sane.

lowmemory killer is broken by design and this one of the examples which
shows why. It simply doesn't fit into shrinkers concept.

The count_object callback simply lies and tells the core that all
the reclaimable LRU pages are scanable and gives it this as a number
which the core uses for total_scan. scan_objects callback then happily
ignore nr_to_reclaim and does its one time job where it iterates over
_all_ tasks and picks up the victim and returns its rss as a return
value. This is just a subset of LRU pages of course so it continues
looping until total_scan goes down to 0 finally.

If this really has to be a shrinker then, shouldn't it evaluate the OOM
situation in the count callback and return non zero only if OOM and then
the scan callback would kill and return nr_to_reclaim.

Or even better wouldn't it be much better to use vmpressure to wake
up a kernel module which would simply check the situation and kill
something?

Please do not put only cosmetic changes on top of broken concept and try
to think about a proper solution that is what staging is for AFAIU.

The code is in this state for quite some time and I would really hate if
it got merged just because it is in staging for too long and it is used
out there.

> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> ---
>  drivers/staging/android/lowmemorykiller.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
> index b545d3d..5bf483f 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -110,7 +110,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
>  	if (min_score_adj == OOM_SCORE_ADJ_MAX + 1) {
>  		lowmem_print(5, "lowmem_scan %lu, %x, return 0\n",
>  			     sc->nr_to_scan, sc->gfp_mask);
> -		return 0;
> +		return SHRINK_STOP;
>  	}
>  
>  	selected_oom_score_adj = min_score_adj;
> @@ -163,6 +163,9 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
>  		set_tsk_thread_flag(selected, TIF_MEMDIE);
>  		send_sig(SIGKILL, selected, 0);
>  		rem += selected_tasksize;
> +	} else {
> +		rcu_read_unlock();
> +		return SHRINK_STOP;
>  	}
>  
>  	lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",
> -- 
> Chintan Pandya
> 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[John Stultz]

On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko <mhocko@suse.cz> wrote:
> On Mon 12-01-15 21:49:14, Chintan Pandya wrote:
>> The global shrinker will invoke lowmem_shrink in a loop.
>> The loop will be run (total_scan_pages/batch_size) times.
>> The default batch_size will be 128 which will make
>> shrinker invoking 100s of times. LMK does meaningful
>> work only during first 2-3 times and then rest of the
>> invocations are just CPU cycle waste. Fix that by returning
>> to the shrinker with SHRINK_STOP when LMK doesn't find any
>> more work to do. The deciding factor here is, no process
>> found in the selected LMK bucket or memory conditions are
>> sane.
>
> lowmemory killer is broken by design and this one of the examples which
> shows why. It simply doesn't fit into shrinkers concept.
>
> The count_object callback simply lies and tells the core that all
> the reclaimable LRU pages are scanable and gives it this as a number
> which the core uses for total_scan. scan_objects callback then happily
> ignore nr_to_reclaim and does its one time job where it iterates over
> _all_ tasks and picks up the victim and returns its rss as a return
> value. This is just a subset of LRU pages of course so it continues
> looping until total_scan goes down to 0 finally.
>
> If this really has to be a shrinker then, shouldn't it evaluate the OOM
> situation in the count callback and return non zero only if OOM and then
> the scan callback would kill and return nr_to_reclaim.
>
> Or even better wouldn't it be much better to use vmpressure to wake
> up a kernel module which would simply check the situation and kill
> something?
>
> Please do not put only cosmetic changes on top of broken concept and try
> to think about a proper solution that is what staging is for AFAIU.
>
> The code is in this state for quite some time and I would really hate if
> it got merged just because it is in staging for too long and it is used
> out there.

So the in-kernel low-memory-killer is hopefully on its way out.

With Lollipop on some devices, Android is using the mempressure
notifiers to kill processes from userland. However, not all devices
have moved to this new model (and possibly some resulting performance
issues are being worked out? Its not clear).  So hopefully we can drop
it soon, but I'd like to make sure we don't get only a half-working
solution upstream before we do remove it.

thanks
-john

Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[Rom Lemarchand]

On Thu, Jan 29, 2015 at 4:44 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko <mhocko@suse.cz> wrote:
>> On Mon 12-01-15 21:49:14, Chintan Pandya wrote:
>>> The global shrinker will invoke lowmem_shrink in a loop.
>>> The loop will be run (total_scan_pages/batch_size) times.
>>> The default batch_size will be 128 which will make
>>> shrinker invoking 100s of times. LMK does meaningful
>>> work only during first 2-3 times and then rest of the
>>> invocations are just CPU cycle waste. Fix that by returning
>>> to the shrinker with SHRINK_STOP when LMK doesn't find any
>>> more work to do. The deciding factor here is, no process
>>> found in the selected LMK bucket or memory conditions are
>>> sane.
>>
>> lowmemory killer is broken by design and this one of the examples which
>> shows why. It simply doesn't fit into shrinkers concept.
>>
>> The count_object callback simply lies and tells the core that all
>> the reclaimable LRU pages are scanable and gives it this as a number
>> which the core uses for total_scan. scan_objects callback then happily
>> ignore nr_to_reclaim and does its one time job where it iterates over
>> _all_ tasks and picks up the victim and returns its rss as a return
>> value. This is just a subset of LRU pages of course so it continues
>> looping until total_scan goes down to 0 finally.
>>
>> If this really has to be a shrinker then, shouldn't it evaluate the OOM
>> situation in the count callback and return non zero only if OOM and then
>> the scan callback would kill and return nr_to_reclaim.
>>
>> Or even better wouldn't it be much better to use vmpressure to wake
>> up a kernel module which would simply check the situation and kill
>> something?
>>
>> Please do not put only cosmetic changes on top of broken concept and try
>> to think about a proper solution that is what staging is for AFAIU.
>>
>> The code is in this state for quite some time and I would really hate if
>> it got merged just because it is in staging for too long and it is used
>> out there.
>
> So the in-kernel low-memory-killer is hopefully on its way out.
>
> With Lollipop on some devices, Android is using the mempressure
> notifiers to kill processes from userland. However, not all devices
> have moved to this new model (and possibly some resulting performance
> issues are being worked out? Its not clear).  So hopefully we can drop
> it soon, but I'd like to make sure we don't get only a half-working
> solution upstream before we do remove it.
>
> thanks
> -john

We are still working on a user space replacement to LMK. We have
definitely had issues with LMKd and so stayed with the in kernel one
for all the lollipop devices we shipped. Issues were mostly related to
performance, timing of OOM notifications and when under intense memory
pressure we ran into issues where even opening a file would fail due
to no RAM being available.
As John said, it's WIP and hopefully we'll be able to drop the in
kernel one soon.

Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[Chintan Pandya]

Please ignore this patch. My extreme bad that I merged commit messages 
applicable to some very old kernel into this patch. Updating shortly.

On 01/12/2015 09:38 PM, Chintan Pandya wrote:
> The global shrinker will invoke lowmem_shrink in a loop.
> The loop will be run (total_scan_pages/batch_size) times.
> The default batch_size will be 128 which will make
> shrinker invoking 100s of times. LMK does meaningful
> work only during first 2-3 times and then rest of the
> invocations are just CPU cycle waste. Fix that by giving
> excessively large batch size so that lowmem_shrink will
> be called just once and in the same try LMK does the
> needful.
>
> Signed-off-by: Chintan Pandya<cpandya@codeaurora.org>
> ---
>   drivers/staging/android/lowmemorykiller.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
> index b545d3d..5bf483f 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -110,7 +110,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
>   	if (min_score_adj == OOM_SCORE_ADJ_MAX + 1) {
>   		lowmem_print(5, "lowmem_scan %lu, %x, return 0\n",
>   			     sc->nr_to_scan, sc->gfp_mask);
> -		return 0;
> +		return SHRINK_STOP;
>   	}
>
>   	selected_oom_score_adj = min_score_adj;
> @@ -163,6 +163,9 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
>   		set_tsk_thread_flag(selected, TIF_MEMDIE);
>   		send_sig(SIGKILL, selected, 0);
>   		rem += selected_tasksize;
> +	} else {
> +		rcu_read_unlock();
> +		return SHRINK_STOP;
>   	}
>
>   	lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",


-- 
Chintan Pandya

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

[PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[Chintan Pandya]

The global shrinker will invoke lowmem_shrink in a loop.
The loop will be run (total_scan_pages/batch_size) times.
The default batch_size will be 128 which will make
shrinker invoking 100s of times. LMK does meaningful
work only during first 2-3 times and then rest of the
invocations are just CPU cycle waste. Fix that by giving
excessively large batch size so that lowmem_shrink will
be called just once and in the same try LMK does the
needful.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 drivers/staging/android/lowmemorykiller.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index b545d3d..5bf483f 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -110,7 +110,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 	if (min_score_adj == OOM_SCORE_ADJ_MAX + 1) {
 		lowmem_print(5, "lowmem_scan %lu, %x, return 0\n",
 			     sc->nr_to_scan, sc->gfp_mask);
-		return 0;
+		return SHRINK_STOP;
 	}
 
 	selected_oom_score_adj = min_score_adj;
@@ -163,6 +163,9 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 		set_tsk_thread_flag(selected, TIF_MEMDIE);
 		send_sig(SIGKILL, selected, 0);
 		rem += selected_tasksize;
+	} else {
+		rcu_read_unlock();
+		return SHRINK_STOP;
 	}
 
 	lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",
-- 
Chintan Pandya

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation