Re: [PATCH V3 6/8] memcg: Don't account root_mem_cgroup page statistics

[Sha Zhengju]

On Wed, Jan 2, 2013 at 8:27 PM, Michal Hocko <mhocko@xxxxxxx> wrote:
> On Wed 26-12-12 01:27:27, Sha Zhengju wrote:
>> From: Sha Zhengju <handai.szj@xxxxxxxxxx>
>>
>> If memcg is enabled and no non-root memcg exists, all allocated pages
>> belongs to root_mem_cgroup and go through root memcg statistics routines
>> which brings some overheads. So for the sake of performance, we can give
>> up accounting stats of root memcg for MEM_CGROUP_STAT_FILE_MAPPED/FILE_DIRTY
>> /WRITEBACK
>
> I do not like this selective approach. We should handle all the stat
> types in the same way. SWAP is not a hot path but RSS and CACHE should
> be optimized as well. It seems that thresholds events might be a
> complication here but it shouldn't be that a big deal (mem_cgroup_usage
> would need some treat).
>

The three MEM_CGROUP_STAT_FILE_{MAPPED, DIRTY, WRITEBACK} stat
are accounted in the similar way: all of them are embedded in original global
accounting routine and have mem_cgroup_update_page_stat() as the unified
accounting entry. But RSS/CACHE/SWAP are counted in memcg charging periods
and may extend the patch larger... so I just want to send this out to
collect opinion.
Now from the feedback, I think I can continue the remaining in next round.


>> and instead we pay special attention while showing root
>> memcg numbers in memcg_stat_show(): as we don't account root memcg stats
>> anymore, the root_mem_cgroup->stat numbers are actually 0.
>
> Yes, this is reasonable.
>
>> But because of hierachy, figures of root_mem_cgroup may just represent
>> numbers of pages used by its own tasks(not belonging to any other
>> child cgroup).
>
> I am not sure what the above means. root might have use_hierarchy set to
> 1 as well.
>

Yes, my fault for not clear here. What I mean is we can not simply set numbers
of root memcg to global state. Whether root.use_hierarchy is 1 or 0, the figures
of root memcg are behalf of its local page stat (the difference exists
in total_* stat).
So we can fake these root numbers out by using stats of global state and
all other memcg.

>> So here we fake these root numbers by using stats of global state and
>> all other memcg.  That is for root memcg:
>>       nr(MEM_CGROUP_STAT_FILE_MAPPED) = global_page_state(NR_FILE_MAPPED) -
>>                               sum_of_all_memcg(MEM_CGROUP_STAT_FILE_MAPPED);
>> Dirty/Writeback pages accounting are in the similar way.
>>
>> Signed-off-by: Sha Zhengju <handai.szj@xxxxxxxxxx>
>
> I like the approach but I do not like the implementation. See details
> bellow.
>
>> ---
>>  mm/memcontrol.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 68 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index fc20ac9..728349d 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
> [...]
>> @@ -5396,18 +5406,70 @@ static inline void mem_cgroup_lru_names_not_uptodate(void)
>>       BUILD_BUG_ON(ARRAY_SIZE(mem_cgroup_lru_names) != NR_LRU_LISTS);
>>  }
>>
>> +long long root_memcg_local_stat(unsigned int i, long long val,
>> +                                     long long nstat[])
>
> Function should be static
> also
> nstat parameter is ugly because this can be done by the caller
> and also expecting that the caller already calculated val is not
> nice (and undocumented). This approach is really hackish and error
> prone. Why should we define a specific function rather than hooking into
> mem_cgroup_read_stat and doing all the stuff there? I think that would
> be much more maintainable.
>

IMHO, hooking into mem_cgroup_read_stat may be also improper because
of the for_each_mem_cgroup traversal. I prefer to make mem_cgroup_read_stat
as the base func unit. But I'll repeal the function base on your opinion in next
version.  Thanks for the advice!

>> +{
>> +     long long res = 0;
>> +
>> +     switch (i) {
>> +     case MEM_CGROUP_STAT_FILE_MAPPED:
>> +             res = global_page_state(NR_FILE_MAPPED);
>> +             break;
>> +     case MEM_CGROUP_STAT_FILE_DIRTY:
>> +             res = global_page_state(NR_FILE_DIRTY);
>> +             break;
>> +     case MEM_CGROUP_STAT_WRITEBACK:
>> +             res = global_page_state(NR_WRITEBACK);
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     res = (res <= val) ? 0 : (res - val) * PAGE_SIZE;
>> +     nstat[i] = res;
>> +
>> +     return res;
>> +}
>> +
>>  static int memcg_stat_show(struct cgroup *cont, struct cftype *cft,
>>                                struct seq_file *m)
>>  {
>>       struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
>>       struct mem_cgroup *mi;
>>       unsigned int i;
>> +     long long nstat[MEM_CGROUP_STAT_NSTATS] = {0};
>
> s/nstat/root_stat/
>
>>
>>       for (i = 0; i < MEM_CGROUP_STAT_NSTATS; i++) {
>> +             long long val = 0, res = 0;
>> +
>>               if (i == MEM_CGROUP_STAT_SWAP && !do_swap_account)
>>                       continue;
>> -             seq_printf(m, "%s %ld\n", mem_cgroup_stat_names[i],
>> -                        mem_cgroup_read_stat(memcg, i) * PAGE_SIZE);
>> +             if (i == MEM_CGROUP_STAT_SWAP || i == MEM_CGROUP_STAT_CACHE ||
>> +                     i == MEM_CGROUP_STAT_RSS) {
>
> This is plain ugly. If nothing else it asks for a comment why those are
> special.

Okay, more comments will be added.


Thanks,
Sha

>
>> +                     seq_printf(m, "%s %ld\n", mem_cgroup_stat_names[i],
>> +                                mem_cgroup_read_stat(memcg, i) * PAGE_SIZE);
>> +                     continue;
>> +             }
>> +
>> +             /* As we don't account root memcg stats anymore, the
>> +              * root_mem_cgroup->stat numbers are actually 0. But because of
>> +              * hierachy, figures of root_mem_cgroup may just represent
>> +              * numbers of pages used by its own tasks(not belonging to any
>> +              * other child cgroup). So here we fake these root numbers by
>> +              * using stats of global state and all other memcg. That is for
>> +              * root memcg:
>> +              * nr(MEM_CGROUP_STAT_FILE_MAPPED) = global_page_state(NR_FILE_
>> +              *      MAPPED) - sum_of_all_memcg(MEM_CGROUP_STAT_FILE_MAPPED)
>> +              * Dirty/Writeback pages accounting are in the similar way.
>> +              */
>> +             if (memcg == root_mem_cgroup) {
>> +                     for_each_mem_cgroup(mi)
>> +                             val += mem_cgroup_read_stat(mi, i);
>> +                     res = root_memcg_local_stat(i, val, nstat);
>> +             } else
>> +                     res = mem_cgroup_read_stat(memcg, i) * PAGE_SIZE;
>> +
>> +             seq_printf(m, "%s %lld\n", mem_cgroup_stat_names[i], res);
>>       }
>>
>>       for (i = 0; i < MEM_CGROUP_EVENTS_NSTATS; i++)
>> @@ -5435,6 +5497,10 @@ static int memcg_stat_show(struct cgroup *cont, struct cftype *cft,
>>                       continue;
>>               for_each_mem_cgroup_tree(mi, memcg)
>>                       val += mem_cgroup_read_stat(mi, i) * PAGE_SIZE;
>> +
>> +             /* Adding local stats of root memcg */
>> +             if (memcg == root_mem_cgroup)
>> +                     val += nstat[i];
>>               seq_printf(m, "total_%s %lld\n", mem_cgroup_stat_names[i], val);
>>       }
>>
>> --
>> 1.7.9.5
>>
>
> --
> Michal Hocko
> SUSE Labs
--
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/