From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757655Ab0JEHLz (ORCPT ); Tue, 5 Oct 2010 03:11:55 -0400 Received: from smtp-out.google.com ([74.125.121.35]:3776 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756777Ab0JEHLy (ORCPT ); Tue, 5 Oct 2010 03:11:54 -0400 DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=from:to:cc:subject:references:date:in-reply-to:message-id: user-agent:mime-version:content-type; b=L+oC191QzhClINvLdY5Nz0Mq/t/OusNWvwXoD0wfCIrY7vfaxw23wzkC1zeSnIlku 5q7AuCfZ+UJWbKlHZJ1Cw== From: Greg Thelen To: KAMEZAWA Hiroyuki Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, containers@lists.osdl.org, Andrea Righi , Balbir Singh , Daisuke Nishimura Subject: Re: [PATCH 03/10] memcg: create extensible page stat update routines References: <1286175485-30643-1-git-send-email-gthelen@google.com> <1286175485-30643-4-git-send-email-gthelen@google.com> <20101005155142.847b1529.kamezawa.hiroyu@jp.fujitsu.com> Date: Tue, 05 Oct 2010 00:10:30 -0700 In-Reply-To: <20101005155142.847b1529.kamezawa.hiroyu@jp.fujitsu.com> (KAMEZAWA Hiroyuki's message of "Tue, 5 Oct 2010 15:51:42 +0900") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org KAMEZAWA Hiroyuki writes: > On Sun, 3 Oct 2010 23:57:58 -0700 > Greg Thelen wrote: > >> Replace usage of the mem_cgroup_update_file_mapped() memcg >> statistic update routine with two new routines: >> * mem_cgroup_inc_page_stat() >> * mem_cgroup_dec_page_stat() >> >> As before, only the file_mapped statistic is managed. However, >> these more general interfaces allow for new statistics to be >> more easily added. New statistics are added with memcg dirty >> page accounting. >> >> Signed-off-by: Greg Thelen >> Signed-off-by: Andrea Righi > > Acked-by: KAMEZAWA Hiroyuki > > a nitpick. see below. > >> --- >> include/linux/memcontrol.h | 31 ++++++++++++++++++++++++++++--- >> mm/memcontrol.c | 17 ++++++++--------- >> mm/rmap.c | 4 ++-- >> 3 files changed, 38 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index 159a076..7c7bec4 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -25,6 +25,11 @@ struct page_cgroup; >> struct page; >> struct mm_struct; >> >> +/* Stats that can be updated by kernel. */ >> +enum mem_cgroup_write_page_stat_item { >> + MEMCG_NR_FILE_MAPPED, /* # of pages charged as file rss */ >> +}; >> + >> extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan, >> struct list_head *dst, >> unsigned long *scanned, int order, >> @@ -121,7 +126,22 @@ static inline bool mem_cgroup_disabled(void) >> return false; >> } >> >> -void mem_cgroup_update_file_mapped(struct page *page, int val); >> +void mem_cgroup_update_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx, >> + int val); >> + >> +static inline void mem_cgroup_inc_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx) >> +{ >> + mem_cgroup_update_page_stat(page, idx, 1); >> +} >> + >> +static inline void mem_cgroup_dec_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx) >> +{ >> + mem_cgroup_update_page_stat(page, idx, -1); >> +} >> + >> unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, >> gfp_t gfp_mask); >> u64 mem_cgroup_get_limit(struct mem_cgroup *mem); >> @@ -293,8 +313,13 @@ mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p) >> { >> } >> >> -static inline void mem_cgroup_update_file_mapped(struct page *page, >> - int val) >> +static inline void mem_cgroup_inc_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx) >> +{ >> +} >> + >> +static inline void mem_cgroup_dec_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx) >> { >> } >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 512cb12..f4259f4 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -1592,7 +1592,9 @@ bool mem_cgroup_handle_oom(struct mem_cgroup *mem, gfp_t mask) >> * possibility of race condition. If there is, we take a lock. >> */ >> >> -static void mem_cgroup_update_file_stat(struct page *page, int idx, int val) >> +void mem_cgroup_update_page_stat(struct page *page, >> + enum mem_cgroup_write_page_stat_item idx, >> + int val) >> { >> struct mem_cgroup *mem; >> struct page_cgroup *pc = lookup_page_cgroup(page); >> @@ -1615,30 +1617,27 @@ static void mem_cgroup_update_file_stat(struct page *page, int idx, int val) >> goto out; >> } >> >> - this_cpu_add(mem->stat->count[idx], val); >> - >> switch (idx) { >> - case MEM_CGROUP_STAT_FILE_MAPPED: >> + case MEMCG_NR_FILE_MAPPED: >> if (val > 0) >> SetPageCgroupFileMapped(pc); >> else if (!page_mapped(page)) >> ClearPageCgroupFileMapped(pc); >> + idx = MEM_CGROUP_STAT_FILE_MAPPED; >> break; >> default: >> BUG(); >> } >> >> + this_cpu_add(mem->stat->count[idx], val); >> + > > Why you move this_cpu_add() placement ? > (This placement is ok but I just wonder..) > > Thanks, > -Kame The reason this_cpu_add() is moved to after the switch is because the switch is needed to convert the input parameter from an enum mem_cgroup_write_page_stat_item (example: MEMCG_NR_FILE_MAPPED) to enum mem_cgroup_stat_index (example: MEM_CGROUP_STAT_FILE_MAPPED) before indexing into the count array. Also in subsequent patches (in this series) "val" is updated depending on page_cgroup flags before usage by this_cpu_add().