Re: [RFC][PATCH 4/8] RSS controller accounting

Pavel Emelianov
2006-11-10T09:06:52Z

Balbir Singh wrote:
> Account RSS usage of a task and the associated container. The definition
> of RSS was debated and discussed in the following thread
>
> http://lkml.org/lkml/2006/10/10/130
>
>
> The code tracks all resident pages (including shared pages) as RSS. This patch
> can easily adapt to the definition of RSS that will be agreed upon. This
> implementation provides a proof of concept RSS controller.
>
> The accounting is inspired from Rohit Seth's container patches.
>
> TODO's
>
> 1. Merge file_rss and anon_rss tracking with the current rss tracking to
> maximize code reuse
> 2. Add/remove RSS tracking as the definition of RSS evolves
>
>
> Signed-off-by: Balbir Singh <balbir@in.ibm.com>
> ---
>

[snip]

> --- linux-2.6.19-rc2/kernel/res_group/memctlr.c~container-memctlr-acct 2006-11-09 21:46:22.000000000 +0530
> +++ linux-2.6.19-rc2-balbir/kernel/res_group/memctlr.c 2006-11-09 21:47:06.000000000 +0530
> @@ -37,6 +37,8 @@ static struct resource_group *root_rgrou
> static const char version[] = "0.01";
> static struct memctlr *memctlr_root;
>
> +#define MEMCTLR_MAGIC 0xdededede
> +
> struct mem_counter {
> atomic_long_t rss;
> };
> @@ -49,6 +51,7 @@ struct memctlr {
> /* Statistics */
> int successes;
> int failures;
> + int magic;

What is this magic for? Is it just for debugging?

[snip]

> +static inline struct memctlr *get_memctlr_from_page(struct page *page)
> +{
> + struct resource_group *rgroup;
> + struct memctlr *res;
> +
> + /*
> + * Is the resource groups infrastructure initialized?
> + */
> + if (!memctlr_root)
> + return NULL;
> +
> + rcu_read_lock();
> + rgroup = (struct resource_group *)rcu_dereference(current->container);
> + rcu_read_unlock();
> +
> + res = get_memctlr(rgroup);
> + if (!res)
> + return NULL;
> +
> + BUG_ON(res->magic != MEMCTLR_MAGIC);
> + return res;
> +}

I don't see how page passed to this function is involved into
'struct memctlr *res' determining. Could you comment this?

[snip]

> --- linux-2.6.19-rc2/mm/rmap.c~container-memctlr-acct 2006-11-09 21:46:22.000000000 +0530
> +++ linux-2.6.19-rc2-balbir/mm/rmap.c 2006-11-09 21:46:22.000000000 +0530
> @@ -537,6 +537,7 @@ void page_add_anon_rmap(struct page *pag
> if (atomic_inc_and_test(&page->_mapcount))
> __page_set_anon_rmap(page, vma, address);
> /* else checking page index and mapping is racy */
> + memctlr_inc_rss(page);
> }
>
> /*
> @@ -553,6 +554,7 @@ void page_add_new_anon_rmap(struct page
> {
> atomic_set(&page->_mapcount, 0); /* elevate count by 1 (starts at -1) */
> __page_set_anon_rmap(page, vma, address);
> + memctlr_inc_rss(page);
> }
>
> /**
> @@ -565,6 +567,7 @@ void page_add_file_rmap(struct page *pag
> {
> if (atomic_inc_and_test(&page->_mapcount))
> __inc_zone_page_state(page, NR_FILE_MAPPED);
> + memctlr_inc_rss(page);

Consider a task maps one file page 100 times in different places
and touches 'all of them'. In this case I see that you'll get
100 in rss counter while real rss will be just 1.

> }
>
> /**
> @@ -596,8 +599,9 @@ void page_remove_rmap(struct page *page)
> if (page_test_and_clear_dirty(page))
> set_page_dirty(page);
> __dec_zone_page_state(page,
> - PageAnon(page) ? NR_ANON_PAGES : NR_FILE_MAPPED);
> + PageAnon(page) ? NR_ANON_PAGES : NR_FILE_MAPPED);

What is this extra space after a question-mark for?

> }
> + memctlr_dec_rss(page, mm);
> }
>
> /*
> diff -puN include/linux/rmap.h~container-memctlr-acct include/linux/rmap.h
> --- linux-2.6.19-rc2/include/linux/rmap.h~container-memctlr-acct 2006-11-09 21:46:22.000000000 +0530
> +++ linux-2.6.19-rc2-balbir/include/linux/rmap.h 2006-11-09 21:46:22.000000000 +0530
> @@ -8,6 +8,7 @@
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/spinlock.h>
> +#include <linux/memctlr.h>
>
> /*
> * The anon_vma heads a list of private "related" vmas, to scan if
> @@ -84,6 +85,7 @@ void page_remove_rmap(struct page *);
> static inline void page_dup_rmap(struct page *page)
> {
> atomic_inc(&page->_mapcount);
> + memctlr_inc_rss(page);
> }

I'm not sure this is correct. page_dup_rmap() happens in the context
of forking process and thus you'll increment rss counter on current.
But this must be incremented at new task's counter, mustn't it?