Re: [PATCH] Add the pagefault count into memcg stats.

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Sat, 26 Mar 2011 17:14:44 -0700
Ying Han <yinghan@google.com> wrote:

> Two new stats in per-memcg memory.stat which tracks the number of
> page faults and number of major page faults.
> 
> "pgfault"
> "pgmajfault"
> 
> It is valuable to track the two stats for both measuring application's
> performance as well as the efficiency of the kernel page reclaim path.
> 
> Functional test: check the total number of pgfault/pgmajfault of all
> memcgs and compare with global vmstat value:
> 
> $ cat /proc/vmstat | grep fault
> pgfault 1070751
> pgmajfault 553
> 
> $ cat /dev/cgroup/memory.stat | grep fault
> pgfault 1069962
> pgmajfault 553
> total_pgfault 1069966
> total_pgmajfault 553
> 
> $ cat /dev/cgroup/A/memory.stat | grep fault
> pgfault 199
> pgmajfault 0
> total_pgfault 199
> total_pgmajfault 0
> 
> Performance test: run page fault test(pft) wit 16 thread on faulting in 15G
> anon pages in 16G container. There is no regression noticed on the "flt/cpu/s"
> 
> Sample output from pft:
> TAG pft:anon-sys-default:
>   Gb  Thr CLine   User     System     Wall    flt/cpu/s fault/wsec
>   15   16   1     0.67s   232.11s    14.68s   16892.130 267796.518
> 
> $ ./ministat mmotm.txt mmotm_fault.txt
> x mmotm.txt (w/o patch)
> + mmotm_fault.txt (w/ patch)
> +-------------------------------------------------------------------------+
>     N           Min           Max        Median           Avg        Stddev
> x  10     16682.962     17344.027     16913.524     16928.812      166.5362
> +  10      16696.49      17480.09     16949.143     16951.448     223.56288
> No difference proven at 95.0% confidence
> 
> Signed-off-by: Ying Han <yinghan@google.com>

Hmm, maybe useful ? (It's good to describe what is difference with PGPGIN)
Especially, you should show why this is useful than per process pgfault count.
What I thought of this, I thought that I need per-process information, finally...
and didn't add this.

Anyway, I have a request for the style of the function. (see below)


> ---
>  Documentation/cgroups/memory.txt |    4 +++
>  fs/ncpfs/mmap.c                  |    2 +
>  include/linux/memcontrol.h       |   22 +++++++++++++++
>  mm/filemap.c                     |    1 +
>  mm/memcontrol.c                  |   54 ++++++++++++++++++++++++++++++++++++++
>  mm/memory.c                      |    2 +
>  mm/shmem.c                       |    1 +
>  7 files changed, 86 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index b6ed61c..2db6103 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -385,6 +385,8 @@ mapped_file	- # of bytes of mapped file (includes tmpfs/shmem)
>  pgpgin		- # of pages paged in (equivalent to # of charging events).
>  pgpgout		- # of pages paged out (equivalent to # of uncharging events).
>  swap		- # of bytes of swap usage
> +pgfault		- # of page faults.
> +pgmajfault	- # of major page faults.
>  inactive_anon	- # of bytes of anonymous memory and swap cache memory on
>  		LRU list.
>  active_anon	- # of bytes of anonymous and swap cache memory on active
> @@ -406,6 +408,8 @@ total_mapped_file	- sum of all children's "cache"
>  total_pgpgin		- sum of all children's "pgpgin"
>  total_pgpgout		- sum of all children's "pgpgout"
>  total_swap		- sum of all children's "swap"
> +total_pgfault		- sum of all children's "pgfault"
> +total_pgmajfault	- sum of all children's "pgmajfault"
>  total_inactive_anon	- sum of all children's "inactive_anon"
>  total_active_anon	- sum of all children's "active_anon"
>  total_inactive_file	- sum of all children's "inactive_file"
> diff --git a/fs/ncpfs/mmap.c b/fs/ncpfs/mmap.c
> index a7c07b4..adb3f45 100644
> --- a/fs/ncpfs/mmap.c
> +++ b/fs/ncpfs/mmap.c
> @@ -16,6 +16,7 @@
>  #include <linux/mman.h>
>  #include <linux/string.h>
>  #include <linux/fcntl.h>
> +#include <linux/memcontrol.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/system.h>
> @@ -92,6 +93,7 @@ static int ncp_file_mmap_fault(struct vm_area_struct *area,
>  	 * -- wli
>  	 */
>  	count_vm_event(PGMAJFAULT);
> +	mem_cgroup_pgmajfault_from_mm(area->vm_mm);

Could you do this as  mem_cgroup_count_vm_event(area->vm_mm, PGMAJFAULT) ?

<snip>

> +void mem_cgroup_pgfault_from_mm(struct mm_struct *mm)
> +{
> +	struct mem_cgroup *mem;
> +
> +	if (!mm)
> +		return;
> +
> +	rcu_read_lock();
> +	mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
> +	if (unlikely(!mem))
> +		goto out;
> +	mem_cgroup_pgfault(mem, 1);
> +
> +out:
> +	rcu_read_unlock();
> +}
> +
> +void mem_cgroup_pgmajfault_from_mm(struct mm_struct *mm)
> +{
> +	struct mem_cgroup *mem;
> +
> +	if (!mm)
> +		return;
> +
> +	rcu_read_lock();
> +	mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
> +	if (unlikely(!mem))
> +		goto out;
> +	mem_cgroup_pgmajfault(mem, 1);
> +out:
> +	rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(mem_cgroup_pgmajfault_from_mm);
> +

Then, you can do above 2 in a function.


Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>