Re: [PATCH] fix unused/stale swap cache handling on memcg v3

[Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>]

On Tue, 21 Apr 2009 11:57:49 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Tue, 21 Apr 2009 11:35:25 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> 
> > @@ -785,6 +786,23 @@ activate_locked:
> >                 SetPageActive(page);
> >                 pgactivate++;
> >  keep_locked:
> > +               if (!scanning_global_lru(sc) && PageSwapCache(page)) {
> > +                       struct page_cgroup *pc;
> > +
> > +                       pc = lookup_page_cgroup(page);
> > +                       /*
> > +                        * Used bit of swapcache is solid under page lock.
> > +                        */
> > +                       if (unlikely(!PageCgroupUsed(pc)))
> > +                               /*
> > +                                * This can happen if the page is free'ed by
> > +                                * the owner process before it is added to
> > +                                * swapcache.
> > +                                * These swapcache cannot be managed by memcg
> > +                                * well, so free it here.
> > +                                */
> > +                               try_to_free_swap(page);
> > +               }
> >                 unlock_page(page);
> >  keep:
> >                 list_add(&page->lru, &ret_pages);
> > 
> > This cannot prevent type-1 orphan SwapCache(caused by the race
> > between exit() and swap-in readahead).
> > Type-1 can pressure the memsw usage(trigger OOM if memsw.limit is set, as a result)
> > and make struct mem_cgroup unfreeable even after rmdir(because it holds refcount
> > to mem_cgroup).
> Hmm.
>    free_swap_cache()
> 	-> trylock_page() => failure case ?
> 
Yes, but there is another case:

            processA                   |           processB
  -------------------------------------+-------------------------------------
    (free_swap_and_cache())            |  (read_swap_cache_async())
                                       |    swap_duplicate()
      swap_entry_free() == 1           |
      find_get_page() -> cannot find   |
                                       |    __set_page_locked()
                                       |    add_to_swap_cache()
                                       |    lru_cache_add_anon()
                                       |      doesn't link this page to memcg's
                                       |      LRU, because of !PageCgroupUsed.


> add following codes.
> ==
>  588                         page = find_get_page(&swapper_space, entry.val);
>  589                         if (page && !trylock_page(page)) {
> 				     mem_cgroup_retry_free_swap_lazy(page);  <=====
>  590                                 page_cache_release(page);
>  591                                 page = NULL;
>  592                         }
> ==
> and  do some kind of lazy ops..I'll try some.
> 
> > 
> > Do you have any ideas to solve orphan SwapCache problem by adding some hooks to shrink_zone() ?
> > (scan some pages from global LRU and check whether it's orphan SwapCache or not by
> > adding some code like above ?)
> > 
> > And, what do you think about adding above code to shrink_page_list() ?
> > I think it might be unnecessary if we can solve the problem in another way, though.
> > 
> 
> I think your hook itself is not very bad. (even if we remove this later..)
> 
I think it depends on how we fix the type-1 whether we should remove this or not.
Anyway, I'll leave it as it is for now.


Thanks,
Daisuke Nishimura.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>