lkml.org 
[lkml]   [2009]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] memcg: fix stale swap cache leak v5
    On Thu, 30 Apr 2009 16:16:27 +0900
    KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

    > This is v5 but all codes are rewritten.
    >
    > After this patch, when memcg is used,
    > 1. page's swapcount is checked after I/O (without locks). If the page is
    > stale swap cache, freeing routine will be scheduled.
    > 2. vmscan.c calls try_to_free_swap() when __remove_mapping() fails.
    >
    > Works well for me. no extra resources and no races.
    >
    > Because my office will be closed until May/7, I'll not be able to make a
    > response. Posting this for showing what I think of now.
    >
    I found a hole immediately after posted this...sorry. plz ignore this patch/
    see you again in the next month.

    -Kame

    > This should be fixed before posting softlimit etc...
    > ==
    > 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 cotroll 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 A) Assume a swap cache which is not mapped.
    > CPU0 CPU1
    > zap_pte().... shrink_page_list()
    > free_swap_and_cache() lock_page()
    > page seems busy.
    >
    > Example B) Assume swapin-readahed.
    > 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.
    > But...
    >
    > 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 triggerred by this "leak" of
    > swp_entry as Nishimura repoted.
    >
    > This patch tries to fix racy case of free_swap_and_cache() and I/O by checking
    > swap's refnct again after I/O. And add a hook to vmscan.c.
    > After this patch applied, follwoing 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.
    >
    > Changelog: v4->v5
    > - completely new design.
    > - added nolock page_swapcount.
    > - checks all swap I/O.
    >
    > Reported-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
    > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > ---
    > include/linux/swap.h | 1
    > mm/page_io.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++
    > mm/swapfile.c | 37 +++++++++++++++
    > mm/vmscan.c | 31 ++++++++++++-
    > 4 files changed, 187 insertions(+), 2 deletions(-)
    >
    > Index: mmotm-2.6.30-Apr24/mm/page_io.c
    > ===================================================================
    > --- mmotm-2.6.30-Apr24.orig/mm/page_io.c
    > +++ mmotm-2.6.30-Apr24/mm/page_io.c
    > @@ -19,6 +19,123 @@
    > #include <linux/writeback.h>
    > #include <asm/pgtable.h>
    >
    > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR
    > +/*
    > + * When memory cgroup is used, race between read/write-swap and zap-swap can
    > + * be a leak of swp_entry accounted. So we have to check the status at
    > + * the end of swap-io. If memory cgroup is not used, Global LRU will find
    > + * unused swap-cache finally. (But this is too lazy for memcg.)
    > + */
    > +
    > +struct swapio_check {
    > + spinlock_t lock;
    > + void *swap_bio_list;
    > + struct delayed_work work;
    > +} stale_swap_check;
    > +
    > +
    > +static void mem_cgroup_check_stale_swap(struct work_struct *work)
    > +{
    > + struct bio *bio;
    > + struct page *page;
    > + struct swapio_check *sc;
    > + int nr = SWAP_CLUSTER_MAX;
    > + swp_entry_t entry;
    > +
    > + sc = &stale_swap_check;
    > +
    > + while (nr--) {
    > + cond_resched();
    > + spin_lock_irq(&sc->lock);
    > + bio = sc->swap_bio_list;
    > + if (bio)
    > + sc->swap_bio_list = bio->bi_next;
    > + spin_unlock_irq(&sc->lock);
    > + if (!bio)
    > + break;
    > + entry.val = (unsigned long)bio->bi_private;
    > + bio_put(bio);
    > +
    > + page = find_get_page(&swapper_space, entry.val);
    > + if (!page || page_mapped(page))
    > + continue;
    > + lock_page(page);
    > + /*
    > + * When it's mapped, this page passed checks in do_swap_page()
    > + * and we don't have to do any more. All other necessary checks
    > + * will be done in try_to_free_swap().
    > + */
    > + if (!page_mapped(page))
    > + try_to_free_swap(page);
    > + unlock_page(page);
    > + put_page(page);
    > + }
    > + if (sc->swap_bio_list)
    > + schedule_delayed_work(&sc->work, HZ/10);
    > +}
    > +
    > +/*
    > + * We can't call try_to_free_swap directly here because of caller's context.
    > + */
    > +static void mem_cgroup_swapio_check_again(struct bio *bio, struct page *page)
    > +{
    > + unsigned long flags;
    > + struct swapio_check *sc;
    > + swp_entry_t entry;
    > + int ret;
    > +
    > + /* check swap count here. If swp_entry is stable, nothing to do.*/
    > + if (likely(mem_cgroup_staleswap_hint(page)))
    > + return;
    > + /* reuse bio if this bio is ready to be freed. */
    > + ret = atomic_inc_return(&bio->bi_cnt);
    > + /* Any other reference other than us ? */
    > + if (unlikely(ret > 2)) {
    > + bio_put(bio);
    > + return;
    > + }
    > + /*
    > + * We don't want to grab this page....record swp_entry instead of page.
    > + */
    > + entry.val = page_private(page);
    > + bio->bi_private = (void *)entry.val;
    > +
    > + sc = &stale_swap_check;
    > + spin_lock_irqsave(&sc->lock, flags);
    > + /* link bio */
    > + bio->bi_next = sc->swap_bio_list;
    > + sc->swap_bio_list = bio;
    > + spin_unlock_irqrestore(&sc->lock, flags);
    > + /*
    > + * Swap I/O is tend to be countinous. Do check in batched manner.
    > + */
    > + if (!delayed_work_pending(&sc->work))
    > + schedule_delayed_work(&sc->work, HZ/10);
    > +}
    > +
    > +static int __init setup_stale_swap_check(void)
    > +{
    > + struct swapio_check *sc;
    > +
    > + sc = &stale_swap_check;
    > + spin_lock_init(&sc->lock);
    > + sc->swap_bio_list = NULL;
    > + INIT_DELAYED_WORK(&sc->work, mem_cgroup_check_stale_swap);
    > + return 0;
    > +}
    > +late_initcall(setup_stale_swap_check);
    > +
    > +
    > +#else /* CONFIG_CGROUP_MEM_RES_CTRL */
    > +
    > +static inline
    > +void mem_cgroup_swapio_check_again(struct bio *bio, struct page *page)
    > +{
    > +}
    > +#endif
    > +
    > +
    > +
    > static struct bio *get_swap_bio(gfp_t gfp_flags, pgoff_t index,
    > struct page *page, bio_end_io_t end_io)
    > {
    > @@ -66,6 +183,8 @@ static void end_swap_bio_write(struct bi
    > (unsigned long long)bio->bi_sector);
    > ClearPageReclaim(page);
    > }
    > + /* While PG_writeback, this page is stable ...then, call this here */
    > + mem_cgroup_swapio_check_again(bio, page);
    > end_page_writeback(page);
    > bio_put(bio);
    > }
    > @@ -85,6 +204,7 @@ void end_swap_bio_read(struct bio *bio,
    > } else {
    > SetPageUptodate(page);
    > }
    > + mem_cgroup_swapio_check_again(bio, page);
    > unlock_page(page);
    > bio_put(bio);
    > }
    > Index: mmotm-2.6.30-Apr24/mm/vmscan.c
    > ===================================================================
    > --- mmotm-2.6.30-Apr24.orig/mm/vmscan.c
    > +++ mmotm-2.6.30-Apr24/mm/vmscan.c
    > @@ -586,6 +586,30 @@ void putback_lru_page(struct page *page)
    > }
    > #endif /* CONFIG_UNEVICTABLE_LRU */
    >
    > +#ifdef CONFIG_CGROUP_MEM_RES_CTRL
    > +/*
    > + * Even if we don't call this, global LRU will finally find this SwapCache and
    > + * free swap entry in the next loop. But, when memcg is used, we may have
    > + * smaller chance to call global LRU's memory reclaim code.
    > + * Freeing unused swap entry in aggresive way is good for avoid "leak" of swap
    > + * entry accounting.
    > + */
    > +static inline void unuse_swapcache_check_again(struct page *page)
    > +{
    > + /*
    > + * The page is locked, but have extra reference from somewhere.
    > + * In typical case, rotate_reclaimable_page()'s extra refcnt makes
    > + * __remove_mapping fail. (see mm/swap.c)
    > + */
    > + if (PageSwapCache(page))
    > + try_to_free_swap(page);
    > +}
    > +#else
    > +static inline void unuse_swapcache_check_again(struct page *page)
    > +{
    > +}
    > +#endif
    > +
    >
    > /*
    > * shrink_page_list() returns the number of reclaimed pages
    > @@ -758,9 +782,12 @@ static unsigned long shrink_page_list(st
    > }
    > }
    >
    > - if (!mapping || !__remove_mapping(mapping, page))
    > + if (!mapping)
    > goto keep_locked;
    > -
    > + if (!__remove_mapping(mapping, page)) {
    > + unuse_swapcache_check_again(page);
    > + goto keep_locked;
    > + }
    > /*
    > * At this point, we have no other references and there is
    > * no way to pick any more up (removed from LRU, removed
    > Index: mmotm-2.6.30-Apr24/mm/swapfile.c
    > ===================================================================
    > --- mmotm-2.6.30-Apr24.orig/mm/swapfile.c
    > +++ mmotm-2.6.30-Apr24/mm/swapfile.c
    > @@ -528,6 +528,43 @@ static inline int page_swapcount(struct
    > return count;
    > }
    >
    > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR
    > +static inline int page_swapused_nolock(struct page *page)
    > +{
    > + swp_entry_t entry;
    > + unsigned long type, offset;
    > + struct swap_info_struct *p;
    > +
    > + entry.val = page_private(page);
    > + type = swp_type(entry);
    > + VM_BUG_ON(type >= nr_swapfiles);
    > +
    > + offset = swp_offset(entry);
    > + p = &swap_info[type];
    > + VM_BUG_ON(!(p->flags & SWP_USED));
    > + VM_BUG_ON(!(p->swap_map[offset]));
    > +
    > + smp_rmb();
    > + return p->swap_map[offset] != 1;
    > +}
    > +/*
    > + * Use a lapping function not to allow reuse this function other than memcg.
    > + */
    > +int mem_cgroup_staleswap_hint(struct page *page)
    > +{
    > + /*
    > + * The page may not under lock_page() but Writeback is set in that case.
    > + * Then, swap_map is stable when this is called.
    > + * Very terrible troube will not occur even if page_swapused_nolock()
    > + * returns wrong value.
    > + * Because this can be called via interrupt context, we use nolock
    > + * version of swap's refcnt check.
    > + */
    > + if (!PageSwapCache(page) || page_mapped(page))
    > + return 1;
    > + return page_swapused_nolock(page);
    > +}
    > +#endif
    > /*
    > * We can write to an anon page without COW if there are no other references
    > * to it. And as a side-effect, free up its swap: because the old content
    > Index: mmotm-2.6.30-Apr24/include/linux/swap.h
    > ===================================================================
    > --- mmotm-2.6.30-Apr24.orig/include/linux/swap.h
    > +++ mmotm-2.6.30-Apr24/include/linux/swap.h
    > @@ -336,6 +336,7 @@ static inline void disable_swap_token(vo
    >
    > #ifdef CONFIG_CGROUP_MEM_RES_CTLR
    > extern void mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent);
    > +extern int mem_cgroup_staleswap_hint(struct page *page);
    > #else
    > static inline void
    > mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >



    \
     
     \ /
      Last update: 2009-04-30 09:39    [W:4.386 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site