Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751652AbZDPIRY (ORCPT ); Thu, 16 Apr 2009 04:17:24 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750966AbZDPIRK (ORCPT ); Thu, 16 Apr 2009 04:17:10 -0400 Received: from fgwmail5.fujitsu.co.jp ([192.51.44.35]:60536 "EHLO fgwmail5.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750749AbZDPIRH (ORCPT ); Thu, 16 Apr 2009 04:17:07 -0400 Date: Thu, 16 Apr 2009 17:15:35 +0900 From: KAMEZAWA Hiroyuki To: KAMEZAWA Hiroyuki Cc: balbir@linux.vnet.ibm.com, "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , Andrew Morton Subject: Re: [PATCH] Add file based RSS accounting for memory resource controller (v2) Message-Id: <20090416171535.cfc4ca84.kamezawa.hiroyu@jp.fujitsu.com> In-Reply-To: <20090416164036.03d7347a.kamezawa.hiroyu@jp.fujitsu.com> References: <20090415120510.GX7082@balbir.in.ibm.com> <20090416095303.b4106e9f.kamezawa.hiroyu@jp.fujitsu.com> <20090416015955.GB7082@balbir.in.ibm.com> <20090416110246.c3fef293.kamezawa.hiroyu@jp.fujitsu.com> <20090416164036.03d7347a.kamezawa.hiroyu@jp.fujitsu.com> Organization: FUJITSU Co. LTD. X-Mailer: Sylpheed 2.5.0 (GTK+ 2.10.14; i686-pc-mingw32) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6923 Lines: 240 > 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(); > +} > == > > 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.) > > 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. > } > == > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/