memcg: change behavior of moving charges at task move

As discussed before, I post this to fix the spec and implementation of task moving. Then, do you think what target kernel version should be ? 3.4/3.5 ? but yes, it may be late for 3.4....

==
In documentation, it's said that 'shared anon are not moved'. But in implementation, the check was wrong.

  if (!move_anon() || page_mapcount(page) > 2)

Ah, memcg has been moving shared anon pages for a long time.

Then, here is a discussion about handling of shared anon pages.

- It's complex
- Now, shared file caches are moved in force.
- It adds unclear check as page_mapcount(). To do correct check, we should check swap users, etc.
- No one notice this implementation behavior. So, no one get benefit from the design.
- In general, once task is moved to a cgroup for running, it will not be moved....
- Finally, we have control knob as memory.move_charge_at_immigrate.

Here is a patch to allow moving shared pages, completely. This makes memcg simpler and fix current broken code.

Note:
IIUC, libcgroup's cgroup daemon moves tasks after exec(). So, it's not affected.
libcgroup's command "cgexec" does move itsef to a memcg and call exec() without fork(). it's not affected.

Changelog:
- fixed PageAnon() check.
- remove call of lookup_swap_cache()
- fixed Documentation.

Suggested-by Hugh Dickins <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

As discussed before, I post this to fix the spec and implementation of task moving. Then, do you think what target kernel version should be ? 3.4/3.5 ? but yes, it may be late for 3.4....

In documentation, it's said that 'shared anon are not moved'. But in implementation, the check was wrong.

  if (!move_anon() || page_mapcount(page) > 2)

Ah, memcg has been moving shared anon pages for a long time.

Then, here is a discussion about handling of shared anon pages.

- It's complex
- Now, shared file caches are moved in force.
- It adds unclear check as page_mapcount(). To do correct check, we should check swap users, etc.
- No one notice this implementation behavior. So, no one get benefit from the design.
- In general, once task is moved to a cgroup for running, it will not be moved....
- Finally, we have control knob as memory.move_charge_at_immigrate.

Here is a patch to allow moving shared pages, completely. This makes memcg simpler and fix current broken code.

Note:
IIUC, libcgroup's cgroup daemon moves tasks after exec(). So, it's not affected.
libcgroup's command "cgexec" does move itsef to a memcg and call exec() without fork(). it's not affected.

Changelog:
- fixed PageAnon() check.
- remove call of lookup_swap_cache()
- fixed Documentation.

Quoted 3 lines · On Wed, 21 Mar 2012 18:52:04 +0900 KAMEZAWA Hiroyuki…

As discussed before, I post this to fix the spec and implementation of task moving. Then, do you think what target kernel version should be ? 3.4/3.5 ? but yes, it may be late for 3.4....

Well, the key information here is "what effect does the bug have upon users".

Quoted 28 lines

In documentation, it's said that 'shared anon are not moved'. But in implementation, the check was wrong.

  if (!move_anon() || page_mapcount(page) > 2)

Ah, memcg has been moving shared anon pages for a long time.

Then, here is a discussion about handling of shared anon pages.

- It's complex
- Now, shared file caches are moved in force.
- It adds unclear check as page_mapcount(). To do correct check, we should check swap users, etc.
- No one notice this implementation behavior. So, no one get benefit from the design.
- In general, once task is moved to a cgroup for running, it will not be moved....
- Finally, we have control knob as memory.move_charge_at_immigrate.

Here is a patch to allow moving shared pages, completely. This makes memcg simpler and fix current broken code.

Note:
IIUC, libcgroup's cgroup daemon moves tasks after exec(). So, it's not affected.
libcgroup's command "cgexec" does move itsef to a memcg and call exec() without fork(). it's not affected.

Quoted 4 lines

Changelog:
- fixed PageAnon() check.
- remove call of lookup_swap_cache()
- fixed Documentation.

But you forgot to tell us :(

static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,

mm/memcontrol.c +0 -1

 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
 {
-	int usage_count;
 	struct page *page = NULL;
 	swp_entry_t ent = pte_to_swp_entry(ptent);
 
 	if (!move_anon() || non_swap_entry(ent))
 		return NULL;

mm/memcontrol.c +1 -0

+#endif
 	if (do_swap_account)
 		entry->val = ent.val;

Quoted 7 lines · On Wed, 21 Mar 2012 18:52:04 +0900 KAMEZAWA Hiroyuki…

static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,

mm/memcontrol.c +0 -1

 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
 {
-	int usage_count;
 	struct page *page = NULL;
 	swp_entry_t ent = pte_to_swp_entry(ptent);

mm/memcontrol.c static struct page *mc_handle_present_pte(struct vm_area_struct *vma, · line 5172

Quoted 12 lines

mm/memcontrol.c +4 -6

 	if (!move_anon() || non_swap_entry(ent))
 		return NULL;
-	usage_count = mem_cgroup_count_swap_user(ent, &page);
-	if (usage_count > 1) { /* we don't move shared anon */
-		if (page)
-			put_page(page);
-		return NULL;
-	}
+#ifdef CONFIG_SWAP
+	/*
+	 * Avoid lookup_swap_cache() not to update statistics.
+	 */

I don't understand this comment - what is it trying to tell us?

mm/memcontrol.c static struct page *mc_handle_present_pte(struct vm_area_struct *vma, · line 5173

Quoted 1 line

mm/memcontrol.c +1 -0

+	page = find_get_page(&swapper_space, ent.val);

The code won't even compile if CONFIG_SWAP=n?

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+#ifdef CONFIG_SWAP
+	/*
+	 * Avoid lookup_swap_cache() not to update statistics.
+	 */

I don't understand this comment - what is it trying to tell us?

High Dickins advised me to use find_get_page() rather than lookup_swap_cache() because lookup_swap_cache() has some statistics with swap.

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	page = find_get_page(&swapper_space, ent.val);

The code won't even compile if CONFIG_SWAP=n?

mm/built-in.o: In function `mc_handle_swap_pte':
/home/kamezawa/Kernel/next/linux/mm/memcontrol.c:5172: undefined reference to `swapper_space'
make: *** [.tmp_vmlinux1] Error 1

Ah...but I think this function (mc_handle_swap_pte) itself should be under CONFIG_SWAP. I'll post v2.

Quoted 11 lines · On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki…

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+#ifdef CONFIG_SWAP
+	/*
+	 * Avoid lookup_swap_cache() not to update statistics.
+	 */

I don't understand this comment - what is it trying to tell us?

High Dickins advised me to use find_get_page() rather than lookup_swap_cache() because lookup_swap_cache() has some statistics with swap.

ah.

mm/memcontrol.c renamed from mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix +2 -1

@@ -5137,7 +5137,8 @@ static struct page *mc_handle_swap_pte(s
 		return NULL;
 #ifdef CONFIG_SWAP
 	/*
-	 * Avoid lookup_swap_cache() not to update statistics.
+	 * Use find_get_page() rather than lookup_swap_cache() because the
+	 * latter alters statistics.
 	 */
 	page = find_get_page(&swapper_space, ent.val);
 #endif

Quoted 10 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	page = find_get_page(&swapper_space, ent.val);

The code won't even compile if CONFIG_SWAP=n?

mm/built-in.o: In function `mc_handle_swap_pte':
/home/kamezawa/Kernel/next/linux/mm/memcontrol.c:5172: undefined reference to `swapper_space'
make: *** [.tmp_vmlinux1] Error 1

Quoted 2 lines

Ah...but I think this function (mc_handle_swap_pte) itself should be under CONFIG_SWAP. I'll post v2.

Confused. The new reference to swapper_space is already inside #ifdef CONFIG_SWAP.

mm/memcontrol.c +1 -1

index b2ee6df..ca8b3a1 100644
@@ -5147,7 +5147,7 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 		return NULL;
 	if (PageAnon(page)) {
 		/* we don't move shared anon */
-		if (!move_anon() || page_mapcount(page) > 2)
+		if (!move_anon())
 			return NULL;
 	} else if (!move_file())
 		/* we ignore mapcount for file pages */

Quoted 17 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +1 -1

index b2ee6df..ca8b3a1 100644
@@ -5147,7 +5147,7 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 		return NULL;
 	if (PageAnon(page)) {
 		/* we don't move shared anon */
-		if (!move_anon() || page_mapcount(page) > 2)
+		if (!move_anon())
 			return NULL;
 	} else if (!move_file())
 		/* we ignore mapcount for file pages */

@@ -5158,26 +5158,32 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 	return page;
 }
 

mm/memcontrol.c static struct page *mc_handle_present_pte(struct vm_area_struct *vma, · line 5161

Quoted 1 line

mm/memcontrol.c +1 -0

+#ifdef CONFFIG_SWAP

That will probably disable it for good :)

Quoted 18 lines · Johannes Weiner

Quoted 17 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +1 -1

index b2ee6df..ca8b3a1 100644
@@ -5147,7 +5147,7 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 		return NULL;
 	if (PageAnon(page)) {
 		/* we don't move shared anon */
-		if (!move_anon() || page_mapcount(page) > 2)
+		if (!move_anon())
 			return NULL;
 	} else if (!move_file())
 		/* we ignore mapcount for file pages */

@@ -5158,26 +5158,32 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 	return page;
 }
 

Quoted 4 lines

mm/memcontrol.c static struct page *mc_handle_present_pte(struct vm_area_struct *vma, · line 5161

Quoted 1 line

mm/memcontrol.c +1 -0

+#ifdef CONFFIG_SWAP

That will probably disable it for good :)

Thank you for your good eyes.. I feel I can't trust my eyes ;(

==
From d7ed385bad22d352bb28aeb9380591b72ec5bec5 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Wed, 21 Mar 2012 19:03:40 +0900
Subject: [PATCH] memcg: fix/change behavior of shared anon at moving task.

This patch changes memcg's behavior at task_move().

At task_move(), the kernel scans a task's page table and move the changes for mapped pages from source cgroup to target cgroup.

There has been a bug at handling shared anonymous pages for a long time.

Before patch:
- The spec says 'shared anonymous pages are not moved.'
- The implementation was 'shared anonymoys pages may be moved'. If page_mapcount <=2, shared anonymous pages's charge were moved.

After patch:
- The spec says 'all anonymous pages are moved'.
- The implementation is 'all anonymous pages are moved'.

Considering usage of memcg, this will not affect user's experience. 'shared anonymous' pages only exists between a tree of processes which don't do exec(). Moving one of process without exec() seems not sane.

For example, libcgroup will not be affected by this change. (Anyway, no one noticed the implementation for a long time...)

Below is a discussion log:

- current spec/implementation are complex
- Now, shared file caches are moved
- It adds unclear check as page_mapcount(). To do correct check, we should check swap users, etc.
- No one notice this implementation behavior. So, no one get benefit from the design.
- In general, once task is moved to a cgroup for running, it will not be moved....
- Finally, we have control knob as memory.move_charge_at_immigrate.

Here is a patch to allow moving shared pages, completely. This makes memcg simpler and fix current broken code.

Changelog:
- fixed CONFFIG_SWAP...
Changelog:
- fixed comment around find_get_page()
- changed CONFIG_SWAP handling
- updated patch description

Suggested-by Hugh Dickins <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 Documentation/cgroups/memory.txt |    9 ++++-----
 include/linux/swap.h             |    9 ---------
 mm/memcontrol.c                  |   22 ++++++++++++++--------
 mm/swapfile.c                    |   31 -------------------------------
 4 files changed, 18 insertions(+), 53 deletions(-)