On Wed, 10 Mar 2010 09:26:24 +0530, Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:
> * nishimura@xxxxxxxxxxxxxxxxx <nishimura@xxxxxxxxxxxxxxxxx> [2010-03-10 10:43:09]:
>
> > > Please please measure the performance overhead of this change.
> > >
> >
> > here.
> >
> > > > > > > > I made a patch below and measured the time(average of 10 times) of kernel build
> > > > > > > > on tmpfs(make -j8 on 8 CPU machine with 2.6.33 defconfig).
> > > > > > > >
> > > > > > > > <before>
> > > > > > > > - root cgroup: 190.47 sec
> > > > > > > > - child cgroup: 192.81 sec
> > > > > > > >
> > > > > > > > <after>
> > > > > > > > - root cgroup: 191.06 sec
> > > > > > > > - child cgroup: 193.06 sec
> > > > > > > >
> >
> > <after2(local_irq_save/restore)>
> > - root cgroup: 191.42 sec
> > - child cgroup: 193.55 sec
> >
> > hmm, I think it's in error range, but I can see a tendency by testing several times
> > that it's getting slower as I add additional codes. Using local_irq_disable()/enable()
> > except in mem_cgroup_update_file_mapped(it can be the only candidate to be called
> > with irq disabled in future) might be the choice.
> >
>
> Error range would depend on things like standard deviation and
> repetition. It might be good to keep update_file_mapped and see the
> impact. My concern is with large systems, the difference might be
> larger.
>
> --
> Three Cheers,
> Balbir
I made a patch(attached) using both local_irq_disable/enable and local_irq_save/restore.
local_irq_save/restore is used only in mem_cgroup_update_file_mapped.
And I attached a histogram graph of 30 times kernel build in root cgroup for each.
before_root: no irq operation(original)
after_root: local_irq_disable/enable for all
after2_root: local_irq_save/restore for all
after3_root: mixed version(attached)
hmm, there seems to be a tendency that before < after < after3 < after2 ?
Should I replace save/restore version to mixed version ?
Thanks,
Daisuke Nishimura.
===
include/linux/page_cgroup.h | 28 ++++++++++++++++++++++++++--
mm/memcontrol.c | 36 ++++++++++++++++++------------------
2 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index 30b0813..c0aca62 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -83,16 +83,40 @@ static inline enum zone_type page_cgroup_zid(struct page_cgroup *pc)
return page_zonenum(pc->page);
}
-static inline void lock_page_cgroup(struct page_cgroup *pc)
+static inline void __lock_page_cgroup(struct page_cgroup *pc)
{
bit_spin_lock(PCG_LOCK, &pc->flags);
}
-static inline void unlock_page_cgroup(struct page_cgroup *pc)
+static inline void __unlock_page_cgroup(struct page_cgroup *pc)
{
bit_spin_unlock(PCG_LOCK, &pc->flags);
}
+#define lock_page_cgroup_irq(pc) \
+ do { \
+ local_irq_disable(); \
+ __lock_page_cgroup(pc); \
+ } while (0)
+
+#define unlock_page_cgroup_irq(pc) \
+ do { \
+ __unlock_page_cgroup(pc); \
+ local_irq_enable(); \
+ } while (0)
+
+#define lock_page_cgroup_irqsave(pc, flags) \
+ do { \
+ local_irq_save(flags); \
+ __lock_page_cgroup(pc); \
+ } while (0)
+
+#define unlock_page_cgroup_irqrestore(pc, flags) \
+ do { \
+ __unlock_page_cgroup(pc); \
+ local_irq_restore(flags); \
+ } while (0)
+
#else /* CONFIG_CGROUP_MEM_RES_CTLR */
struct page_cgroup;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 02ea959..11d483e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1354,12 +1354,13 @@ void mem_cgroup_update_file_mapped(struct page *page, int val)
{
struct mem_cgroup *mem;
struct page_cgroup *pc;
+ unsigned long flags;
pc = lookup_page_cgroup(page);
if (unlikely(!pc))
return;
- lock_page_cgroup(pc);
+ lock_page_cgroup_irqsave(pc, flags);
mem = pc->mem_cgroup;
if (!mem)
goto done;
@@ -1373,7 +1374,7 @@ void mem_cgroup_update_file_mapped(struct page *page, int val)
__this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_FILE_MAPPED], val);
done:
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irqrestore(pc, flags);
}
/*
@@ -1711,7 +1712,7 @@ struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page)
VM_BUG_ON(!PageLocked(page));
pc = lookup_page_cgroup(page);
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
if (PageCgroupUsed(pc)) {
mem = pc->mem_cgroup;
if (mem && !css_tryget(&mem->css))
@@ -1725,7 +1726,7 @@ struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page)
mem = NULL;
rcu_read_unlock();
}
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
return mem;
}
@@ -1742,9 +1743,9 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *mem,
if (!mem)
return;
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
if (unlikely(PageCgroupUsed(pc))) {
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
mem_cgroup_cancel_charge(mem);
return;
}
@@ -1774,7 +1775,7 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *mem,
mem_cgroup_charge_statistics(mem, pc, true);
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
/*
* "charge_statistics" updated event counter. Then, check it.
* Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
@@ -1844,12 +1845,12 @@ static int mem_cgroup_move_account(struct page_cgroup *pc,
struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge)
{
int ret = -EINVAL;
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
if (PageCgroupUsed(pc) && pc->mem_cgroup == from) {
__mem_cgroup_move_account(pc, from, to, uncharge);
ret = 0;
}
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
/*
* check events
*/
@@ -1977,16 +1978,15 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
if (!(gfp_mask & __GFP_WAIT)) {
struct page_cgroup *pc;
-
pc = lookup_page_cgroup(page);
if (!pc)
return 0;
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
if (PageCgroupUsed(pc)) {
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
return 0;
}
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
}
if (unlikely(!mm && !mem))
@@ -2182,7 +2182,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
if (unlikely(!pc || !PageCgroupUsed(pc)))
return NULL;
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
mem = pc->mem_cgroup;
@@ -2221,7 +2221,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
*/
mz = page_cgroup_zoneinfo(pc);
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
memcg_check_events(mem, page);
/* at swapout, this memcg will be accessed to record to swap */
@@ -2231,7 +2231,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
return mem;
unlock_out:
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
return NULL;
}
@@ -2424,12 +2424,12 @@ int mem_cgroup_prepare_migration(struct page *page, struct mem_cgroup **ptr)
return 0;
pc = lookup_page_cgroup(page);
- lock_page_cgroup(pc);
+ lock_page_cgroup_irq(pc);
if (PageCgroupUsed(pc)) {
mem = pc->mem_cgroup;
css_get(&mem->css);
}
- unlock_page_cgroup(pc);
+ unlock_page_cgroup_irq(pc);
if (mem) {
ret = __mem_cgroup_try_charge(NULL, GFP_KERNEL, &mem, false);
Attachment:
root_cgroup.bmp.gz
Description: Binary data