Re: [PATCH 2/5] use ID in page cgroup

From: KAMEZAWA Hiroyuki
Date: Thu Jul 29 2010 - 20:17:56 EST

Next message: J.A. Magallón: "Re: Questions and problems with NFS4"
Previous message: Mike Frysinger: "Re: [PATCH net-next 2/3] drivers/net/bfin_mac.c: Use pr_<level>, netdev_<level>"
In reply to: Greg Thelen: "Re: [PATCH 2/5] use ID in page cgroup"
Next in thread: KAMEZAWA Hiroyuki: "[PATCH 3/5] low cost updates of file stat for memcg"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 29 Jul 2010 11:31:15 -0700
Greg Thelen <gthelen@xxxxxxxxxx> wrote:

> KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> writes:
>
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >
> > Now, addresses of memory cgroup can be calculated by their ID without complex.
> > This patch relplaces pc->mem_cgroup from a pointer to a unsigned short.
> > On 64bit architecture, this offers us more 6bytes room per page_cgroup.
> > Use 2bytes for blkio-cgroup's page tracking. More 4bytes will be used for
> > some light-weight concurrent access.
> >
> > We may able to move this id onto flags field but ...go step by step.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > ---
> > include/linux/page_cgroup.h | 3 ++-
> > mm/memcontrol.c | 40 +++++++++++++++++++++++++---------------
> > mm/page_cgroup.c | 2 +-
> > 3 files changed, 28 insertions(+), 17 deletions(-)
> >
> > Index: mmotm-0727/include/linux/page_cgroup.h
> > ===================================================================
> > --- mmotm-0727.orig/include/linux/page_cgroup.h
> > +++ mmotm-0727/include/linux/page_cgroup.h
> > @@ -12,7 +12,8 @@
> > */
> > struct page_cgroup {
> > unsigned long flags;
> > - struct mem_cgroup *mem_cgroup;
> > + unsigned short mem_cgroup; /* ID of assigned memory cgroup */
> > + unsigned short blk_cgroup; /* Not Used..but will be. */
> > struct page *page;
> > struct list_head lru; /* per cgroup LRU list */
> > };
> > Index: mmotm-0727/mm/page_cgroup.c
> > ===================================================================
> > --- mmotm-0727.orig/mm/page_cgroup.c
> > +++ mmotm-0727/mm/page_cgroup.c
> > @@ -15,7 +15,7 @@ static void __meminit
> > __init_page_cgroup(struct page_cgroup *pc, unsigned long pfn)
> > {
> > pc->flags = 0;
> > - pc->mem_cgroup = NULL;
> > + pc->mem_cgroup = 0;
> > pc->page = pfn_to_page(pfn);
> > INIT_LIST_HEAD(&pc->lru);
> > }
> > Index: mmotm-0727/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-0727.orig/mm/memcontrol.c
> > +++ mmotm-0727/mm/memcontrol.c
> > @@ -376,7 +376,7 @@ struct cgroup_subsys_state *mem_cgroup_c
> > static struct mem_cgroup_per_zone *
> > page_cgroup_zoneinfo(struct page_cgroup *pc)
> > {
> > - struct mem_cgroup *mem = pc->mem_cgroup;
> > + struct mem_cgroup *mem = id_to_memcg(pc->mem_cgroup);
> > int nid = page_cgroup_nid(pc);
> > int zid = page_cgroup_zid(pc);
> >
> > @@ -581,7 +581,11 @@ static void mem_cgroup_charge_statistics
> > bool charge)
> > {
> > int val = (charge) ? 1 : -1;
> > -
> > + if (pc->mem_cgroup == 0) {
> > + show_stack(NULL, NULL);
> > + printk("charge to 0\n");
> > + while(1);
> > + }
> Why hang the task here. If this is bad then maybe BUG_ON()?

Ouch, debug code is remaining.

> > preempt_disable();
> >
> > if (PageCgroupCache(pc))
> > @@ -718,6 +722,11 @@ static inline bool mem_cgroup_is_root(st
> > return (mem == root_mem_cgroup);
> > }
> >
> > +static inline bool mem_cgroup_is_rootid(unsigned short id)
> > +{
> > + return (id == 1);
> > +}
> > +
> > /*
> > * Following LRU functions are allowed to be used without PCG_LOCK.
> > * Operations are called by routine of global LRU independently from memcg.
> > @@ -750,7 +759,7 @@ void mem_cgroup_del_lru_list(struct page
> > */
> > mz = page_cgroup_zoneinfo(pc);
> > MEM_CGROUP_ZSTAT(mz, lru) -= 1;
> > - if (mem_cgroup_is_root(pc->mem_cgroup))
> > + if (mem_cgroup_is_rootid(pc->mem_cgroup))
> > return;
> > VM_BUG_ON(list_empty(&pc->lru));
> > list_del_init(&pc->lru);
> > @@ -777,7 +786,7 @@ void mem_cgroup_rotate_lru_list(struct p
> > */
> > smp_rmb();
> > /* unused or root page is not rotated. */
> > - if (!PageCgroupUsed(pc) || mem_cgroup_is_root(pc->mem_cgroup))
> > + if (!PageCgroupUsed(pc) || mem_cgroup_is_rootid(pc->mem_cgroup))
> > return;
> > mz = page_cgroup_zoneinfo(pc);
> > list_move(&pc->lru, &mz->lists[lru]);
> > @@ -803,7 +812,7 @@ void mem_cgroup_add_lru_list(struct page
> > mz = page_cgroup_zoneinfo(pc);
> > MEM_CGROUP_ZSTAT(mz, lru) += 1;
> > SetPageCgroupAcctLRU(pc);
> > - if (mem_cgroup_is_root(pc->mem_cgroup))
> > + if (mem_cgroup_is_rootid(pc->mem_cgroup))
> > return;
> > list_add(&pc->lru, &mz->lists[lru]);
> > }
> > @@ -1471,7 +1480,7 @@ void mem_cgroup_update_file_mapped(struc
> > return;
> >
> > lock_page_cgroup(pc);
> > - mem = pc->mem_cgroup;
> > + mem = id_to_memcg(pc->mem_cgroup);
> > if (!mem || !PageCgroupUsed(pc))
> > goto done;
> >
> > @@ -1859,7 +1868,7 @@ struct mem_cgroup *try_get_mem_cgroup_fr
> > pc = lookup_page_cgroup(page);
> > lock_page_cgroup(pc);
> > if (PageCgroupUsed(pc)) {
> > - mem = pc->mem_cgroup;
> > + mem = id_to_memcg(pc->mem_cgroup);
> > if (mem && !css_tryget(&mem->css))
> > mem = NULL;
> > } else if (PageSwapCache(page)) {
> > @@ -1895,7 +1904,7 @@ static void __mem_cgroup_commit_charge(s
> > return;
> > }
> >
> > - pc->mem_cgroup = mem;
> > + pc->mem_cgroup = css_id(&mem->css);
> > /*
> > * We access a page_cgroup asynchronously without lock_page_cgroup().
> > * Especially when a page_cgroup is taken from a page, pc->mem_cgroup
> > @@ -1953,7 +1962,7 @@ static void __mem_cgroup_move_account(st
> > VM_BUG_ON(PageLRU(pc->page));
> > VM_BUG_ON(!PageCgroupLocked(pc));
> > VM_BUG_ON(!PageCgroupUsed(pc));
> > - VM_BUG_ON(pc->mem_cgroup != from);
> > + VM_BUG_ON(id_to_memcg(pc->mem_cgroup) != from);
> >
> > if (PageCgroupFileMapped(pc)) {
> > /* Update mapped_file data for mem_cgroup */
> > @@ -1968,7 +1977,7 @@ static void __mem_cgroup_move_account(st
> > mem_cgroup_cancel_charge(from);
> >
> > /* caller should have done css_get */
> > - pc->mem_cgroup = to;
> > + pc->mem_cgroup = css_id(&to->css);
> > mem_cgroup_charge_statistics(to, pc, true);
> > /*
> > * We charges against "to" which may not have any tasks. Then, "to"
> > @@ -1988,7 +1997,7 @@ static int mem_cgroup_move_account(struc
> > {
> > int ret = -EINVAL;
> > lock_page_cgroup(pc);
> > - if (PageCgroupUsed(pc) && pc->mem_cgroup == from) {
> > + if (PageCgroupUsed(pc) && id_to_memcg(pc->mem_cgroup) == from) {
> > __mem_cgroup_move_account(pc, from, to, uncharge);
> > ret = 0;
> > }
> > @@ -2327,9 +2336,9 @@ __mem_cgroup_uncharge_common(struct page
> >
> > lock_page_cgroup(pc);
> >
> > - mem = pc->mem_cgroup;
> > + mem = id_to_memcg(pc->mem_cgroup);
> >
> > - if (!PageCgroupUsed(pc))
> > + if (!mem || !PageCgroupUsed(pc))
> Why add the extra !mem check here?
>
> Can PageCgroupUsed() return true if mem==NULL?
>
mem && PageCgroupUsed() => what we want
mem && !PageCgroupUsed() => can be true
!mem && PageCgroupUsed() => never happens(bug?)
!mem && !PageCgroupUsed() => a clean state.

AH, yes. debug code again.

Thanks,
-Kame

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: J.A. Magallón: "Re: Questions and problems with NFS4"
Previous message: Mike Frysinger: "Re: [PATCH net-next 2/3] drivers/net/bfin_mac.c: Use pr_<level>, netdev_<level>"
In reply to: Greg Thelen: "Re: [PATCH 2/5] use ID in page cgroup"
Next in thread: KAMEZAWA Hiroyuki: "[PATCH 3/5] low cost updates of file stat for memcg"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]