Re: [BUGFIX][PATCH] fix race in file_mapped accounting in memcg

[Daisuke Nishimura]

On Wed, 24 Mar 2010 15:43:24 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> A fix for race in file_mapped statistics. I noticed this race while discussing
> Andrea's dirty accounting patch series. 
Yes, I've also noticed this race.
IMHO, one cause of this race is that we check "page_mapped(page)" while we might
not take the pte lock referring the page.
So, I suggested Andrea to introduce PCG_FILE_MAPPED and protect the bit by
page cgroup lock.

> At the end of discusstion, I said "please don't touch file mapped". So, this bugfix
> should be posted as an independent patch.
agreed.

> Tested on the latest mmotm.
> 
> Thanks,
> -Kame
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> 
> Now, memcg's FILE_MAPPED accounting has following race with
> move_account (happens at rmdir()).
> 
>     increment page->mapcount (rmap.c)
>     mem_cgroup_update_file_mapped()           move_account()
> 					      lock_page_cgroup()
> 					      check page_mapped() if
> 					      page_mapped(page)>1 {
> 						FILE_MAPPED -1 from old memcg
> 						FILE_MAPPED +1 to old memcg
> 					      }
> 					      .....
> 					      overwrite pc->mem_cgroup
> 					      unlock_page_cgroup()
>     lock_page_cgroup()
>     FILE_MAPPED + 1 to pc->mem_cgroup
>     unlock_page_cgroup()
> 
> Then,
> 	old memcg (-1 file mapped)
> 	new memcg (+2 file mapped)
> 
> 
> This happens because move_account see page_mapped() which is not guarded by
> lock_page_cgroup(). This patch adds FILE_MAPPED flag to page_cgroup and
> move account information based on it. Now, all checks are synchronous with
> lock_page_cgroup().
> 
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
Looks good to me.

	Reviewed-by: Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx>

Thanks,
Daisuke Nishimura.

> ---
>  include/linux/page_cgroup.h |    6 ++++++
>  mm/memcontrol.c             |   18 +++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> Index: mmotm-2.6.34-Mar23/include/linux/page_cgroup.h
> ===================================================================
> --- mmotm-2.6.34-Mar23.orig/include/linux/page_cgroup.h
> +++ mmotm-2.6.34-Mar23/include/linux/page_cgroup.h
> @@ -39,6 +39,7 @@ enum {
>  	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_ACCT_LRU, /* page has been accounted for */
> +	PCG_FILE_MAPPED, /* page is accounted as "mapped" */
>  };
>  
>  #define TESTPCGFLAG(uname, lname)			\
> @@ -73,6 +74,11 @@ CLEARPCGFLAG(AcctLRU, ACCT_LRU)
>  TESTPCGFLAG(AcctLRU, ACCT_LRU)
>  TESTCLEARPCGFLAG(AcctLRU, ACCT_LRU)
>  
> +
> +SETPCGFLAG(FileMapped, FILE_MAPPED)
> +CLEARPCGFLAG(FileMapped, FILE_MAPPED)
> +TESTPCGFLAG(FileMapped, FILE_MAPPED)
> +
>  static inline int page_cgroup_nid(struct page_cgroup *pc)
>  {
>  	return page_to_nid(pc->page);
> Index: mmotm-2.6.34-Mar23/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.34-Mar23.orig/mm/memcontrol.c
> +++ mmotm-2.6.34-Mar23/mm/memcontrol.c
> @@ -1430,16 +1430,19 @@ void mem_cgroup_update_file_mapped(struc
>  
>  	lock_page_cgroup(pc);
>  	mem = pc->mem_cgroup;
> -	if (!mem)
> -		goto done;
> -
> -	if (!PageCgroupUsed(pc))
> +	if (!mem || !PageCgroupUsed(pc))
>  		goto done;
>  
>  	/*
>  	 * Preemption is already disabled. We can use __this_cpu_xxx
>  	 */
> -	__this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_FILE_MAPPED], val);
> +	if (val > 0) {
> +		__this_cpu_inc(mem->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
> +		SetPageCgroupFileMapped(pc);
> +	} else {
> +		__this_cpu_dec(mem->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
> +		ClearPageCgroupFileMapped(pc);
> +	}
>  
>  done:
>  	unlock_page_cgroup(pc);
> @@ -1872,16 +1875,13 @@ static void __mem_cgroup_commit_charge(s
>  static void __mem_cgroup_move_account(struct page_cgroup *pc,
>  	struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge)
>  {
> -	struct page *page;
> -
>  	VM_BUG_ON(from == to);
>  	VM_BUG_ON(PageLRU(pc->page));
>  	VM_BUG_ON(!PageCgroupLocked(pc));
>  	VM_BUG_ON(!PageCgroupUsed(pc));
>  	VM_BUG_ON(pc->mem_cgroup != from);
>  
> -	page = pc->page;
> -	if (page_mapped(page) && !PageAnon(page)) {
> +	if (PageCgroupFileMapped(pc)) {
>  		/* Update mapped_file data for mem_cgroup */
>  		preempt_disable();
>  		__this_cpu_dec(from->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
> 
--
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/