* [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move.
@ 2010-06-17 8:20 KAMEZAWA Hiroyuki
2010-06-17 9:24 ` Balbir Singh
0 siblings, 1 reply; 5+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-06-17 8:20 UTC (permalink / raw)
To: linux-mm; +Cc: akpm, nishimura, balbir, linux-kernel
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
When cgroup_cancel_attach() is called via cgroup_attach_task(),
mem_cgroup_clear_mc() can be called even when any migration
was done. In such case, mc.to and mc.from is NULL.
But, memcg-clean-up-waiting-move-acct-v2.patch
doesn't handle this correctly and pass NULL to memcg_oom_recover.
fix it.
BUG: unable to handle kernel paging request at 000000000000114c
IP: [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
PGD 61ce4b067 PUD 613ea0067 PMD 0
Oops: 0000 [#1] SMP
<snip>
Call Trace:
[<ffffffff81155359>] mem_cgroup_clear_mc+0x119/0x1c0
[<ffffffff811554de>] mem_cgroup_cancel_attach+0xe/0x10
[<ffffffff810b619c>] cgroup_attach_task+0x26c/0x2c0
[<ffffffff810b6257>] cgroup_tasks_write+0x67/0x1c0
[<ffffffff81121555>] ? might_fault+0xa5/0xb0
[<ffffffff8112150c>] ? might_fault+0x5c/0xb0
[<ffffffff810b40a2>] cgroup_file_write+0x2d2/0x330
[<ffffffff81093aa2>] ? print_lock_contention_bug+0x22/0xf0
[<ffffffff81259fef>] ? security_file_permission+0x1f/0x80
[<ffffffff8115d998>] vfs_write+0xc8/0x190
[<ffffffff8115e3a1>] sys_write+0x51/0x90
[<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
Code: 20 48 39 43 20 41 bc f0 ff ff ff 75 c7 45 88 ae 48 11 00 00 45 31 e4 eb bb 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 0f 1f 44 00 00 <8b> 87 4c 11 00 00 85 c0 75 05 c9 c3 0f 1f 00 48 89 f9 31 d2 be
RIP [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
mm/memcontrol.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: mmotm-2.6.35-0611/mm/memcontrol.c
===================================================================
--- mmotm-2.6.35-0611.orig/mm/memcontrol.c
+++ mmotm-2.6.35-0611/mm/memcontrol.c
@@ -4485,8 +4485,10 @@ static void mem_cgroup_clear_mc(void)
mc.to = NULL;
mc.moving_task = NULL;
spin_unlock(&mc.lock);
- memcg_oom_recover(from);
- memcg_oom_recover(to);
+ if (from)
+ memcg_oom_recover(from);
+ if (to)
+ memcg_oom_recover(to);
wake_up_all(&mc.waitq);
}
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move.
2010-06-17 8:20 [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move KAMEZAWA Hiroyuki
@ 2010-06-17 9:24 ` Balbir Singh
2010-06-18 1:57 ` Daisuke Nishimura
0 siblings, 1 reply; 5+ messages in thread
From: Balbir Singh @ 2010-06-17 9:24 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: linux-mm, akpm, nishimura, linux-kernel
* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-06-17 17:20:34]:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> When cgroup_cancel_attach() is called via cgroup_attach_task(),
> mem_cgroup_clear_mc() can be called even when any migration
> was done. In such case, mc.to and mc.from is NULL.
>
> But, memcg-clean-up-waiting-move-acct-v2.patch
> doesn't handle this correctly and pass NULL to memcg_oom_recover.
> fix it.
>
> BUG: unable to handle kernel paging request at 000000000000114c
> IP: [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
> PGD 61ce4b067 PUD 613ea0067 PMD 0
> Oops: 0000 [#1] SMP
> <snip>
> Call Trace:
> [<ffffffff81155359>] mem_cgroup_clear_mc+0x119/0x1c0
> [<ffffffff811554de>] mem_cgroup_cancel_attach+0xe/0x10
> [<ffffffff810b619c>] cgroup_attach_task+0x26c/0x2c0
> [<ffffffff810b6257>] cgroup_tasks_write+0x67/0x1c0
> [<ffffffff81121555>] ? might_fault+0xa5/0xb0
> [<ffffffff8112150c>] ? might_fault+0x5c/0xb0
> [<ffffffff810b40a2>] cgroup_file_write+0x2d2/0x330
> [<ffffffff81093aa2>] ? print_lock_contention_bug+0x22/0xf0
> [<ffffffff81259fef>] ? security_file_permission+0x1f/0x80
> [<ffffffff8115d998>] vfs_write+0xc8/0x190
> [<ffffffff8115e3a1>] sys_write+0x51/0x90
> [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
> Code: 20 48 39 43 20 41 bc f0 ff ff ff 75 c7 45 88 ae 48 11 00 00 45 31 e4 eb bb 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 0f 1f 44 00 00 <8b> 87 4c 11 00 00 85 c0 75 05 c9 c3 0f 1f 00 48 89 f9 31 d2 be
> RIP [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
> mm/memcontrol.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: mmotm-2.6.35-0611/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.35-0611.orig/mm/memcontrol.c
> +++ mmotm-2.6.35-0611/mm/memcontrol.c
> @@ -4485,8 +4485,10 @@ static void mem_cgroup_clear_mc(void)
> mc.to = NULL;
> mc.moving_task = NULL;
> spin_unlock(&mc.lock);
> - memcg_oom_recover(from);
> - memcg_oom_recover(to);
> + if (from)
> + memcg_oom_recover(from);
> + if (to)
> + memcg_oom_recover(to);
> wake_up_all(&mc.waitq);
May I recommend the following change instead
Don't crash on a null memcg being passed, check if memcg
is NULL and handle the condition gracefully
Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---
mm/memcontrol.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c6ece0a..d71c488 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1370,7 +1370,7 @@ static void memcg_wakeup_oom(struct mem_cgroup *mem)
static void memcg_oom_recover(struct mem_cgroup *mem)
{
- if (mem->oom_kill_disable && atomic_read(&mem->oom_lock))
+ if (mem && mem->oom_kill_disable && atomic_read(&mem->oom_lock))
memcg_wakeup_oom(mem);
}
--
1.7.0.1
--
Three Cheers,
Balbir
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move.
2010-06-17 9:24 ` Balbir Singh
@ 2010-06-18 1:57 ` Daisuke Nishimura
2010-06-18 2:17 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 5+ messages in thread
From: Daisuke Nishimura @ 2010-06-18 1:57 UTC (permalink / raw)
To: balbir; +Cc: nishimura, KAMEZAWA Hiroyuki, linux-mm, akpm, linux-kernel
On Thu, 17 Jun 2010 14:54:42 +0530, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-06-17 17:20:34]:
>
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> >
> > When cgroup_cancel_attach() is called via cgroup_attach_task(),
> > mem_cgroup_clear_mc() can be called even when any migration
> > was done. In such case, mc.to and mc.from is NULL.
> >
> > But, memcg-clean-up-waiting-move-acct-v2.patch
> > doesn't handle this correctly and pass NULL to memcg_oom_recover.
> > fix it.
> >
> > BUG: unable to handle kernel paging request at 000000000000114c
> > IP: [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
> > PGD 61ce4b067 PUD 613ea0067 PMD 0
> > Oops: 0000 [#1] SMP
> > <snip>
> > Call Trace:
> > [<ffffffff81155359>] mem_cgroup_clear_mc+0x119/0x1c0
> > [<ffffffff811554de>] mem_cgroup_cancel_attach+0xe/0x10
> > [<ffffffff810b619c>] cgroup_attach_task+0x26c/0x2c0
> > [<ffffffff810b6257>] cgroup_tasks_write+0x67/0x1c0
> > [<ffffffff81121555>] ? might_fault+0xa5/0xb0
> > [<ffffffff8112150c>] ? might_fault+0x5c/0xb0
> > [<ffffffff810b40a2>] cgroup_file_write+0x2d2/0x330
> > [<ffffffff81093aa2>] ? print_lock_contention_bug+0x22/0xf0
> > [<ffffffff81259fef>] ? security_file_permission+0x1f/0x80
> > [<ffffffff8115d998>] vfs_write+0xc8/0x190
> > [<ffffffff8115e3a1>] sys_write+0x51/0x90
> > [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
> > Code: 20 48 39 43 20 41 bc f0 ff ff ff 75 c7 45 88 ae 48 11 00 00 45 31 e4 eb bb 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 0f 1f 44 00 00 <8b> 87 4c 11 00 00 85 c0 75 05 c9 c3 0f 1f 00 48 89 f9 31 d2 be
> > RIP [<ffffffff81153bb9>] memcg_oom_recover+0x9/0x30
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> > mm/memcontrol.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Index: mmotm-2.6.35-0611/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-2.6.35-0611.orig/mm/memcontrol.c
> > +++ mmotm-2.6.35-0611/mm/memcontrol.c
> > @@ -4485,8 +4485,10 @@ static void mem_cgroup_clear_mc(void)
> > mc.to = NULL;
> > mc.moving_task = NULL;
> > spin_unlock(&mc.lock);
> > - memcg_oom_recover(from);
> > - memcg_oom_recover(to);
> > + if (from)
> > + memcg_oom_recover(from);
> > + if (to)
> > + memcg_oom_recover(to);
> > wake_up_all(&mc.waitq);
>
> May I recommend the following change instead
>
>
> Don't crash on a null memcg being passed, check if memcg
> is NULL and handle the condition gracefully
>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> mm/memcontrol.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c6ece0a..d71c488 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1370,7 +1370,7 @@ static void memcg_wakeup_oom(struct mem_cgroup *mem)
>
> static void memcg_oom_recover(struct mem_cgroup *mem)
> {
> - if (mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> + if (mem && mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> memcg_wakeup_oom(mem);
> }
>
I agree to this fix.
Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Thanks,
Daisuke Nishimura.
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move.
2010-06-18 1:57 ` Daisuke Nishimura
@ 2010-06-18 2:17 ` KAMEZAWA Hiroyuki
2010-06-18 3:15 ` Balbir Singh
0 siblings, 1 reply; 5+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-06-18 2:17 UTC (permalink / raw)
To: Daisuke Nishimura; +Cc: balbir, linux-mm, akpm, linux-kernel
On Fri, 18 Jun 2010 10:57:41 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> > May I recommend the following change instead
> >
> >
> > Don't crash on a null memcg being passed, check if memcg
> > is NULL and handle the condition gracefully
> >
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> > ---
> > mm/memcontrol.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index c6ece0a..d71c488 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1370,7 +1370,7 @@ static void memcg_wakeup_oom(struct mem_cgroup *mem)
> >
> > static void memcg_oom_recover(struct mem_cgroup *mem)
> > {
> > - if (mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> > + if (mem && mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> > memcg_wakeup_oom(mem);
> > }
> >
> I agree to this fix.
>
> Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>
I tend to dislike band-aid in callee. but it's not important here.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move.
2010-06-18 2:17 ` KAMEZAWA Hiroyuki
@ 2010-06-18 3:15 ` Balbir Singh
0 siblings, 0 replies; 5+ messages in thread
From: Balbir Singh @ 2010-06-18 3:15 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: Daisuke Nishimura, linux-mm, akpm, linux-kernel
* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-06-18 11:17:35]:
> On Fri, 18 Jun 2010 10:57:41 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
>
> > > May I recommend the following change instead
> > >
> > >
> > > Don't crash on a null memcg being passed, check if memcg
> > > is NULL and handle the condition gracefully
> > >
> > > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> > > ---
> > > mm/memcontrol.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index c6ece0a..d71c488 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -1370,7 +1370,7 @@ static void memcg_wakeup_oom(struct mem_cgroup *mem)
> > >
> > > static void memcg_oom_recover(struct mem_cgroup *mem)
> > > {
> > > - if (mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> > > + if (mem && mem->oom_kill_disable && atomic_read(&mem->oom_lock))
> > > memcg_wakeup_oom(mem);
> > > }
> > >
> > I agree to this fix.
> >
> > Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> >
>
> I tend to dislike band-aid in callee. but it's not important here.
>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
The reason is just to make the reading easier
if (cond)
func(cond)
if (cond2)
func(cond2)
It is easier to read
func(cond)
...
func(cond2)
Provided it is valid for us to test the condition inside func()
This way new callers don't have to worry about using func(). This is
very much like how the free calls work today, they can tolerate a NULL
argument and return gracefully.
--
Three Cheers,
Balbir
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-06-18 3:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-17 8:20 [BUGFIX][PATCH -mm] fix bad call of memcg_oom_recover at cancel move KAMEZAWA Hiroyuki
2010-06-17 9:24 ` Balbir Singh
2010-06-18 1:57 ` Daisuke Nishimura
2010-06-18 2:17 ` KAMEZAWA Hiroyuki
2010-06-18 3:15 ` Balbir Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox