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

[Bob Liu <lliubbo@gmail.com>]

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@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


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>