memcg: cleanup mem_cgroup_move_parent()

mem_cgroup_move_parent() calls try_charge first and cancel_charge on failure. IMHO, charge/uncharge(especially charge) is high cost operation, so we should avoid it as far as possible. This patch tries to delay try_charge in mem_cgroup_move_parent() by re-ordering checks it does. And this patch renames mem_cgroup_move_account() to __mem_cgroup_move_account(), changes the return value of __mem_cgroup_move_account() from int to void, and adds a new wrapper(mem_cgroup_move_account()), which checks whether a @pc is valid for moving account and calls __mem_cgroup_move_account(). This patch removes the last caller of trylock_page_cgroup(), so removes its definition too. Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

memcg: cleanup mem_cgroup_move_parent()
mem_cgroup_move_parent() calls try_charge first and cancel_charge on failure. IMHO, charge/uncharge(especially charge) is high cost operation, so we should avoid it as far as possible. This patch tries to delay try_charge in mem_cgroup_move_parent() by re-ordering checks it does. And this patch renames mem_cgroup_move_account() to __mem_cgroup_move_account(), changes the return value of __mem_cgroup_move_account() from int to void, and adds a new wrapper(mem_cgroup_move_account()), which checks whether a @pc is valid for moving account and calls __mem_cgroup_move_account(). This patch removes the last caller of trylock_page_cgroup(), so removes its definition too. Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Daisuke Nishimura · Linus Torvalds
1 parent a3032a2c15
Showing 2 changed files with 37 additions and 54 deletions Side-by-side Diff
include/linux/page_cgroup.h
mm/memcontrol.c
@@ -57,6 +57,8 @@
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
  
+TESTPCGFLAG(Locked, LOCK)
+
 /* Cache flag is set only once (at allocation) */
 TESTPCGFLAG(Cache, CACHE)
 CLEARPCGFLAG(Cache, CACHE)
@@ -84,11 +86,6 @@
 static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	bit_spin_lock(PCG_LOCK, &pc->flags);
-}
-
-static inline int trylock_page_cgroup(struct page_cgroup *pc)
-{
-	return bit_spin_trylock(PCG_LOCK, &pc->flags);
 }
  
 static inline void unlock_page_cgroup(struct page_cgroup *pc)
@@ -1613,27 +1613,22 @@
 }
  
 /**
- * mem_cgroup_move_account - move account of the page
+ * __mem_cgroup_move_account - move account of the page
  * @pc:	page_cgroup of the page.
  * @from: mem_cgroup which the page is moved from.
  * @to:	mem_cgroup which the page is moved to. @from != @to.
  *
  * The caller must confirm following.
  * - page is not on LRU (isolate_page() is useful.)
+ * - the pc is locked, used, and ->mem_cgroup points to @from.
  *
- * returns 0 at success,
- * returns -EBUSY when lock is busy or "pc" is unstable.
- *
  * This function does "uncharge" from old cgroup but doesn't do "charge" to
  * new cgroup. It should be done by a caller.
  */
  
-static int mem_cgroup_move_account(struct page_cgroup *pc,
+static void __mem_cgroup_move_account(struct page_cgroup *pc,
 	struct mem_cgroup *from, struct mem_cgroup *to)
 {
-	struct mem_cgroup_per_zone *from_mz, *to_mz;
-	int nid, zid;
-	int ret = -EBUSY;
 	struct page *page;
 	int cpu;
 	struct mem_cgroup_stat *stat;
  
@@ -1641,21 +1636,10 @@
  
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(pc->page));
+	VM_BUG_ON(!PageCgroupLocked(pc));
+	VM_BUG_ON(!PageCgroupUsed(pc));
+	VM_BUG_ON(pc->mem_cgroup != from);
  
-	nid = page_cgroup_nid(pc);
-	zid = page_cgroup_zid(pc);
-	from_mz =  mem_cgroup_zoneinfo(from, nid, zid);
-	to_mz =  mem_cgroup_zoneinfo(to, nid, zid);
-
-	if (!trylock_page_cgroup(pc))
-		return ret;
-
-	if (!PageCgroupUsed(pc))
-		goto out;
-
-	if (pc->mem_cgroup != from)
-		goto out;
-
 	if (!mem_cgroup_is_root(from))
 		res_counter_uncharge(&from->res, PAGE_SIZE);
 	mem_cgroup_charge_statistics(from, pc, false);
  
@@ -1683,15 +1667,28 @@
 	css_get(&to->css);
 	pc->mem_cgroup = to;
 	mem_cgroup_charge_statistics(to, pc, true);
-	ret = 0;
-out:
-	unlock_page_cgroup(pc);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
 	 * this function is just force_empty() and it's garanteed that
 	 * "to" is never removed. So, we don't check rmdir status here.
 	 */
+}
+
+/*
+ * check whether the @pc is valid for moving account and call
+ * __mem_cgroup_move_account()
+ */
+static int mem_cgroup_move_account(struct page_cgroup *pc,
+				struct mem_cgroup *from, struct mem_cgroup *to)
+{
+	int ret = -EINVAL;
+	lock_page_cgroup(pc);
+	if (PageCgroupUsed(pc) && pc->mem_cgroup == from) {
+		__mem_cgroup_move_account(pc, from, to);
+		ret = 0;
+	}
+	unlock_page_cgroup(pc);
 	return ret;
 }
  
  
  
  
  
  
  
@@ -1713,38 +1710,27 @@
 	if (!pcg)
 		return -EINVAL;
  
+	ret = -EBUSY;
+	if (!get_page_unless_zero(page))
+		goto out;
+	if (isolate_lru_page(page))
+		goto put;
  
 	parent = mem_cgroup_from_cont(pcg);
-
-
 	ret = __mem_cgroup_try_charge(NULL, gfp_mask, &parent, false, page);
 	if (ret || !parent)
-		return ret;
+		goto put_back;
  
-	if (!get_page_unless_zero(page)) {
-		ret = -EBUSY;
-		goto uncharge;
-	}
-
-	ret = isolate_lru_page(page);
-
-	if (ret)
-		goto cancel;
-
 	ret = mem_cgroup_move_account(pc, child, parent);
-
+	if (!ret)
+		css_put(&parent->css);	/* drop extra refcnt by try_charge() */
+	else
+		mem_cgroup_cancel_charge(parent);	/* does css_put */
+put_back:
 	putback_lru_page(page);
-	if (!ret) {
-		put_page(page);
-		/* drop extra refcnt by try_charge() */
-		css_put(&parent->css);
-		return 0;
-	}
-
-cancel:
+put:
 	put_page(page);
-uncharge:
-	mem_cgroup_cancel_charge(parent);
+out:
 	return ret;
 }
...	...	@@ -57,6 +57,8 @@
57	57	static inline int TestClearPageCgroup##uname(struct page_cgroup *pc) \
58	58	{ return test_and_clear_bit(PCG_##lname, &pc->flags); }
59	59
	60	+TESTPCGFLAG(Locked, LOCK)
	61	+
60	62	/* Cache flag is set only once (at allocation) */
61	63	TESTPCGFLAG(Cache, CACHE)
62	64	CLEARPCGFLAG(Cache, CACHE)
...	...	@@ -84,11 +86,6 @@
84	86	static inline void lock_page_cgroup(struct page_cgroup *pc)
85	87	{
86	88	bit_spin_lock(PCG_LOCK, &pc->flags);
87		-}
88		-
89		-static inline int trylock_page_cgroup(struct page_cgroup *pc)
90		-{
91		- return bit_spin_trylock(PCG_LOCK, &pc->flags);
92	89	}
93	90
94	91	static inline void unlock_page_cgroup(struct page_cgroup *pc)
...	...	@@ -1613,27 +1613,22 @@
1613	1613	}
1614	1614
1615	1615	/**
1616		- * mem_cgroup_move_account - move account of the page
	1616	+ * __mem_cgroup_move_account - move account of the page
1617	1617	* @pc: page_cgroup of the page.
1618	1618	* @from: mem_cgroup which the page is moved from.
1619	1619	* @to: mem_cgroup which the page is moved to. @from != @to.
1620	1620	*
1621	1621	* The caller must confirm following.
1622	1622	* - page is not on LRU (isolate_page() is useful.)
	1623	+ * - the pc is locked, used, and ->mem_cgroup points to @from.
1623	1624	*
1624		- * returns 0 at success,
1625		- * returns -EBUSY when lock is busy or "pc" is unstable.
1626		- *
1627	1625	* This function does "uncharge" from old cgroup but doesn't do "charge" to
1628	1626	* new cgroup. It should be done by a caller.
1629	1627	*/
1630	1628
1631		-static int mem_cgroup_move_account(struct page_cgroup *pc,
	1629	+static void __mem_cgroup_move_account(struct page_cgroup *pc,
1632	1630	struct mem_cgroup from, struct mem_cgroup to)
1633	1631	{
1634		- struct mem_cgroup_per_zone from_mz, to_mz;
1635		- int nid, zid;
1636		- int ret = -EBUSY;
1637	1632	struct page *page;
1638	1633	int cpu;
1639	1634	struct mem_cgroup_stat *stat;
1640	1635
...	...	@@ -1641,21 +1636,10 @@
1641	1636
1642	1637	VM_BUG_ON(from == to);
1643	1638	VM_BUG_ON(PageLRU(pc->page));
	1639	+ VM_BUG_ON(!PageCgroupLocked(pc));
	1640	+ VM_BUG_ON(!PageCgroupUsed(pc));
	1641	+ VM_BUG_ON(pc->mem_cgroup != from);
1644	1642
1645		- nid = page_cgroup_nid(pc);
1646		- zid = page_cgroup_zid(pc);
1647		- from_mz = mem_cgroup_zoneinfo(from, nid, zid);
1648		- to_mz = mem_cgroup_zoneinfo(to, nid, zid);
1649		-
1650		- if (!trylock_page_cgroup(pc))
1651		- return ret;
1652		-
1653		- if (!PageCgroupUsed(pc))
1654		- goto out;
1655		-
1656		- if (pc->mem_cgroup != from)
1657		- goto out;
1658		-
1659	1643	if (!mem_cgroup_is_root(from))
1660	1644	res_counter_uncharge(&from->res, PAGE_SIZE);
1661	1645	mem_cgroup_charge_statistics(from, pc, false);
1662	1646
...	...	@@ -1683,15 +1667,28 @@
1683	1667	css_get(&to->css);
1684	1668	pc->mem_cgroup = to;
1685	1669	mem_cgroup_charge_statistics(to, pc, true);
1686		- ret = 0;
1687		-out:
1688		- unlock_page_cgroup(pc);
1689	1670	/*
1690	1671	* We charges against "to" which may not have any tasks. Then, "to"
1691	1672	* can be under rmdir(). But in current implementation, caller of
1692	1673	* this function is just force_empty() and it's garanteed that
1693	1674	* "to" is never removed. So, we don't check rmdir status here.
1694	1675	*/
	1676	+}
	1677	+
	1678	+/*
	1679	+ * check whether the @pc is valid for moving account and call
	1680	+ * __mem_cgroup_move_account()
	1681	+ */
	1682	+static int mem_cgroup_move_account(struct page_cgroup *pc,
	1683	+ struct mem_cgroup from, struct mem_cgroup to)
	1684	+{
	1685	+ int ret = -EINVAL;
	1686	+ lock_page_cgroup(pc);
	1687	+ if (PageCgroupUsed(pc) && pc->mem_cgroup == from) {
	1688	+ __mem_cgroup_move_account(pc, from, to);
	1689	+ ret = 0;
	1690	+ }
	1691	+ unlock_page_cgroup(pc);
1695	1692	return ret;
1696	1693	}
1697	1694
1698	1695
1699	1696
1700	1697
1701	1698
1702	1699
1703	1700
...	...	@@ -1713,38 +1710,27 @@
1713	1710	if (!pcg)
1714	1711	return -EINVAL;
1715	1712
	1713	+ ret = -EBUSY;
	1714	+ if (!get_page_unless_zero(page))
	1715	+ goto out;
	1716	+ if (isolate_lru_page(page))
	1717	+ goto put;
1716	1718
1717	1719	parent = mem_cgroup_from_cont(pcg);
1718		-
1719		-
1720	1720	ret = __mem_cgroup_try_charge(NULL, gfp_mask, &parent, false, page);
1721	1721	if (ret \|\| !parent)
1722		- return ret;
	1722	+ goto put_back;
1723	1723
1724		- if (!get_page_unless_zero(page)) {
1725		- ret = -EBUSY;
1726		- goto uncharge;
1727		- }
1728		-
1729		- ret = isolate_lru_page(page);
1730		-
1731		- if (ret)
1732		- goto cancel;
1733		-
1734	1724	ret = mem_cgroup_move_account(pc, child, parent);
1735		-
	1725	+ if (!ret)
	1726	+ css_put(&parent->css); /* drop extra refcnt by try_charge() */
	1727	+ else
	1728	+ mem_cgroup_cancel_charge(parent); /* does css_put */
	1729	+put_back:
1736	1730	putback_lru_page(page);
1737		- if (!ret) {
1738		- put_page(page);
1739		- /* drop extra refcnt by try_charge() */
1740		- css_put(&parent->css);
1741		- return 0;
1742		- }
1743		-
1744		-cancel:
	1731	+put:
1745	1732	put_page(page);
1746		-uncharge:
1747		- mem_cgroup_cancel_charge(parent);
	1733	+out:
1748	1734	return ret;
1749	1735	}
1750	1736