Re: [PATCH] Add file based RSS accounting for memory resourcecontroller (v2)

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> [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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/