Re: [RFC][BUGFIX] memcg: fix memsw uncharged twice in do_swap_page

[Bob Liu <lliubbo@gmail.com>]

On Tue, Dec 13, 2011 at 4:10 PM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> Hi,
>
> On Tue, 13 Dec 2011 15:55:25 +0800
> Bob Liu <lliubbo@gmail.com> wrote:
>
>> As the document memcg_test.txt said:
>> In do_swap_page(), following events occur when pte is unchanged.
>>       (1) the page (SwapCache) is looked up.
>>       (2) lock_page()
>>       (3) try_charge_swapin()
>>       (4) reuse_swap_page() (may call delete_swap_cache())
>>       (5) commit_charge_swapin()
>>       (6) swap_free().
>>
>> And below situation:
>> (C) The page has been charged before (2) and reuse_swap_page() doesn't
>>       call delete_from_swap_cache().
>>
>> In this case, __mem_cgroup_commit_charge_swapin() may uncharge memsw twice.
>> See below two uncharge place:
>>
>> __mem_cgroup_commit_charge_swapin {
>>       => __mem_cgroup_commit_charge_lrucare
>>               => __mem_cgroup_commit_charge()    <== PageCgroupUsed
>>                       => __mem_cgroup_cancel_charge()
>>                                               <== 1.uncharge memsw here
>>
>>       if (do_swap_account && PageSwapCache(page)) {
>>               if (swap_memcg) {
> IIRC, if the page(swapcache) has been already charged as memory, swap_cgroup_record(ent, 0)
> returns 0, so swap_memcg is NULL.
>

Got it, sorry for my noise.
Thank you for all.

> Thanks,
> Daisuke Nishimura.
>
>>                       if (!mem_cgroup_is_root(swap_memcg))
>>                               res_counter_uncharge(&swap_memcg->memsw,
>>                                               PAGE_SIZE);
>>                                               <== 2.uncharged memsw again here
>>
>>                       mem_cgroup_swap_statistics(swap_memcg, false);
>>                       mem_cgroup_put(swap_memcg);
>>               }
>>       }
>> }
>>
>> This patch added a return val for __mem_cgroup_commit_charge(), if canceled then
>> don't uncharge memsw again.
>>
>> But i didn't find a definite testcase can confirm this situaction current.
>> Maybe i missed something. Welcome point.
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/memcontrol.c |   56 +++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 32 insertions(+), 24 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index bc396e7..6ead0cd 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2416,7 +2416,10 @@ struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page)
>>       return memcg;
>>  }
>>
>> -static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>> +/*
>> + * return -1 if cancel charge else return 0
>> + */
>> +static int __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>>                                      struct page *page,
>>                                      unsigned int nr_pages,
>>                                      struct page_cgroup *pc,
>> @@ -2426,7 +2429,7 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>>       if (unlikely(PageCgroupUsed(pc))) {
>>               unlock_page_cgroup(pc);
>>               __mem_cgroup_cancel_charge(memcg, nr_pages);
>> -             return;
>> +             return -1;
>>       }
>>       /*
>>        * we don't need page_cgroup_lock about tail pages, becase they are not
>> @@ -2463,6 +2466,7 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>>        * if they exceeds softlimit.
>>        */
>>       memcg_check_events(memcg, page);
>> +     return 0;
>>  }
>>
>>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> @@ -2690,20 +2694,21 @@ static void
>>  __mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr,
>>                                       enum charge_type ctype);
>>
>> -static void
>> +static int
>>  __mem_cgroup_commit_charge_lrucare(struct page *page, struct mem_cgroup *memcg,
>>                                       enum charge_type ctype)
>>  {
>>       struct page_cgroup *pc = lookup_page_cgroup(page);
>> +     int ret;
>>       /*
>>        * In some case, SwapCache, FUSE(splice_buf->radixtree), the page
>>        * is already on LRU. It means the page may on some other page_cgroup's
>>        * LRU. Take care of it.
>>        */
>>       mem_cgroup_lru_del_before_commit(page);
>> -     __mem_cgroup_commit_charge(memcg, page, 1, pc, ctype);
>> +     ret = __mem_cgroup_commit_charge(memcg, page, 1, pc, ctype);
>>       mem_cgroup_lru_add_after_commit(page);
>> -     return;
>> +     return ret;
>>  }
>>
>>  int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
>> @@ -2792,13 +2797,14 @@ static void
>>  __mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *memcg,
>>                                       enum charge_type ctype)
>>  {
>> +     int ret;
>>       if (mem_cgroup_disabled())
>>               return;
>>       if (!memcg)
>>               return;
>>       cgroup_exclude_rmdir(&memcg->css);
>>
>> -     __mem_cgroup_commit_charge_lrucare(page, memcg, ctype);
>> +     ret = __mem_cgroup_commit_charge_lrucare(page, memcg, ctype);
>>       /*
>>        * Now swap is on-memory. This means this page may be
>>        * counted both as mem and swap....double count.
>> @@ -2807,25 +2813,27 @@ __mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *memcg,
>>        * may call delete_from_swap_cache() before reach here.
>>        */
>>       if (do_swap_account && PageSwapCache(page)) {
>> -             swp_entry_t ent = {.val = page_private(page)};
>> -             struct mem_cgroup *swap_memcg;
>> -             unsigned short id;
>> +             if(!ret) {
>> +                     swp_entry_t ent = {.val = page_private(page)};
>> +                     struct mem_cgroup *swap_memcg;
>> +                     unsigned short id;
>>
>> -             id = swap_cgroup_record(ent, 0);
>> -             rcu_read_lock();
>> -             swap_memcg = mem_cgroup_lookup(id);
>> -             if (swap_memcg) {
>> -                     /*
>> -                      * This recorded memcg can be obsolete one. So, avoid
>> -                      * calling css_tryget
>> -                      */
>> -                     if (!mem_cgroup_is_root(swap_memcg))
>> -                             res_counter_uncharge(&swap_memcg->memsw,
>> -                                                  PAGE_SIZE);
>> -                     mem_cgroup_swap_statistics(swap_memcg, false);
>> -                     mem_cgroup_put(swap_memcg);
>> +                     id = swap_cgroup_record(ent, 0);
>> +                     rcu_read_lock();
>> +                     swap_memcg = mem_cgroup_lookup(id);
>> +                     if (swap_memcg) {
>> +                             /*
>> +                              * This recorded memcg can be obsolete one. So, avoid
>> +                              * calling css_tryget
>> +                              */
>> +                             if (!mem_cgroup_is_root(swap_memcg))
>> +                                     res_counter_uncharge(&swap_memcg->memsw,
>> +                                                     PAGE_SIZE);
>> +                             mem_cgroup_swap_statistics(swap_memcg, false);
>> +                             mem_cgroup_put(swap_memcg);
>> +                     }
>> +                     rcu_read_unlock();
>>               }
>> -             rcu_read_unlock();
>>       }
>>       /*
>>        * At swapin, we may charge account against cgroup which has no tasks.
>> --
>> 1.7.0.4
>>
>>

-- 
Regards,
--Bob