lkml.org 
[lkml]   [2009]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] fix swap cache account leak at swapin-readahead
    On Tue, May 12, 2009 at 10:46:03AM +0900, KAMEZAWA Hiroyuki wrote:
    > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    >
    > In general, Linux's swp_entry handling is done by combination of lazy techniques
    > and global LRU. It works well but when we use mem+swap controller, some more
    > strict control is appropriate. Otherwise, swp_entry used by a cgroup will be
    > never freed until global LRU works. In a system where memcg is well-configured,
    > global LRU doesn't work frequently.
    >
    > Example) Assume swapin-readahead.
    > CPU0 CPU1
    > zap_pte() read_swap_cache_async()
    > swap_duplicate().
    > swap_entry_free() = 1
    > find_get_page()=> NULL.
    > add_to_swap_cache().
    > issue swap I/O.
    >
    > There are many patterns of this kind of race (but no problems).
    >
    > free_swap_and_cache() is called for freeing swp_entry. But it is a best-effort
    > function. If the swp_entry/page seems busy, swp_entry is not freed.
    > This is not a problem because global-LRU will find SwapCache at page reclaim.
    >
    > If memcg is used, on the other hand, global LRU may not work. Then, above
    > unused SwapCache will not be freed.
    > (unmapped SwapCache occupy swp_entry but never be freed if not on memcg's LRU)
    >
    > So, even if there are no tasks in a cgroup, swp_entry usage still remains.
    > In bad case, OOM by mem+swap controller is triggered by this "leak" of
    > swp_entry as Nishimura reported.
    >
    > Considering this issue, swapin-readahead itself is not very good for memcg.
    > It read swap cache which will not be used. (and _unused_ swapcache will
    > not be accounted.) Even if we account swap cache at add_to_swap_cache(),
    > we need to account page to several _unrelated_ memcg. This is bad.
    >
    > This patch tries to fix racy case of free_swap_and_cache() and page status.
    >
    > After this patch applied, following test works well.
    >
    > # echo 1-2M > ../memory.limit_in_bytes
    > # run tasks under memcg.
    > # kill all tasks and make memory.tasks empty
    > # check memory.memsw.usage_in_bytes == memory.usage_in_bytes and
    > there is no _used_ swp_entry.
    >
    > What this patch does is
    > - avoid swapin-readahead when memcg is activated.
    >
    > Changelog: v6 -> v7
    > - just handle races in readahead.
    > - races in writeback is handled in the next patch.
    >
    > Changelog: v5 -> v6
    > - works only when memcg is activated.
    > - check after I/O works only after writeback.
    > - avoid swapin-readahead when memcg is activated.
    > - fixed page refcnt issue.
    > Changelog: v4->v5
    > - completely new design.
    >
    > Reported-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
    > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > ---
    > mm/swap_state.c | 20 +++++++++++++++++---
    > 1 file changed, 17 insertions(+), 3 deletions(-)
    >
    > Index: mmotm-2.6.30-May07/mm/swap_state.c
    > ===================================================================
    > --- mmotm-2.6.30-May07.orig/mm/swap_state.c
    > +++ mmotm-2.6.30-May07/mm/swap_state.c
    > @@ -349,9 +349,9 @@ struct page *read_swap_cache_async(swp_e
    > struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
    > struct vm_area_struct *vma, unsigned long addr)
    > {
    > - int nr_pages;
    > + int nr_pages = 1;
    > struct page *page;
    > - unsigned long offset;
    > + unsigned long offset = 0;
    > unsigned long end_offset;
    >
    > /*
    > @@ -360,8 +360,22 @@ struct page *swapin_readahead(swp_entry_
    > * No, it's very unlikely that swap layout would follow vma layout,
    > * more likely that neighbouring swap pages came from the same node:
    > * so use the same "addr" to choose the same node for each swap read.
    > + *
    > + * But, when memcg is used, swapin readahead give us some bad
    > + * effects. There are 2 big problems in general.
    > + * 1. Swapin readahead tend to use/read _not required_ memory.
    > + * And _not required_ memory is only freed by global LRU.
    > + * 2. We can't charge pages for swap-cache readahead because
    > + * we should avoid account memory in a cgroup which a
    > + * thread call this function is not related to.
    > + * And swapin-readahead have racy condition with
    > + * free_swap_and_cache(). This also annoys memcg.
    > + * Then, if memcg is really used, we avoid readahead.
    > */
    > - nr_pages = valid_swaphandles(entry, &offset);
    > +
    > + if (!mem_cgroup_activated())
    > + nr_pages = valid_swaphandles(entry, &offset);
    > +
    > for (end_offset = offset + nr_pages; offset < end_offset; offset++) {
    > /* Ok, do the async read-ahead now */
    > page = read_swap_cache_async(swp_entry(swp_type(entry), offset),

    Having nr_pages set to 1 and offset to zero will actually enter hat
    loop and try to read a swap slot at offset zero, including a
    superfluous page allocation, just to fail at the swap_duplicate()
    (swap slot 0 is swap header -> SWAP_MAP_BAD).

    How about:

    if (mem_cgroup_activated())
    goto pivot;
    nr_pages = valid_swaphandles(...);
    for (readahead loop)
    ...
    pivot:
    return read_swap_cache_async();

    That will also save you the runtime initialization of nr_pages and
    offset completely when the cgroup is active. And you'll have only one
    branch and no second one for offset < end_offset in the loop. And the
    lru draining, but I'm not sure about that. I think it's not needed.

    Hannes


    \
     
     \ /
      Last update: 2009-05-12 13:29    [W:0.050 / U:121.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site