[BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[Daisuke Nishimura]

From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

memcg: fix try_get_mem_cgroup_from_swapcache()

This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
Used bit of swapcache is solid under page lock, but considering move_account,
pc->mem_cgroup is not.

We need lock_page_cgroup() anyway.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
---
 mm/memcontrol.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ccc69b4..84f856c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1024,9 +1024,7 @@ static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
 		return NULL;
 
 	pc = lookup_page_cgroup(page);
-	/*
-	 * Used bit of swapcache is solid under page lock.
-	 */
+	lock_page_cgroup(pc);
 	if (PageCgroupUsed(pc)) {
 		mem = pc->mem_cgroup;
 		if (mem && !css_tryget(&mem->css))
@@ -1040,6 +1038,7 @@ static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
 			mem = NULL;
 		rcu_read_unlock();
 	}
+	unlock_page_cgroup(pc);
 	return mem;
 }
 

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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[KAMEZAWA Hiroyuki]

On Mon, 27 Apr 2009 09:51:00 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> memcg: fix try_get_mem_cgroup_from_swapcache()
> 
> This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> Used bit of swapcache is solid under page lock, but considering move_account,
> pc->mem_cgroup is not.
> 
> We need lock_page_cgroup() anyway.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

you are right.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

> ---
>  mm/memcontrol.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ccc69b4..84f856c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1024,9 +1024,7 @@ static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
>  		return NULL;
>  
>  	pc = lookup_page_cgroup(page);
> -	/*
> -	 * Used bit of swapcache is solid under page lock.
> -	 */
> +	lock_page_cgroup(pc);
>  	if (PageCgroupUsed(pc)) {
>  		mem = pc->mem_cgroup;
>  		if (mem && !css_tryget(&mem->css))
> @@ -1040,6 +1038,7 @@ static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
>  			mem = NULL;
>  		rcu_read_unlock();
>  	}
> +	unlock_page_cgroup(pc);
>  	return mem;
>  }
>  
> 

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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[Balbir Singh]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-04-27 09:51:00]:

> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> memcg: fix try_get_mem_cgroup_from_swapcache()
> 
> This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> Used bit of swapcache is solid under page lock, but considering move_account,
> pc->mem_cgroup is not.
> 
> We need lock_page_cgroup() anyway.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

I think we need to start documenting the locks the
page_cgroup lock nests under.

If memcg_tasklist were a spinlock instead of mutex, could we use that
instead of page_cgroup lock, since we care only about task migration?

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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[KAMEZAWA Hiroyuki]

On Mon, 27 Apr 2009 12:23:58 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-04-27 09:51:00]:
> 
> > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > 
> > memcg: fix try_get_mem_cgroup_from_swapcache()
> > 
> > This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> > Used bit of swapcache is solid under page lock, but considering move_account,
> > pc->mem_cgroup is not.
> > 
> > We need lock_page_cgroup() anyway.
> > 
> > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> I think we need to start documenting the locks the
> page_cgroup lock nests under.
> 
Addin some comments on source code may be necessary.

> If memcg_tasklist were a spinlock instead of mutex, could we use that
> instead of page_cgroup lock, since we care only about task migration?
> 

Hmm ? Another problem ? I can't catch what you ask.
move_account() is a function called by force_empty()->"move account to parent"

Thanks,
-Kame

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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-04-27 15:59:53]:

> On Mon, 27 Apr 2009 12:23:58 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-04-27 09:51:00]:
> > 
> > > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > 
> > > memcg: fix try_get_mem_cgroup_from_swapcache()
> > > 
> > > This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> > > Used bit of swapcache is solid under page lock, but considering move_account,
> > > pc->mem_cgroup is not.
> > > 
> > > We need lock_page_cgroup() anyway.
> > > 
> > > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > 
> > I think we need to start documenting the locks the
> > page_cgroup lock nests under.
> > 
> Addin some comments on source code may be necessary.
> 
> > If memcg_tasklist were a spinlock instead of mutex, could we use that
> > instead of page_cgroup lock, since we care only about task migration?
> > 
> 
> Hmm ? Another problem ? I can't catch what you ask.
> move_account() is a function called by force_empty()->"move account to parent"

IIUC, pc->mem_cgroup might change due to task migration and that is
why we've added the page cgroup lock. If the race is between task
migration and force_empty(), we can use the memcg_tasklist by making
it a spinlock.

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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[KAMEZAWA Hiroyuki]

On Mon, 27 Apr 2009 12:33:42 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-04-27 15:59:53]:
> 
> > On Mon, 27 Apr 2009 12:23:58 +0530
> > Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> > 
> > > * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-04-27 09:51:00]:
> > > 
> > > > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > > 
> > > > memcg: fix try_get_mem_cgroup_from_swapcache()
> > > > 
> > > > This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> > > > Used bit of swapcache is solid under page lock, but considering move_account,
> > > > pc->mem_cgroup is not.
> > > > 
> > > > We need lock_page_cgroup() anyway.
> > > > 
> > > > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > 
> > > I think we need to start documenting the locks the
> > > page_cgroup lock nests under.
> > > 
> > Addin some comments on source code may be necessary.
> > 
> > > If memcg_tasklist were a spinlock instead of mutex, could we use that
> > > instead of page_cgroup lock, since we care only about task migration?
> > > 
> > 
> > Hmm ? Another problem ? I can't catch what you ask.
> > move_account() is a function called by force_empty()->"move account to parent"
> 
> IIUC, pc->mem_cgroup might change due to task migration and that is
> why we've added the page cgroup lock.

"task" migration ? I think it's "page" migration.

"page cgroup lock" is necessary because we have to modify 2 params
(pc->flags and pc->mem_cgroup) at once.

>  If the race is between task
> migration and force_empty(), we can use the memcg_tasklist by making
> it a spinlock.
> 
The race is between force_empty() and any other swap accounitng ops.
This patch is good enough from my point of view.

Thanks,
-kame


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

Re: [BUGFIX][PATCH] memcg: fix try_get_mem_cgroup_from_swapcache()

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-04-27 16:08:46]:

> On Mon, 27 Apr 2009 12:33:42 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-04-27 15:59:53]:
> > 
> > > On Mon, 27 Apr 2009 12:23:58 +0530
> > > Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> > > 
> > > > * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-04-27 09:51:00]:
> > > > 
> > > > > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > > > 
> > > > > memcg: fix try_get_mem_cgroup_from_swapcache()
> > > > > 
> > > > > This is a bugfix for commit 3c776e64660028236313f0e54f3a9945764422df(included 2.6.30-rc1).
> > > > > Used bit of swapcache is solid under page lock, but considering move_account,
> > > > > pc->mem_cgroup is not.
> > > > > 
> > > > > We need lock_page_cgroup() anyway.
> > > > > 
> > > > > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > > 
> > > > I think we need to start documenting the locks the
> > > > page_cgroup lock nests under.
> > > > 
> > > Addin some comments on source code may be necessary.
> > > 
> > > > If memcg_tasklist were a spinlock instead of mutex, could we use that
> > > > instead of page_cgroup lock, since we care only about task migration?
> > > > 
> > > 
> > > Hmm ? Another problem ? I can't catch what you ask.
> > > move_account() is a function called by force_empty()->"move account to parent"
> > 
> > IIUC, pc->mem_cgroup might change due to task migration and that is
> > why we've added the page cgroup lock.
> 
> "task" migration ? I think it's "page" migration.
>

OK
 
> "page cgroup lock" is necessary because we have to modify 2 params
> (pc->flags and pc->mem_cgroup) at once.
>

Yes, definitely!
 
> >  If the race is between task
> > migration and force_empty(), we can use the memcg_tasklist by making
> > it a spinlock.
> > 
> The race is between force_empty() and any other swap accounitng ops.
> This patch is good enough from my point of view.

Fair enough


Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
 

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