linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC] Low overhead patches for the memory cgroup controller (v2)
@ 2009-05-15 17:45 KAMEZAWA Hiroyuki
  2009-05-15 18:16 ` Balbir Singh
  2009-05-17  4:15 ` Balbir Singh
  0 siblings, 2 replies; 11+ messages in thread
From: KAMEZAWA Hiroyuki @ 2009-05-15 17:45 UTC (permalink / raw)
  To: balbir
  Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton,
	KAMEZAWA Hiroyuki, nishimura@mxp.nes.nec.co.jp,
	lizf@cn.fujitsu.com, menage@google.com, KOSAKI Motohiro

Balbir Singh wrote:
> Feature: Remove the overhead associated with the root cgroup
>
> From: Balbir Singh <balbir@linux.vnet.ibm.com>
>
> This patch changes the memory cgroup and removes the overhead associated
> with LRU maintenance of all pages in the root cgroup. As a side-effect, we
> can
> no longer set a memory hard limit in the root cgroup.
>
> A new flag is used to track page_cgroup associated with the root cgroup
> pages. A new flag to track whether the page has been accounted or not
> has been added as well.
>
> Review comments higly appreciated
>
> Tests
>
> 1. Tested with allocate, touch and limit test case for a non-root cgroup
> 2. For the root cgroup tested performance impact with reaim
>
>
> 		+patch		mmtom-08-may-2009
> AIM9		1362.93		1338.17
> Dbase		17457.75	16021.58
> New Dbase	18070.18	16518.54
> Shared		9681.85		8882.11
> Compute		16197.79	15226.13
>
Hmm, at first impression, I can't convice the numbers...
Just avoiding list_add/del makes programs _10%_ faster ?
Could you show changes in cpu cache-miss late if you can ?
(And why Aim9 goes bad ?)
Hmm, page_cgroup_zoneinfo() is accessed anyway, then...per zone counter
is not a problem here..

Could you show your .config and environment ?
When I trunst above numbers, it seems there is more optimization/
prefetch point in usual path

BTW, how the perfomance changes in children(not default) groups ?

> 3. Tested accounting in root cgroup to make sure it looks sane and
> correct.
>
Not sure but swap and shmem case should be checked carefully..


> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
>
>  include/linux/page_cgroup.h |   10 ++++++++++
>  mm/memcontrol.c             |   29 ++++++++++++++++++++++++++---
>  mm/page_cgroup.c            |    1 -
>  3 files changed, 36 insertions(+), 4 deletions(-)
>
>
> diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> index 7339c7b..8b85752 100644
> --- a/include/linux/page_cgroup.h
> +++ b/include/linux/page_cgroup.h
> @@ -26,6 +26,8 @@ enum {
>  	PCG_LOCK,  /* page cgroup is locked */
>  	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
> +	PCG_ROOT, /* page belongs to root cgroup */
> +	PCG_ACCT, /* page has been accounted for */
Reading codes, this PCG_ACCT should be PCG_AcctLRU.

>  };
>
>  #define TESTPCGFLAG(uname, lname)			\
> @@ -46,6 +48,14 @@ TESTPCGFLAG(Cache, CACHE)
>  TESTPCGFLAG(Used, USED)
>  CLEARPCGFLAG(Used, USED)
>
> +SETPCGFLAG(Root, ROOT)
> +CLEARPCGFLAG(Root, ROOT)
> +TESTPCGFLAG(Root, ROOT)
> +
> +SETPCGFLAG(Acct, ACCT)
> +CLEARPCGFLAG(Acct, ACCT)
> +TESTPCGFLAG(Acct, ACCT)
> +
>  static inline int page_cgroup_nid(struct page_cgroup *pc)
>  {
>  	return page_to_nid(pc->page);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9712ef7..18d2819 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -43,6 +43,7 @@
>
>  struct cgroup_subsys mem_cgroup_subsys __read_mostly;
>  #define MEM_CGROUP_RECLAIM_RETRIES	5
> +struct mem_cgroup *root_mem_cgroup __read_mostly;
>
>  #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
>  /* Turned on only when memory cgroup is enabled && really_do_swap_account
> = 0 */
> @@ -196,6 +197,10 @@ enum charge_type {
>  #define PCGF_CACHE	(1UL << PCG_CACHE)
>  #define PCGF_USED	(1UL << PCG_USED)
>  #define PCGF_LOCK	(1UL << PCG_LOCK)
> +/* Not used, but added here for completeness */
> +#define PCGF_ROOT	(1UL << PCG_ROOT)
> +#define PCGF_ACCT	(1UL << PCG_ACCT)
> +
>  static const unsigned long
>  pcg_default_flags[NR_CHARGE_TYPE] = {
>  	PCGF_CACHE | PCGF_USED | PCGF_LOCK, /* File Cache */
> @@ -420,7 +425,7 @@ void mem_cgroup_del_lru_list(struct page *page, enum
> lru_list lru)
>  		return;
>  	pc = lookup_page_cgroup(page);
>  	/* can happen while we handle swapcache. */
> -	if (list_empty(&pc->lru) || !pc->mem_cgroup)
> +	if ((!PageCgroupAcct(pc) && list_empty(&pc->lru)) || !pc->mem_cgroup)
>  		return;
>  	/*
>  	 * We don't check PCG_USED bit. It's cleared when the "page" is finally
> @@ -429,6 +434,9 @@ void mem_cgroup_del_lru_list(struct page *page, enum
> lru_list lru)
>  	mz = page_cgroup_zoneinfo(pc);
>  	mem = pc->mem_cgroup;
>  	MEM_CGROUP_ZSTAT(mz, lru) -= 1;
> +	ClearPageCgroupAcct(pc);
> +	if (PageCgroupRoot(pc))
> +		return;
>  	list_del_init(&pc->lru);
>  	return;
>  }


> @@ -452,8 +460,8 @@ void mem_cgroup_rotate_lru_list(struct page *page,
> enum lru_list lru)
>  	 * For making pc->mem_cgroup visible, insert smp_rmb() here.
>  	 */
>  	smp_rmb();
> -	/* unused page is not rotated. */
> -	if (!PageCgroupUsed(pc))
> +	/* unused or root page is not rotated. */
> +	if (!PageCgroupUsed(pc) || PageCgroupRoot(pc))
>  		return;
>  	mz = page_cgroup_zoneinfo(pc);
>  	list_move(&pc->lru, &mz->lists[lru]);
> @@ -477,6 +485,9 @@ void mem_cgroup_add_lru_list(struct page *page, enum
> lru_list lru)
>
>  	mz = page_cgroup_zoneinfo(pc);
>  	MEM_CGROUP_ZSTAT(mz, lru) += 1;
> +	SetPageCgroupAcct(pc);
> +	if (PageCgroupRoot(pc))
> +		return;
>  	list_add(&pc->lru, &mz->lists[lru]);
>  }
I think set/clear flag here adds race condtion....because pc->flags is
modfied by
  pc->flags = pcg_dafault_flags[ctype] in commit_charge()
you have to modify above lines to be

  SetPageCgroupCache(pc) or some..
  ...
  SetPageCgroupUsed(pc)

Then, you can use set_bit() without lock_page_cgroup().
(Currently, pc->flags is modified only under lock_page_cgroup(), so,
 non atomic code is used.)

Regards,
-Kame



^ permalink raw reply	[flat|nested] 11+ messages in thread
* [RFC] Low overhead patches for the memory cgroup controller (v2)
@ 2009-05-15 15:18 Balbir Singh
  0 siblings, 0 replies; 11+ messages in thread
From: Balbir Singh @ 2009-05-15 15:18 UTC (permalink / raw)
  To: linux-mm@kvack.org
  Cc: linux-kernel@vger.kernel.org, Andrew Morton, KAMEZAWA Hiroyuki,
	nishimura@mxp.nes.nec.co.jp, lizf@cn.fujitsu.com,
	menage@google.com, KOSAKI Motohiro

Feature: Remove the overhead associated with the root cgroup

From: Balbir Singh <balbir@linux.vnet.ibm.com>

This patch changes the memory cgroup and removes the overhead associated
with LRU maintenance of all pages in the root cgroup. As a side-effect, we can
no longer set a memory hard limit in the root cgroup.

A new flag is used to track page_cgroup associated with the root cgroup
pages. A new flag to track whether the page has been accounted or not
has been added as well.

Review comments higly appreciated

Tests

1. Tested with allocate, touch and limit test case for a non-root cgroup
2. For the root cgroup tested performance impact with reaim


		+patch		mmtom-08-may-2009
AIM9		1362.93		1338.17
Dbase		17457.75	16021.58
New Dbase	18070.18	16518.54
Shared		9681.85		8882.11
Compute		16197.79	15226.13

3. Tested accounting in root cgroup to make sure it looks sane and
correct.

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

 include/linux/page_cgroup.h |   10 ++++++++++
 mm/memcontrol.c             |   29 ++++++++++++++++++++++++++---
 mm/page_cgroup.c            |    1 -
 3 files changed, 36 insertions(+), 4 deletions(-)


diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index 7339c7b..8b85752 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -26,6 +26,8 @@ enum {
 	PCG_LOCK,  /* page cgroup is locked */
 	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
+	PCG_ROOT, /* page belongs to root cgroup */
+	PCG_ACCT, /* page has been accounted for */
 };
 
 #define TESTPCGFLAG(uname, lname)			\
@@ -46,6 +48,14 @@ TESTPCGFLAG(Cache, CACHE)
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 
+SETPCGFLAG(Root, ROOT)
+CLEARPCGFLAG(Root, ROOT)
+TESTPCGFLAG(Root, ROOT)
+
+SETPCGFLAG(Acct, ACCT)
+CLEARPCGFLAG(Acct, ACCT)
+TESTPCGFLAG(Acct, ACCT)
+
 static inline int page_cgroup_nid(struct page_cgroup *pc)
 {
 	return page_to_nid(pc->page);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9712ef7..18d2819 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -43,6 +43,7 @@
 
 struct cgroup_subsys mem_cgroup_subsys __read_mostly;
 #define MEM_CGROUP_RECLAIM_RETRIES	5
+struct mem_cgroup *root_mem_cgroup __read_mostly;
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 /* Turned on only when memory cgroup is enabled && really_do_swap_account = 0 */
@@ -196,6 +197,10 @@ enum charge_type {
 #define PCGF_CACHE	(1UL << PCG_CACHE)
 #define PCGF_USED	(1UL << PCG_USED)
 #define PCGF_LOCK	(1UL << PCG_LOCK)
+/* Not used, but added here for completeness */
+#define PCGF_ROOT	(1UL << PCG_ROOT)
+#define PCGF_ACCT	(1UL << PCG_ACCT)
+
 static const unsigned long
 pcg_default_flags[NR_CHARGE_TYPE] = {
 	PCGF_CACHE | PCGF_USED | PCGF_LOCK, /* File Cache */
@@ -420,7 +425,7 @@ void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru)
 		return;
 	pc = lookup_page_cgroup(page);
 	/* can happen while we handle swapcache. */
-	if (list_empty(&pc->lru) || !pc->mem_cgroup)
+	if ((!PageCgroupAcct(pc) && list_empty(&pc->lru)) || !pc->mem_cgroup)
 		return;
 	/*
 	 * We don't check PCG_USED bit. It's cleared when the "page" is finally
@@ -429,6 +434,9 @@ void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru)
 	mz = page_cgroup_zoneinfo(pc);
 	mem = pc->mem_cgroup;
 	MEM_CGROUP_ZSTAT(mz, lru) -= 1;
+	ClearPageCgroupAcct(pc);
+	if (PageCgroupRoot(pc))
+		return;
 	list_del_init(&pc->lru);
 	return;
 }
@@ -452,8 +460,8 @@ void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru)
 	 * For making pc->mem_cgroup visible, insert smp_rmb() here.
 	 */
 	smp_rmb();
-	/* unused page is not rotated. */
-	if (!PageCgroupUsed(pc))
+	/* unused or root page is not rotated. */
+	if (!PageCgroupUsed(pc) || PageCgroupRoot(pc))
 		return;
 	mz = page_cgroup_zoneinfo(pc);
 	list_move(&pc->lru, &mz->lists[lru]);
@@ -477,6 +485,9 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
 
 	mz = page_cgroup_zoneinfo(pc);
 	MEM_CGROUP_ZSTAT(mz, lru) += 1;
+	SetPageCgroupAcct(pc);
+	if (PageCgroupRoot(pc))
+		return;
 	list_add(&pc->lru, &mz->lists[lru]);
 }
 
@@ -1114,10 +1125,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *mem,
 		css_put(&mem->css);
 		return;
 	}
+
 	pc->mem_cgroup = mem;
 	smp_wmb();
 	pc->flags = pcg_default_flags[ctype];
 
+	if (mem == root_mem_cgroup)
+		SetPageCgroupRoot(pc);
+
 	mem_cgroup_charge_statistics(mem, pc, true);
 
 	unlock_page_cgroup(pc);
@@ -1521,6 +1536,8 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	mem_cgroup_charge_statistics(mem, pc, false);
 
 	ClearPageCgroupUsed(pc);
+	if (mem == root_mem_cgroup)
+		ClearPageCgroupRoot(pc);
 	/*
 	 * pc->mem_cgroup is not cleared here. It will be accessed when it's
 	 * freed from LRU. This is safe because uncharged page is expected not
@@ -2038,6 +2055,10 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
 	name = MEMFILE_ATTR(cft->private);
 	switch (name) {
 	case RES_LIMIT:
+		if (memcg == root_mem_cgroup) { /* Can't set limit on root */
+			ret = -EINVAL;
+			break;
+		}
 		/* This function does all necessary parse...reuse it */
 		ret = res_counter_memparse_write_strategy(buffer, &val);
 		if (ret)
@@ -2504,6 +2525,7 @@ mem_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cont)
 	if (cont->parent == NULL) {
 		enable_swap_cgroup();
 		parent = NULL;
+		root_mem_cgroup = mem;
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);
 		mem->use_hierarchy = parent->use_hierarchy;
@@ -2532,6 +2554,7 @@ mem_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cont)
 	return &mem->css;
 free_out:
 	__mem_cgroup_free(mem);
+	root_mem_cgroup = NULL;
 	return ERR_PTR(error);
 }
 
diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
index 09b73c5..6145ff6 100644
--- a/mm/page_cgroup.c
+++ b/mm/page_cgroup.c
@@ -276,7 +276,6 @@ void __meminit pgdat_page_cgroup_init(struct pglist_data *pgdat)
 
 #endif
 
-
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 
 static DEFINE_MUTEX(swap_cgroup_mutex);

-- 
	Balbir

^ permalink raw reply related	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-06-01  5:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-15 17:45 [RFC] Low overhead patches for the memory cgroup controller (v2) KAMEZAWA Hiroyuki
2009-05-15 18:16 ` Balbir Singh
2009-05-18 10:11   ` KAMEZAWA Hiroyuki
2009-05-18 10:45     ` Balbir Singh
2009-05-18 16:01       ` KAMEZAWA Hiroyuki
2009-05-19 13:18         ` Balbir Singh
2009-05-17  4:15 ` Balbir Singh
2009-06-01  4:25   ` Daisuke Nishimura
2009-06-01  5:01     ` Daisuke Nishimura
2009-06-01  5:49     ` Balbir Singh
  -- strict thread matches above, loose matches on Subject: below --
2009-05-15 15:18 Balbir Singh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).