Re: memcg: add mlock statistic in memory.stat

[Ying Han <yinghan@google.com>]

On Wed, Jan 11, 2012 at 3:59 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Wed, 11 Jan 2012 15:17:42 -0800 (PST)
> Hugh Dickins <hughd@google.com> wrote:
>
>> On Wed, 11 Jan 2012, Ying Han wrote:
>>
>> > We have the nr_mlock stat both in meminfo as well as vmstat system wide, this
>> > patch adds the mlock field into per-memcg memory stat. The stat itself enhances
>> > the metrics exported by memcg, especially is used together with "uneivctable"
>> > lru stat.
>> >
>> > --- a/include/linux/page_cgroup.h
>> > +++ b/include/linux/page_cgroup.h
>> > @@ -10,6 +10,7 @@ enum {
>> >     /* flags for mem_cgroup and file and I/O status */
>> >     PCG_MOVE_LOCK, /* For race between move_account v.s. following bits */
>> >     PCG_FILE_MAPPED, /* page is accounted as "mapped" */
>> > +   PCG_MLOCK, /* page is accounted as "mlock" */
>> >     /* No lock in page_cgroup */
>> >     PCG_ACCT_LRU, /* page has been accounted for (under lru_lock) */
>> >     __NR_PCG_FLAGS,
>>
>> Is this really necessary?  KAMEZAWA-san is engaged in trying to reduce
>> the number of PageCgroup flags, and I expect that in due course we shall
>> want to merge them in with Page flags, so adding more is unwelcome.
>> I'd  have thought that with memcg_ hooks in the right places,
>> a separate flag would not be necessary?
>>
>
> Please don't ;)
>
> NR_UNEIVCTABLE_LRU is not enough ?

Seems not.

The unevictable lru includes more than mlock()'d pages ( SHM_LOCK'd
etc). There are use cases where we like to know the mlock-ed size
per-cgroup. We used to archived that in fake-numa based container by
reading the value from per-node meminfo, however we miss that
information in memcg. What do you think?

Thank you Hugh and Kame for the reference. Apparently I missed that
patch and I will take a look at it. (still catching up emails after
vacation).

--Ying

>
> Following is the patch I posted before to remove PCG_FILE_MAPPED.
> Then, I think you can use similar logic and make use of UNEVICTABLE flags.
>
> ==
> better (lockless) idea is welcomed.
>
> From fd2b5822838eebbacc41f343f9eb8c6f0ad8e1cc Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 15 Dec 2011 11:42:49 +0900
> Subject: [PATCH 2/5] memcg: safer page stat updating
>
> Now, page stat accounting is done like this.
>
>     if (....set flag or some)
>            update vmstat
>            update memcg'stat
>
> Unlike vmstat, memcg must take care of changes in pc->mem_cgroup.
> This is done by page_cgroup_move_lock and other flags per stats.
>
> I think FileMapped works well. But, considering update of other
> statistics, current logic doesn't works well. Assume following case,
>
>     set flag
>     ..(delay by some preemption)..
>                                            clear flag
>                                            pc's flag is unset and
>                                            don't update anything.
>     memcg = pc->mem_cgroup
>     set flag to pc->mem_cgroup
>     update memcg stat
>
> In this case, the stat will be leaked out. I think memcg's account
> routine should see no flags. To avoid using memcg's original flags,
> we need to prevent overwriting pc->mem_cgroup while we updating
> the memcg.
>
> This patch adds
>   - mem_cgroup_begin_update_page_stats(),
>   - mem_cgroup_end_update_page_stats()
>
> And guarantees pc->mem_cgroup is not overwritten while updating.
> The caller should do
>
>     mem_cgroup_begin_update_page_stats()
>     if (.... set flag or some)
>            update vmstat
>            update memcg's stat
>     mem_cgroup_end_update_page_stats().
>
> This beign...end will check a counter (which is 0 in most case) under
> rcu_read_lock/rcu_read_unlock. And take a spinlock if required.
>
> Following patch in this series will remove PCG_FILE_MAPPED flag.
> ---
>  include/linux/memcontrol.h |   49 +++++++++++++++++++++++++++++++++++++++++-
>  mm/memcontrol.c            |   50 +++++++++++++++++++++++++++++--------------
>  mm/rmap.c                  |    5 ++++
>  3 files changed, 86 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 598b3c9..4a61c4b 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -141,9 +141,52 @@ static inline bool mem_cgroup_disabled(void)
>        return false;
>  }
>
> -void mem_cgroup_update_page_stat(struct page *page,
> +/*
> + * Unlike vmstat, page's mem_cgroup can be overwritten and for which memcg
> + * the page stats should be accounted to is determined dynamically.
> + * Unfortunately, there are many races. To avoid races, the caller should do
> + *
> + * locked = mem_cgroup_begin_update_page_stat(page)
> + * if (set page flags etc)
> + *     mem_cgroup_update_page_stat(page);
> + * mem_cgroup_end_update_page_stat(page, locked);
> + *
> + * Between [begin, end) calls, page's mem_cgroup will never be changed.
> + */
> +void __mem_cgroup_update_page_stat(struct page *page,
> +                       enum mem_cgroup_page_stat_item idx,
> +                       int val);
> +
> +static inline void mem_cgroup_update_page_stat(struct page *page,
>                                 enum mem_cgroup_page_stat_item idx,
> -                                int val);
> +                                int val)
> +{
> +       if (mem_cgroup_disabled())
> +               return;
> +       __mem_cgroup_update_page_stat(page, idx, val);
> +}
> +
> +bool __mem_cgroup_begin_update_page_stats(struct page *page,
> +               unsigned long *flags);
> +static inline bool
> +mem_cgroup_begin_update_page_stats(struct page *page, unsigned long *flags)
> +{
> +       if (mem_cgroup_disabled())
> +               return false;
> +       return __mem_cgroup_begin_update_page_stats(page, flags);
> +}
> +
> +void __mem_cgroup_end_update_page_stats(struct page *page, bool locked,
> +               unsigned long *flags);
> +
> +static inline void
> +mem_cgroup_end_update_page_stats(struct page *page,
> +               bool locked, unsigned long *flags)
> +{
> +       if (mem_cgroup_disabled())
> +               return;
> +       __mem_cgroup_end_update_page_stats(page, locked, flags);
> +}
>
>  static inline void mem_cgroup_inc_page_stat(struct page *page,
>                                            enum mem_cgroup_page_stat_item idx)
> @@ -171,6 +214,8 @@ void mem_cgroup_split_huge_fixup(struct page *head);
>  bool mem_cgroup_bad_page_check(struct page *page);
>  void mem_cgroup_print_bad_page(struct page *page);
>  #endif
> +
> +
>  #else /* CONFIG_CGROUP_MEM_RES_CTLR */
>  struct mem_cgroup;
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d183e1b..f4e6d5c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1831,27 +1831,50 @@ bool mem_cgroup_handle_oom(struct mem_cgroup *memcg, gfp_t mask)
>  * possibility of race condition. If there is, we take a lock.
>  */
>
> -void mem_cgroup_update_page_stat(struct page *page,
> -                                enum mem_cgroup_page_stat_item idx, int val)
> +/*
> + * This function calls rcu_read_lock(). This lock is unlocked by
> + * __mem_cgroup_end_update_page_stat().
> + */
> +bool __mem_cgroup_begin_update_page_stats(struct page *page, unsigned long *flags)
>  {
>        struct mem_cgroup *memcg;
>        struct page_cgroup *pc = lookup_page_cgroup(page);
>        bool need_unlock = false;
> -       unsigned long uninitialized_var(flags);
>
>        rcu_read_lock();
>        memcg = pc->mem_cgroup;
> -       if (unlikely(!memcg || !PageCgroupUsed(pc)))
> +       if (!memcg  || !PageCgroupUsed(pc))
>                goto out;
> -       /* pc->mem_cgroup is unstable ? */
>        if (unlikely(mem_cgroup_stealed(memcg)) || PageTransHuge(page)) {
> -               /* take a lock against to access pc->mem_cgroup */
> -               move_lock_page_cgroup(pc, &flags);
> +               move_lock_page_cgroup(pc, flags);
>                need_unlock = true;
> -               memcg = pc->mem_cgroup;
> -               if (!memcg || !PageCgroupUsed(pc))
> -                       goto out;
>        }
> +out:
> +       return need_unlock;
> +}
> +EXPORT_SYMBOL(__mem_cgroup_begin_update_page_stats);
> +
> +void __mem_cgroup_end_update_page_stats(struct page *page, bool locked,
> +                       unsigned long *flags)
> +{
> +       struct page_cgroup *pc;
> +
> +       if (unlikely(locked)) {
> +               pc = lookup_page_cgroup(page);
> +               move_unlock_page_cgroup(pc, flags);
> +       }
> +       rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(__mem_cgroup_end_update_page_stats);
> +
> +void __mem_cgroup_update_page_stat(struct page *page,
> +                                enum mem_cgroup_page_stat_item idx, int val)
> +{
> +       struct page_cgroup *pc = lookup_page_cgroup(page);
> +       struct mem_cgroup *memcg = pc->mem_cgroup;
> +
> +       if (!memcg || !PageCgroupUsed(pc))
> +               return;
>
>        switch (idx) {
>        case MEMCG_NR_FILE_MAPPED:
> @@ -1866,14 +1889,9 @@ void mem_cgroup_update_page_stat(struct page *page,
>        }
>
>        this_cpu_add(memcg->stat->count[idx], val);
> -
> -out:
> -       if (unlikely(need_unlock))
> -               move_unlock_page_cgroup(pc, &flags);
> -       rcu_read_unlock();
>        return;
>  }
> -EXPORT_SYMBOL(mem_cgroup_update_page_stat);
> +EXPORT_SYMBOL(__mem_cgroup_update_page_stat);
>
>  /*
>  * size of first charge trial. "32" comes from vmscan.c's magic value.
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 54d140a..3648c88 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1105,10 +1105,15 @@ void page_add_new_anon_rmap(struct page *page,
>  */
>  void page_add_file_rmap(struct page *page)
>  {
> +       unsigned long flags;
> +       bool locked;
> +
> +       locked = mem_cgroup_begin_update_page_stats(page, &flags);
>        if (atomic_inc_and_test(&page->_mapcount)) {
>                __inc_zone_page_state(page, NR_FILE_MAPPED);
>                mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED);
>        }
> +       mem_cgroup_end_update_page_stats(page, locked, &flags);
>  }
>
>  /**
> --
> 1.7.4.1
>
>

--
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>