| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 16 Apr 2009 17:33:16 +0530
From: Balbir Singh <balbir@...ux.vnet.ibm.com>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] Add file based RSS accounting for memory resource
controller (v2)
* KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> [2009-04-16 17:15:35]:
>
> > Sorry, some troubles found. Ignore above Ack. 3points now.
> >
> > 1. get_cpu should be after (*)
> > ==mem_cgroup_update_mapped_file_stat()
> > + int cpu = get_cpu();
> > +
> > + if (!page_is_file_cache(page))
> > + return;
> > +
> > + if (unlikely(!mm))
> > + mm = &init_mm;
> > +
> > + mem = try_get_mem_cgroup_from_mm(mm);
> > + if (!mem)
> > + return;
> > + ----------------------------------------(*)
> > + stat = &mem->stat;
> > + cpustat = &stat->cpustat[cpu];
> > +
> > + __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, val);
> > + put_cpu();
> > +}
> > ==
Yes or I should have a goto
> >
> > 2. In above, "mem" shouldn't be got from "mm"....please get "mem" from page_cgroup.
> > (Because it's file cache, pc->mem_cgroup is not NULL always.)
Hmmm.. not sure I understand this part. Are you suggesting that mm can
be NULL? I added the check for !mm as a safety check. Since this
routine is only called from rmap context, mm is not NULL, hence mem
should not be NULL. Did you find a race between mm->owner assignment
and lookup via mm->owner?
> >
> > I saw this very easily.
> > ==
> > Cache: 4096
> > mapped_file: 20480
> > ==
> >
> > 3. at force_empty().
> > ==
> > +
> > + cpu = get_cpu();
> > + /* Update mapped_file data for mem_cgroup "from" */
> > + stat = &from->stat;
> > + cpustat = &stat->cpustat[cpu];
> > + __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, -1);
> > +
> > + /* Update mapped_file data for mem_cgroup "to" */
> > + stat = &to->stat;
> > + cpustat = &stat->cpustat[cpu];
> > + __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, 1);
> > + put_cpu();
> >
> > This just breaks counter when page is not mapped. please check page_mapped().
> >
> > like this:
> > ==
> > if (page_is_file_cache(page) && page_mapped(page)) {
> > modify counter.
> > }
>
Oh! yeah..
> ==
> >
> > and call lock_page_cgroup() in mem_cgroup_update_mapped_file_stat().
> >
> > This will be slow, but optimization will be very tricky and need some amount of time.
> >
>
> This is my fix for above 3 problems. but plz do as you like.
> I'm not very intersted in details.
> ==
>
> ---
> Index: mmotm-2.6.30-Apr14/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.30-Apr14.orig/mm/memcontrol.c
> +++ mmotm-2.6.30-Apr14/mm/memcontrol.c
> @@ -322,33 +322,39 @@ static bool mem_cgroup_is_obsolete(struc
> return css_is_removed(&mem->css);
> }
>
> -/*
> - * Currently used to update mapped file statistics, but the routine can be
> - * generalized to update other statistics as well.
> - */
> -void mem_cgroup_update_mapped_file_stat(struct page *page, struct mm_struct *mm,
> - int val)
> +void mem_cgroup_update_mapped_file_stat(struct page *page, bool map)
> {
> struct mem_cgroup *mem;
> struct mem_cgroup_stat *stat;
> struct mem_cgroup_stat_cpu *cpustat;
> - int cpu = get_cpu();
> + struct page_cgroup *pc;
> + int cpu;
>
> if (!page_is_file_cache(page))
> return;
>
> - if (unlikely(!mm))
> - mm = &init_mm;
> -
> - mem = try_get_mem_cgroup_from_mm(mm);
> - if (!mem)
> + pc = lookup_page_cgroup(page);
> + if (unlikely(!pc))
> return;
> -
> - stat = &mem->stat;
> - cpustat = &stat->cpustat[cpu];
> -
> - __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, val);
> - put_cpu();
> + lock_page_cgroup(pc);
> + mem = pc->mem_cgroup;
> + if (mem) {
> + cpu = get_cpu();
> + stat = &mem->stat;
> + cpustat = &stat->cpustat[cpu];
> + if (map)
> + __mem_cgroup_stat_add_safe(cpustat,
> + MEM_CGROUP_STAT_MAPPED_FILE, 1);
> + else
> + __mem_cgroup_stat_add_safe(cpustat,
> + MEM_CGROUP_STAT_MAPPED_FILE, -1);
> + put_cpu();
> + }
> + if (map)
> + SetPageCgroupMapped(pc);
> + else
> + ClearPageCgroupMapped(pc);
> + unlock_page_cgroup(pc);
> }
>
> /*
> @@ -1149,17 +1155,19 @@ static int mem_cgroup_move_account(struc
> res_counter_uncharge(&from->res, PAGE_SIZE);
> mem_cgroup_charge_statistics(from, pc, false);
>
> - cpu = get_cpu();
> - /* Update mapped_file data for mem_cgroup "from" */
> - stat = &from->stat;
> - cpustat = &stat->cpustat[cpu];
> - __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, -1);
> -
> - /* Update mapped_file data for mem_cgroup "to" */
> - stat = &to->stat;
> - cpustat = &stat->cpustat[cpu];
> - __mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, 1);
> - put_cpu();
> + if (PageCgroupMapped(pc)) {
> + cpu = get_cpu();
> + /* Update mapped_file data for mem_cgroup "from" and "to" */
> + stat = &from->stat;
> + cpustat = &stat->cpustat[cpu];
> + __mem_cgroup_stat_add_safe(cpustat,
> + MEM_CGROUP_STAT_MAPPED_FILE, -1);
> + stat = &to->stat;
> + cpustat = &stat->cpustat[cpu];
> + __mem_cgroup_stat_add_safe(cpustat,
> + MEM_CGROUP_STAT_MAPPED_FILE, 1);
> + put_cpu();
> + }
>
> if (do_swap_account)
> res_counter_uncharge(&from->memsw, PAGE_SIZE);
> Index: mmotm-2.6.30-Apr14/include/linux/page_cgroup.h
> ===================================================================
> --- mmotm-2.6.30-Apr14.orig/include/linux/page_cgroup.h
> +++ mmotm-2.6.30-Apr14/include/linux/page_cgroup.h
> @@ -26,6 +26,7 @@ enum {
> PCG_LOCK, /* page cgroup is locked */
> PCG_CACHE, /* charged as cache */
> PCG_USED, /* this object is in use. */
> + PCG_MAPPED, /* mapped file cache */
> };
>
> #define TESTPCGFLAG(uname, lname) \
> @@ -46,6 +47,10 @@ TESTPCGFLAG(Cache, CACHE)
> TESTPCGFLAG(Used, USED)
> CLEARPCGFLAG(Used, USED)
>
> +TESTPCGFLAG(Mapped, USED)
> +CLEARPCGFLAG(Mapped, USED)
> +SETPCGFLAG(Mapped, USED)
> +
> static inline int page_cgroup_nid(struct page_cgroup *pc)
> {
> return page_to_nid(pc->page);
> Index: mmotm-2.6.30-Apr14/include/linux/memcontrol.h
> ===================================================================
> --- mmotm-2.6.30-Apr14.orig/include/linux/memcontrol.h
> +++ mmotm-2.6.30-Apr14/include/linux/memcontrol.h
> @@ -116,8 +116,7 @@ static inline bool mem_cgroup_disabled(v
> }
>
> extern bool mem_cgroup_oom_called(struct task_struct *task);
> -void mem_cgroup_update_mapped_file_stat(struct page *page, struct mm_struct *mm,
> - int val);
> +void mem_cgroup_update_mapped_file_stat(struct page *page, bool map);
> #else /* CONFIG_CGROUP_MEM_RES_CTLR */
> struct mem_cgroup;
>
> @@ -266,8 +265,7 @@ mem_cgroup_print_oom_info(struct mem_cgr
> }
>
> static inline void mem_cgroup_update_mapped_file_stat(struct page *page,
> - struct mm_struct *mm,
> - int val)
> + bool map)
> {
> }
>
> Index: mmotm-2.6.30-Apr14/mm/rmap.c
> ===================================================================
> --- mmotm-2.6.30-Apr14.orig/mm/rmap.c
> +++ mmotm-2.6.30-Apr14/mm/rmap.c
> @@ -690,7 +690,7 @@ void page_add_file_rmap(struct page *pag
> {
> if (atomic_inc_and_test(&page->_mapcount)) {
> __inc_zone_page_state(page, NR_FILE_MAPPED);
> - mem_cgroup_update_mapped_file_stat(page, vma->vm_mm, 1);
> + mem_cgroup_update_mapped_file_stat(page, true);
> }
> }
>
> @@ -740,7 +740,7 @@ void page_remove_rmap(struct page *page,
> mem_cgroup_uncharge_page(page);
> __dec_zone_page_state(page,
> PageAnon(page) ? NR_ANON_PAGES : NR_FILE_MAPPED);
> - mem_cgroup_update_mapped_file_stat(page, vma->vm_mm, -1);
> + mem_cgroup_update_mapped_file_stat(page, false);
> /*
> * It would be tidy to reset the PageAnon mapping here,
> * but that might overwrite a racing page_add_anon_rmap
>
>
>
>
>
>
--
Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists