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) {
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@xxxxxxxxx>
---
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>