On Fri 23-03-12 18:00:34, KAMEZAWA Hiroyuki wrote:

(2012/03/23 17:53), Johannes Weiner wrote:

quoted

On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:

quoted

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b2ee6df..ca8b3a1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c

@@ -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;
 }
 
+#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 <redacted>
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.

charges?

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. 

Why it wouldn't be 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

Anyway the patch looks good to me and I agree that we should make anon
moving easier.
Acked-by: Michal Hocko <redacted>

Suggested-by: Hugh Dickins <redacted>
Signed-off-by: KAMEZAWA Hiroyuki <redacted>
---
 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(-)

[...]
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic