[Devel] Re: [PATCH 03/10] memcg: create extensible page stat update routines

[Minchan Kim]

On Wed, Oct 6, 2010 at 4:59 AM, Greg Thelen <gthelen at google.com> wrote:
> Minchan Kim <minchan.kim at gmail.com> writes:
>
>> On Sun, Oct 03, 2010 at 11:57:58PM -0700, Greg Thelen wrote:
>>> Replace usage of the mem_cgroup_update_file_mapped() memcg
>>> statistic update routine with two new routines:
>>> * mem_cgroup_inc_page_stat()
>>> * mem_cgroup_dec_page_stat()
>>>
>>> As before, only the file_mapped statistic is managed.  However,
>>> these more general interfaces allow for new statistics to be
>>> more easily added.  New statistics are added with memcg dirty
>>> page accounting.
>>>
>>> Signed-off-by: Greg Thelen <gthelen at google.com>
>>> Signed-off-by: Andrea Righi <arighi at develer.com>
>>> ---
>>>  include/linux/memcontrol.h |   31 ++++++++++++++++++++++++++++---
>>>  mm/memcontrol.c            |   17 ++++++++---------
>>>  mm/rmap.c                  |    4 ++--
>>>  3 files changed, 38 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index 159a076..7c7bec4 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -25,6 +25,11 @@ struct page_cgroup;
>>>  struct page;
>>>  struct mm_struct;
>>>
>>> +/* Stats that can be updated by kernel. */
>>> +enum mem_cgroup_write_page_stat_item {
>>> +    MEMCG_NR_FILE_MAPPED, /* # of pages charged as file rss */
>>> +};
>>> +
>>
>> mem_cgrou_"write"_page_stat_item?
>> Does "write" make sense to abstract page_state generally?
>
> First I will summarize the portion of the design relevant to this
> comment:
>
> This patch series introduces two sets of memcg statistics.
> a) the writable set of statistics the kernel updates when pages change
>   state (example: when a page becomes dirty) using:
>     mem_cgroup_inc_page_stat(struct page *page,
>                                enum mem_cgroup_write_page_stat_item idx)
>     mem_cgroup_dec_page_stat(struct page *page,
>                                enum mem_cgroup_write_page_stat_item idx)
>
> b) the read-only set of statistics the kernel queries to measure the
>   amount of dirty memory used by the current cgroup using:
>     s64 mem_cgroup_page_stat(enum mem_cgroup_read_page_stat_item item)
>
>   This read-only set of statistics is set of a higher level conceptual
>   counters.  For example, MEMCG_NR_DIRTYABLE_PAGES is the sum of the
>   counts of pages in various states (active + inactive).  mem_cgroup
>   exports this value as a higher level counter rather than individual
>   counters (active & inactive) to minimize the number of calls into
>   mem_cgroup_page_stat().  This avoids extra calls to cgroup tree
>   iteration with for_each_mem_cgroup_tree().
>
> Notice that each of the two sets of statistics are addressed by a
> different type, mem_cgroup_{read vs write}_page_stat_item.
>
> This particular patch (memcg: create extensible page stat update
> routines) introduces part of this design.  A later patch I emailed
> (memcg: add dirty limits to mem_cgroup) added
> mem_cgroup_read_page_stat_item.
>
>
> I think the code would read better if I renamed
> enum mem_cgroup_write_page_stat_item to
> enum mem_cgroup_update_page_stat_item.
>
> Would this address your concern

Thanks for the kind explanation.
I understand your concept.

I think you makes update and query as completely different level
abstraction but you could use similar terms.
Even the terms(write VS read) make me more confusing.

How about renaming following as?

1. mem_cgroup_write_page_stat_item -> mem_cgroup_page_stat_item
2. mem_cgroup_read_page_stat_item -> mem_cgroup_nr_pages_item

At least it looks to be easy for me to understand the code.
But it's just my preference. If others think your semantic is more
desirable, I am not against it strongly.

Thanks, Greg.

>
> --
> Greg
>



-- 
Kind regards,
Minchan Kim
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers