| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20090415090130.26813449.kamezawa.hiroyu@jp.fujitsu.com>
Date: Wed, 15 Apr 2009 09:01:30 +0900
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To: balbir@...ux.vnet.ibm.com
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC][PATCH] Add file RSS accounting to the memory resource
controller
I think your concept is good. A few comments below.
On Tue, 14 Apr 2009 23:37:06 +0530
Balbir Singh <balbir@...ux.vnet.ibm.com> wrote:
>
> Feature: Add file RSS tracking per memory cgroup
>
> From: Balbir Singh <balbir@...ux.vnet.ibm.com>
>
> Testing
>
> 1. Feature enabled, built and booted the kernel. Mounted the memory
> resource controller and verified the statistics
> 2. Compiled with CGROUP_MEM_RES_CTLR disabled
>
> We currently don't track file RSS, the RSS we report is actually anon RSS.
> All the file mapped pages, come in through the page cache and get accounted
> there. This patch adds support for accounting file RSS pages. It should
>
> 1. Help improve the metrics reported by the memory resource controller
> 2. Will form the basis for a future shared memory accounting heuristic
> that has been proposed by Kamezawa.
>
> Unfortunately, we cannot rename the existing "rss" keyword used in memory.stat
> to "anon_rss". We however, add "file_rss" data and hope to educate the end
> user through documentation.
> ---
>
> include/linux/memcontrol.h | 7 +++++++
> include/linux/rmap.h | 4 ++--
> mm/filemap_xip.c | 2 +-
> mm/fremap.c | 2 +-
> mm/memcontrol.c | 34 +++++++++++++++++++++++++++++++++-
> mm/memory.c | 8 ++++----
> mm/migrate.c | 2 +-
> mm/rmap.c | 13 ++++++++-----
> 8 files changed, 57 insertions(+), 15 deletions(-)
>
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 18146c9..c844a13 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -116,6 +116,8 @@ static inline bool mem_cgroup_disabled(void)
> }
>
> extern bool mem_cgroup_oom_called(struct task_struct *task);
> +extern void
> +mem_cgroup_update_statistics(struct page *page, struct mm_struct *mm, int val);
>
> #else /* CONFIG_CGROUP_MEM_RES_CTLR */
> struct mem_cgroup;
> @@ -264,6 +266,11 @@ mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
> {
> }
>
> +static inline void
> +mem_cgroup_update_statistics(struct page *page, struct mm_struct *mm, int val)
> +{
> +}
> +
> #endif /* CONFIG_CGROUP_MEM_CONT */
>
> #endif /* _LINUX_MEMCONTROL_H */
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index b35bc0e..01b4af1 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -68,8 +68,8 @@ void __anon_vma_link(struct vm_area_struct *);
> */
> void page_add_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
> void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
> -void page_add_file_rmap(struct page *);
> -void page_remove_rmap(struct page *);
> +void page_add_file_rmap(struct page *, struct vm_area_struct *);
> +void page_remove_rmap(struct page *, struct vm_area_struct *);
>
> #ifdef CONFIG_DEBUG_VM
> void page_dup_rmap(struct page *page, struct vm_area_struct *vma, unsigned long address);
> diff --git a/mm/filemap_xip.c b/mm/filemap_xip.c
> index 427dfe3..e8b2b18 100644
> --- a/mm/filemap_xip.c
> +++ b/mm/filemap_xip.c
> @@ -193,7 +193,7 @@ retry:
> /* Nuke the page table entry. */
> flush_cache_page(vma, address, pte_pfn(*pte));
> pteval = ptep_clear_flush_notify(vma, address, pte);
> - page_remove_rmap(page);
> + page_remove_rmap(page, vma);
> dec_mm_counter(mm, file_rss);
> BUG_ON(pte_dirty(pteval));
> pte_unmap_unlock(pte, ptl);
> diff --git a/mm/fremap.c b/mm/fremap.c
> index b6ec85a..01ea2da 100644
> --- a/mm/fremap.c
> +++ b/mm/fremap.c
> @@ -37,7 +37,7 @@ static void zap_pte(struct mm_struct *mm, struct vm_area_struct *vma,
> if (page) {
> if (pte_dirty(pte))
> set_page_dirty(page);
> - page_remove_rmap(page);
> + page_remove_rmap(page, vma);
> page_cache_release(page);
> update_hiwater_rss(mm);
> dec_mm_counter(mm, file_rss);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e44fb0f..d903b61 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -62,7 +62,8 @@ enum mem_cgroup_stat_index {
> * For MEM_CONTAINER_TYPE_ALL, usage = pagecache + rss.
> */
> MEM_CGROUP_STAT_CACHE, /* # of pages charged as cache */
> - MEM_CGROUP_STAT_RSS, /* # of pages charged as rss */
> + MEM_CGROUP_STAT_RSS, /* # of pages charged as anon rss */
> + MEM_CGROUP_STAT_FILE_RSS, /* # of pages charged as file rss */
I don't like the word "FILE RSS". MAPPED FILE PAGES or some will be more straigtforward.
(IIUC, meminfo shows this information as MAPPED_FILE)
> MEM_CGROUP_STAT_PGPGIN_COUNT, /* # of pages paged in */
> MEM_CGROUP_STAT_PGPGOUT_COUNT, /* # of pages paged out */
>
> @@ -321,6 +322,33 @@ static bool mem_cgroup_is_obsolete(struct mem_cgroup *mem)
> return css_is_removed(&mem->css);
> }
>
> +/*
> + * Currently used to update file_rss statistics, but the routine can be
> + * generalized to update other statistics as well.
> + */
> +void
> +mem_cgroup_update_statistics(struct page *page, struct mm_struct *mm, int val)
> +{
Hmm. "mem_cgroup_update_statistics()" have wider meaning than what it does..
Could you
- modify function name for showing "this function counts MAPPED_FILE"
or
- add a function argument as "Which counter should be modified."
> + struct mem_cgroup *mem;
> + struct mem_cgroup_stat *stat;
> + struct mem_cgroup_stat_cpu *cpustat;
> + 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_FILE_RSS, val);
> +}
>
> /*
> * Call callback function against all cgroup under hierarchy tree.
> @@ -2051,6 +2079,7 @@ static int mem_cgroup_reset(struct cgroup *cont, unsigned int event)
> enum {
> MCS_CACHE,
> MCS_RSS,
> + MCS_FILE_RSS,
> MCS_PGPGIN,
> MCS_PGPGOUT,
> MCS_INACTIVE_ANON,
> @@ -2071,6 +2100,7 @@ struct {
> } memcg_stat_strings[NR_MCS_STAT] = {
> {"cache", "total_cache"},
> {"rss", "total_rss"},
> + {"file_rss", "total_file_rss"},
> {"pgpgin", "total_pgpgin"},
> {"pgpgout", "total_pgpgout"},
> {"inactive_anon", "total_inactive_anon"},
> @@ -2091,6 +2121,8 @@ static int mem_cgroup_get_local_stat(struct mem_cgroup *mem, void *data)
> s->stat[MCS_CACHE] += val * PAGE_SIZE;
> val = mem_cgroup_read_stat(&mem->stat, MEM_CGROUP_STAT_RSS);
> s->stat[MCS_RSS] += val * PAGE_SIZE;
> + val = mem_cgroup_read_stat(&mem->stat, MEM_CGROUP_STAT_FILE_RSS);
> + s->stat[MCS_FILE_RSS] += val * PAGE_SIZE;
> val = mem_cgroup_read_stat(&mem->stat, MEM_CGROUP_STAT_PGPGIN_COUNT);
> s->stat[MCS_PGPGIN] += val;
> val = mem_cgroup_read_stat(&mem->stat, MEM_CGROUP_STAT_PGPGOUT_COUNT);
IIUC, you have to add hook to force_empty() and move FILE_RSS counter to the parent.
I hope there is no terrible race around force empty...
Thanks,
-Kame
> diff --git a/mm/memory.c b/mm/memory.c
> index a715b19..95a9ded 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -822,7 +822,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
> mark_page_accessed(page);
> file_rss--;
> }
> - page_remove_rmap(page);
> + page_remove_rmap(page, vma);
> if (unlikely(page_mapcount(page) < 0))
> print_bad_pte(vma, addr, ptent, page);
> tlb_remove_page(tlb, page);
> @@ -1421,7 +1421,7 @@ static int insert_page(struct vm_area_struct *vma, unsigned long addr,
> /* Ok, finally just insert the thing.. */
> get_page(page);
> inc_mm_counter(mm, file_rss);
> - page_add_file_rmap(page);
> + page_add_file_rmap(page, vma);
> set_pte_at(mm, addr, pte, mk_pte(page, prot));
>
> retval = 0;
> @@ -2080,7 +2080,7 @@ gotten:
> * mapcount is visible. So transitively, TLBs to
> * old page will be flushed before it can be reused.
> */
> - page_remove_rmap(old_page);
> + page_remove_rmap(old_page, vma);
> }
>
> /* Free the old page.. */
> @@ -2718,7 +2718,7 @@ static int __do_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> page_add_new_anon_rmap(page, vma, address);
> } else {
> inc_mm_counter(mm, file_rss);
> - page_add_file_rmap(page);
> + page_add_file_rmap(page, vma);
> if (flags & FAULT_FLAG_WRITE) {
> dirty_page = page;
> get_page(dirty_page);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 068655d..098d365 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -131,7 +131,7 @@ static void remove_migration_pte(struct vm_area_struct *vma,
> if (PageAnon(new))
> page_add_anon_rmap(new, vma, addr);
> else
> - page_add_file_rmap(new);
> + page_add_file_rmap(new, vma);
>
> /* No need to invalidate - it was non-present before */
> update_mmu_cache(vma, addr, pte);
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 1652166..3ece45e 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -686,10 +686,12 @@ void page_add_new_anon_rmap(struct page *page,
> *
> * The caller needs to hold the pte lock.
> */
> -void page_add_file_rmap(struct page *page)
> +void page_add_file_rmap(struct page *page, struct vm_area_struct *vma)
> {
> - if (atomic_inc_and_test(&page->_mapcount))
> + if (atomic_inc_and_test(&page->_mapcount)) {
> __inc_zone_page_state(page, NR_FILE_MAPPED);
> + mem_cgroup_update_statistics(page, vma->vm_mm, 1);
> + }
> }
>
> #ifdef CONFIG_DEBUG_VM
> @@ -719,7 +721,7 @@ void page_dup_rmap(struct page *page, struct vm_area_struct *vma, unsigned long
> *
> * The caller needs to hold the pte lock.
> */
> -void page_remove_rmap(struct page *page)
> +void page_remove_rmap(struct page *page, struct vm_area_struct *vma)
> {
> if (atomic_add_negative(-1, &page->_mapcount)) {
> /*
> @@ -738,6 +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_statistics(page, vma->vm_mm, -1);
> /*
> * It would be tidy to reset the PageAnon mapping here,
> * but that might overwrite a racing page_add_anon_rmap
> @@ -835,7 +838,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> dec_mm_counter(mm, file_rss);
>
>
> - page_remove_rmap(page);
> + page_remove_rmap(page, vma);
> page_cache_release(page);
>
> out_unmap:
> @@ -950,7 +953,7 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
> if (pte_dirty(pteval))
> set_page_dirty(page);
>
> - page_remove_rmap(page);
> + page_remove_rmap(page, vma);
> page_cache_release(page);
> dec_mm_counter(mm, file_rss);
> (*mapcount)--;
>
> --
> Balbir
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists