Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Michal Hocko]

On Wed 02-09-15 18:06:20, Greg KH wrote:
[...]
> And if we aren't taking patch 1/2, I guess this one isn't needed either?

Unlike the patch1 which was pretty much cosmetic this fixes a real
issue.
-- 
Michal Hocko
SUSE Labs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Greg KH]

On Fri, Sep 04, 2015 at 04:05:59PM +0200, Michal Hocko wrote:
> On Wed 02-09-15 18:06:20, Greg KH wrote:
> [...]
> > And if we aren't taking patch 1/2, I guess this one isn't needed either?
> 
> Unlike the patch1 which was pretty much cosmetic this fixes a real
> issue.

Ok, then it would be great to get this in a format that I can apply it
in :)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Tetsuo Handa]

Greg KH wrote:
> On Fri, Sep 04, 2015 at 04:05:59PM +0200, Michal Hocko wrote:
> > On Wed 02-09-15 18:06:20, Greg KH wrote:
> > [...]
> > > And if we aren't taking patch 1/2, I guess this one isn't needed either?
> > 
> > Unlike the patch1 which was pretty much cosmetic this fixes a real
> > issue.
> 
> Ok, then it would be great to get this in a format that I can apply it
> in :)

I see. Here is a minimal patch.
(Acked-by: from http://lkml.kernel.org/r/20150827084443.ge14...@dhcp22.suse.cz )

>From 118609fa25700af11791b1b7e8349f8973a9e7e4 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa 
Date: Sat, 5 Sep 2015 02:58:12 +0900
Subject: [PATCH] android, lmk: Send SIGKILL before setting TIF_MEMDIE.

It was observed that setting TIF_MEMDIE before sending SIGKILL at
oom_kill_process() allows memory reserves to be depleted by allocations
which are not needed for terminating the OOM victim.

This patch reverts commit 6bc2b856bb7c ("staging: android: lowmemorykiller:
set TIF_MEMDIE before send kill sig"), for oom_kill_process() was updated
to send SIGKILL before setting TIF_MEMDIE.

Signed-off-by: Tetsuo Handa 
Acked-by: Michal Hocko 
---
 drivers/staging/android/lowmemorykiller.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c 
b/drivers/staging/android/lowmemorykiller.c
index 872bd60..569d12c 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -157,26 +157,22 @@ static unsigned long lowmem_scan(struct shrinker *s, 
struct shrink_control *sc)
}
if (selected) {
task_lock(selected);
-   if (!selected->mm) {
-   /* Already exited, cannot do mark_tsk_oom_victim() */
-   task_unlock(selected);
-   goto out;
-   }
+   send_sig(SIGKILL, selected, 0);
/*
 * FIXME: lowmemorykiller shouldn't abuse global OOM killer
 * infrastructure. There is no real reason why the selected
 * task should have access to the memory reserves.
 */
-   mark_oom_victim(selected);
+   if (selected->mm)
+   mark_oom_victim(selected);
task_unlock(selected);
lowmem_print(1, "send sigkill to %d (%s), adj %hd, size %d\n",
 selected->pid, selected->comm,
 selected_oom_score_adj, selected_tasksize);
lowmem_deathpending_timeout = jiffies + HZ;
-   send_sig(SIGKILL, selected, 0);
rem += selected_tasksize;
}
-out:
+
lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",
 sc->nr_to_scan, sc->gfp_mask, rem);
rcu_read_unlock();
-- 
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Greg KH]

On Sat, Sep 05, 2015 at 03:06:46AM +0900, Tetsuo Handa wrote:
> Greg KH wrote:
> > On Fri, Sep 04, 2015 at 04:05:59PM +0200, Michal Hocko wrote:
> > > On Wed 02-09-15 18:06:20, Greg KH wrote:
> > > [...]
> > > > And if we aren't taking patch 1/2, I guess this one isn't needed either?
> > > 
> > > Unlike the patch1 which was pretty much cosmetic this fixes a real
> > > issue.
> > 
> > Ok, then it would be great to get this in a format that I can apply it
> > in :)
> 
> I see. Here is a minimal patch.
> (Acked-by: from 
> http://lkml.kernel.org/r/20150827084443.ge14...@dhcp22.suse.cz )
> 
> >From 118609fa25700af11791b1b7e8349f8973a9e7e4 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa 
> Date: Sat, 5 Sep 2015 02:58:12 +0900
> Subject: [PATCH] android, lmk: Send SIGKILL before setting TIF_MEMDIE.
> 
> It was observed that setting TIF_MEMDIE before sending SIGKILL at
> oom_kill_process() allows memory reserves to be depleted by allocations
> which are not needed for terminating the OOM victim.
> 
> This patch reverts commit 6bc2b856bb7c ("staging: android: lowmemorykiller:
> set TIF_MEMDIE before send kill sig"), for oom_kill_process() was updated
> to send SIGKILL before setting TIF_MEMDIE.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> ---
>  drivers/staging/android/lowmemorykiller.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)

Please send this in a format that I can apply it in that doesn't require
me to hand-edit the email :(

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Greg KH]

On Wed, Aug 26, 2015 at 09:19:48PM +0900, Tetsuo Handa wrote:
> Hello.
> 
> Should selected_tasksize be added to rem even when TIF_MEMDIE was not set?
> 
> Please see a thread from http://www.spinics.net/lists/linux-mm/msg93246.html
> if you want to know why to reverse the order.
> 
> >From 2d4cc11d8128e4c1397631b91fea78da3eaefb47 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa 
> Date: Wed, 26 Aug 2015 20:52:39 +0900
> Subject: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE 
> and sending SIGKILL.
> 
> If we set TIF_MEMDIE before sending SIGKILL, memory reserves could be
> spent for allocations which are not needed for terminating the victim.
> Reverse the order as with oom_kill_process() does.


I can't take a patch that I have to hand-edit in order to apply it :(

And if we aren't taking patch 1/2, I guess this one isn't needed either?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Michal Hocko]

On Thu 27-08-15 00:34:47, Tetsuo Handa wrote:
 Reposting updated version as it turned out that we can call do_send_sig_info()
 with task_lock held. ;-) ( http://marc.info/?l=linux-mmm=144059948628905w=2 
 )
 
 Tetsuo Handa wrote:
  Should selected_tasksize be added to rem even when TIF_MEMDIE was not set?
 Commit e1099a69a624 android, lmk: avoid setting TIF_MEMDIE if process
 has already exited changed not to add selected_tasksize to rem. But I
 noticed that rem is initialized to 0 and there is only one addition
 (i.e. rem += selected_tasksize means rem = selected_tasksize) since
 commit 7dc19d5affd7 drivers: convert shrinkers to new count/scan API.
 I don't know what values we should return, but this patch restores
 pre commit e1099a69a624 because omitting a call to mark_oom_victim()
 due to race will not prevent from reclaiming memory because we already
 sent SIGKILL.
 
 From b7075abd3a1e903e88f1755c68adc017d2125b0d Mon Sep 17 00:00:00 2001
 From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
 Date: Thu, 27 Aug 2015 00:13:57 +0900
 Subject: [PATCH 2/2] android, lmk: Send SIGKILL before setting TIF_MEMDIE.
 
 It was observed that setting TIF_MEMDIE before sending SIGKILL at
 oom_kill_process() allows memory reserves to be depleted by allocations
 which are not needed for terminating the OOM victim.
 
 This patch reverts commit 6bc2b856bb7c (staging: android: lowmemorykiller:
 set TIF_MEMDIE before send kill sig), for oom_kill_process() was updated
 to send SIGKILL before setting TIF_MEMDIE.
 
 Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp

Acked-by: Michal Hocko mho...@suse.com

 ---
  drivers/staging/android/lowmemorykiller.c | 17 ++---
  1 file changed, 6 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/staging/android/lowmemorykiller.c 
 b/drivers/staging/android/lowmemorykiller.c
 index d5d25e4..af604cf 100644
 --- a/drivers/staging/android/lowmemorykiller.c
 +++ b/drivers/staging/android/lowmemorykiller.c
 @@ -156,26 +156,21 @@ next:
   }
   if (selected) {
   task_lock(selected);
 - if (!selected-mm) {
 - /* Already exited, cannot do mark_tsk_oom_victim() */
 - task_unlock(selected);
 - goto out;
 - }
 + lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
 +  selected-pid, selected-comm,
 +  selected_oom_score_adj, selected_tasksize);
 + send_sig(SIGKILL, selected, 0);
   /*
* FIXME: lowmemorykiller shouldn't abuse global OOM killer
* infrastructure. There is no real reason why the selected
* task should have access to the memory reserves.
*/
 - mark_oom_victim(selected);
 - lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
 -  selected-pid, selected-comm,
 -  selected_oom_score_adj, selected_tasksize);
 + if (selected-mm)
 + mark_oom_victim(selected);
   task_unlock(selected);
   lowmem_deathpending_timeout = jiffies + HZ;
 - send_sig(SIGKILL, selected, 0);
   rem += selected_tasksize;
   }
 -out:
   lowmem_print(4, lowmem_scan %lu, %x, return %lu\n,
sc-nr_to_scan, sc-gfp_mask, rem);
   rcu_read_unlock();
 -- 
 1.8.3.1

-- 
Michal Hocko
SUSE Labs
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.

[Tetsuo Handa]

Reposting updated version as it turned out that we can call do_send_sig_info()
with task_lock held. ;-) ( http://marc.info/?l=linux-mmm=144059948628905w=2 )

Tetsuo Handa wrote:
 Should selected_tasksize be added to rem even when TIF_MEMDIE was not set?
Commit e1099a69a624 android, lmk: avoid setting TIF_MEMDIE if process
has already exited changed not to add selected_tasksize to rem. But I
noticed that rem is initialized to 0 and there is only one addition
(i.e. rem += selected_tasksize means rem = selected_tasksize) since
commit 7dc19d5affd7 drivers: convert shrinkers to new count/scan API.
I don't know what values we should return, but this patch restores
pre commit e1099a69a624 because omitting a call to mark_oom_victim()
due to race will not prevent from reclaiming memory because we already
sent SIGKILL.

From b7075abd3a1e903e88f1755c68adc017d2125b0d Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Thu, 27 Aug 2015 00:13:57 +0900
Subject: [PATCH 2/2] android, lmk: Send SIGKILL before setting TIF_MEMDIE.

It was observed that setting TIF_MEMDIE before sending SIGKILL at
oom_kill_process() allows memory reserves to be depleted by allocations
which are not needed for terminating the OOM victim.

This patch reverts commit 6bc2b856bb7c (staging: android: lowmemorykiller:
set TIF_MEMDIE before send kill sig), for oom_kill_process() was updated
to send SIGKILL before setting TIF_MEMDIE.

Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
 drivers/staging/android/lowmemorykiller.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/lowmemorykiller.c 
b/drivers/staging/android/lowmemorykiller.c
index d5d25e4..af604cf 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -156,26 +156,21 @@ next:
}
if (selected) {
task_lock(selected);
-   if (!selected-mm) {
-   /* Already exited, cannot do mark_tsk_oom_victim() */
-   task_unlock(selected);
-   goto out;
-   }
+   lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
+selected-pid, selected-comm,
+selected_oom_score_adj, selected_tasksize);
+   send_sig(SIGKILL, selected, 0);
/*
 * FIXME: lowmemorykiller shouldn't abuse global OOM killer
 * infrastructure. There is no real reason why the selected
 * task should have access to the memory reserves.
 */
-   mark_oom_victim(selected);
-   lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
-selected-pid, selected-comm,
-selected_oom_score_adj, selected_tasksize);
+   if (selected-mm)
+   mark_oom_victim(selected);
task_unlock(selected);
lowmem_deathpending_timeout = jiffies + HZ;
-   send_sig(SIGKILL, selected, 0);
rem += selected_tasksize;
}
-out:
lowmem_print(4, lowmem_scan %lu, %x, return %lu\n,
 sc-nr_to_scan, sc-gfp_mask, rem);
rcu_read_unlock();
-- 
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel