[PATCH] memcg: change behavior of moving charges at task move

[PATCH] memcg: change behavior of moving charges at task move

[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....

==
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 <hughd@google.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/cgroups/memory.txt |    9 ++++-----
 include/linux/swap.h             |    9 ---------
 mm/memcontrol.c                  |   15 +++++++--------
 mm/swapfile.c                    |   31 -------------------------------
 4 files changed, 11 insertions(+), 53 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b2ee6df..a48d185 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 */
@@ -5161,18 +5161,17 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
+#endif
 	if (do_swap_account)
 		entry->val = ent.val;
 
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1

[PATCH] memcg: change behavior of moving charges at task move

[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....

==
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 <hughd@google.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/cgroups/memory.txt |    9 ++++-----
 include/linux/swap.h             |    9 ---------
 mm/memcontrol.c                  |   15 +++++++--------
 mm/swapfile.c                    |   31 -------------------------------
 4 files changed, 11 insertions(+), 53 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b2ee6df..a48d185 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 */
@@ -5161,18 +5161,17 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
+#endif
 	if (do_swap_account)
 		entry->val = ent.val;
 
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1


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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Naoya Horiguchi]

On Wed, Mar 21, 2012 at 06:52:04PM +0900, KAMEZAWA Hiroyuki wrote:
> 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 <hughd@google.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

It looks good to me.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks,
Naoya

Re: [PATCH] memcg: change behavior of moving charges at task move

[Naoya Horiguchi]

On Wed, Mar 21, 2012 at 06:52:04PM +0900, KAMEZAWA Hiroyuki wrote:
> 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 <hughd@google.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

It looks good to me.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks,
Naoya

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Wed, 21 Mar 2012 18:52:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> 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".

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

But you forgot to tell us :(

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Wed, 21 Mar 2012 18:52:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> 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".

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

But you forgot to tell us :(

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Wed, 21 Mar 2012 18:52:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

>  static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>  			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;
> -	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?

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

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

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Wed, 21 Mar 2012 18:52:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

>  static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>  			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;
> -	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?

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

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

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

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Wed, 21 Mar 2012 18:52:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:

>  static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>  			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;
> -	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?

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

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

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 6:29), Andrew Morton wrote:

> On Wed, 21 Mar 2012 18:52:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>> 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".
> 


Ah, sorry.

Before patch:

The spec was
  "shared anonymous pages are not moved at task_move."
The implementation was
  "shared anonymous page whose mapcount > 2 was not moved at task_move."

After patch:

The spec is:
  "all anonymous pages mapped by the task are moved at task_move.."
The implementation is
  "all anonymous pages mapped by the task are moved at task_move."

Then, with old spec, the implementation was wrong.... shared pages may be moved.
But no one has noticed this behavior until now. 

Then, this patch tries to fix the situation by simplifying the rule.
Maybe no visible effect to users because it was broken for a long time and
this will not change behavior of task_move in most of cases. Anon pages are
not shared unless processes are in a tree. Considering memcg nature, it's hard
to think users move a process in the tree without exec().
And as I pointed out in changelog note, libcgroup etc..will not be affected by
this change.



>> 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.
> 
> But you forgot to tell us :(
> 

Sorry.

-Kame

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 6:29), Andrew Morton wrote:

> On Wed, 21 Mar 2012 18:52:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>> 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".
> 


Ah, sorry.

Before patch:

The spec was
  "shared anonymous pages are not moved at task_move."
The implementation was
  "shared anonymous page whose mapcount > 2 was not moved at task_move."

After patch:

The spec is:
  "all anonymous pages mapped by the task are moved at task_move.."
The implementation is
  "all anonymous pages mapped by the task are moved at task_move."

Then, with old spec, the implementation was wrong.... shared pages may be moved.
But no one has noticed this behavior until now. 

Then, this patch tries to fix the situation by simplifying the rule.
Maybe no visible effect to users because it was broken for a long time and
this will not change behavior of task_move in most of cases. Anon pages are
not shared unless processes are in a tree. Considering memcg nature, it's hard
to think users move a process in the tree without exec().
And as I pointed out in changelog note, libcgroup etc..will not be affected by
this change.



>> 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.
> 
> But you forgot to tell us :(
> 

Sorry.

-Kame


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

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 6:36), Andrew Morton wrote:

> On Wed, 21 Mar 2012 18:52:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>>  static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>>  			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;
>> -	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?
> 


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

>> +	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.

Thank you for review!
-Kame

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 6:36), Andrew Morton wrote:

> On Wed, 21 Mar 2012 18:52:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>>  static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>>  			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;
>> -	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?
> 


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

>> +	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.

Thank you for review!
-Kame

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> >> +#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.

--- a/mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix
+++ a/mm/memcontrol.c
@@ -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

> >> +	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.

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Andrew Morton]

On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> >> +#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.

--- a/mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix
+++ a/mm/memcontrol.c
@@ -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

> >> +	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.

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

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 9:30), Andrew Morton wrote:

> On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>>>> +#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.
> 
> --- a/mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix
> +++ a/mm/memcontrol.c
> @@ -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
> 
>>>> +	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.
> 
> Confused.  The new reference to swapper_space is already inside #ifdef
> CONFIG_SWAP.

Sorry for confusion. Above log was I saw when I removed CONFIG_SWAP as a trial.

How about this ? Maybe cleaner.
==

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 comment around find_get_page()
  - changed CONFIG_SWAP handling
  - updated patch description

Suggested-by: Hugh Dickins <hughd@google.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 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(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
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
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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;
-	}
+	/*
+	 * Because lookup_swap_cache() updates some statistics counter,
+	 * we call find_get_page() with swapper_space directly.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
 	if (do_swap_account)
 		entry->val = ent.val;
 
 	return page;
 }
+#else
+static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
+			unsigned long addr, pte_t ptent, swp_entry_t *entry)
+{
+	return NULL;
+}
+#endif
 
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 9:30), Andrew Morton wrote:

> On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
>>>> +#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.
> 
> --- a/mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix
> +++ a/mm/memcontrol.c
> @@ -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
> 
>>>> +	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.
> 
> Confused.  The new reference to swapper_space is already inside #ifdef
> CONFIG_SWAP.

Sorry for confusion. Above log was I saw when I removed CONFIG_SWAP as a trial.

How about this ? Maybe cleaner.
==

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 comment around find_get_page()
  - changed CONFIG_SWAP handling
  - updated patch description

Suggested-by: Hugh Dickins <hughd@google.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 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(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
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
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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;
-	}
+	/*
+	 * Because lookup_swap_cache() updates some statistics counter,
+	 * we call find_get_page() with swapper_space directly.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
 	if (do_swap_account)
 		entry->val = ent.val;
 
 	return page;
 }
+#else
+static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
+			unsigned long addr, pte_t ptent, swp_entry_t *entry)
+{
+	return NULL;
+}
+#endif
 
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1






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

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

(2012/03/23 9:30), Andrew Morton wrote:

> On Fri, 23 Mar 2012 09:18:46 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> 
>>>> +#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.
> 
> --- a/mm/memcontrol.c~memcg-change-behavior-of-moving-charges-at-task-move-fix
> +++ a/mm/memcontrol.c
> @@ -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
> 
>>>> +	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.
> 
> Confused.  The new reference to swapper_space is already inside #ifdef
> CONFIG_SWAP.

Sorry for confusion. Above log was I saw when I removed CONFIG_SWAP as a trial.

How about this ? Maybe cleaner.
==

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 comment around find_get_page()
  - changed CONFIG_SWAP handling
  - updated patch description

Suggested-by: Hugh Dickins <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 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(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
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
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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;
-	}
+	/*
+	 * Because lookup_swap_cache() updates some statistics counter,
+	 * we call find_get_page() with swapper_space directly.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
 	if (do_swap_account)
 		entry->val = ent.val;
 
 	return page;
 }
+#else
+static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
+			unsigned long addr, pte_t ptent, swp_entry_t *entry)
+{
+	return NULL;
+}
+#endif
 
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1

Re: [PATCH] memcg: change behavior of moving charges at task move

[Johannes Weiner]

On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
> 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 :)

Re: [PATCH] memcg: change behavior of moving charges at task move

[Johannes Weiner]

On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
> 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 :)

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

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

> On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
>> 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 <kamezawa.hiroyu@jp.fujitsu.com>
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 <hughd@google.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 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(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b2ee6df..962b97a 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 CONFIG_SWAP
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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;
-	}
+	/*
+	 * Because lookup_swap_cache() updates some statistics counter,
+	 * we call find_get_page() with swapper_space directly.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
 	if (do_swap_account)
 		entry->val = ent.val;
 
 	return page;
 }
+#else
+static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
+			unsigned long addr, pte_t ptent, swp_entry_t *entry)
+{
+	return NULL;
+}
+#endif
 
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

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

> On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
>> 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 ;(


==

Re: [PATCH] memcg: change behavior of moving charges at task move

[KAMEZAWA Hiroyuki]

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

> On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
>> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
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 <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 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(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 4c95c00..84d4f00 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -185,12 +185,14 @@ behind this approach is that a cgroup that aggressively uses a shared
 page will eventually get charged for it (once it is uncharged from
 the cgroup that brought it in -- this will happen on memory pressure).
 
+But see section 8.2: when moving a task to another cgroup, its pages may
+be recharged to the new cgroup, if move_charge_at_immigrate has been chosen.
+
 Exception: If CONFIG_CGROUP_CGROUP_MEM_RES_CTLR_SWAP is not used.
 When you do swapoff and make swapped-out pages of shmem(tmpfs) to
 be backed into memory in force, charges for pages are accounted against the
 caller of swapoff rather than the users of shmem.
 
-
 2.4 Swap Extension (CONFIG_CGROUP_MEM_RES_CTLR_SWAP)
 
 Swap Extension allows you to record charge for swap. A swapped-in page is
@@ -623,8 +625,7 @@ memory cgroup.
   bit | what type of charges would be moved ?
  -----+------------------------------------------------------------------------
    0  | A charge of an anonymous page(or swap of it) used by the target task.
-      | Those pages and swaps must be used only by the target task. You must
-      | enable Swap Extension(see 2.4) to enable move of swap charges.
+      | You must enable Swap Extension(see 2.4) to enable move of swap charges.
  -----+------------------------------------------------------------------------
    1  | A charge of file pages(normal file, tmpfs file(e.g. ipc shared memory)
       | and swaps of tmpfs file) mmapped by the target task. Unlike the case of
@@ -637,8 +638,6 @@ memory cgroup.
 
 8.3 TODO
 
-- Implement madvise(2) to let users decide the vma to be moved or not to be
-  moved.
 - All of moving charge operations are done under cgroup_mutex. It's not good
   behavior to hold the mutex too long, so we may need some trick.
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 6e66c03..70d2c74 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -387,7 +387,6 @@ static inline void deactivate_swap_token(struct mm_struct *mm, bool swap_token)
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 extern void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout);
-extern int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep);
 #else
 static inline void
 mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
@@ -532,14 +531,6 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
 {
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-static inline int
-mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	return 0;
-}
-#endif
-
 #endif /* CONFIG_SWAP */
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b2ee6df..962b97a 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 CONFIG_SWAP
 static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
 			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;
-	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;
-	}
+	/*
+	 * Because lookup_swap_cache() updates some statistics counter,
+	 * we call find_get_page() with swapper_space directly.
+	 */
+	page = find_get_page(&swapper_space, ent.val);
 	if (do_swap_account)
 		entry->val = ent.val;
 
 	return page;
 }
+#else
+static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
+			unsigned long addr, pte_t ptent, swp_entry_t *entry)
+{
+	return NULL;
+}
+#endif
 
 static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index dae42f3..fedeb6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -717,37 +717,6 @@ int free_swap_and_cache(swp_entry_t entry)
 	return p != NULL;
 }
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR
-/**
- * mem_cgroup_count_swap_user - count the user of a swap entry
- * @ent: the swap entry to be checked
- * @pagep: the pointer for the swap cache page of the entry to be stored
- *
- * Returns the number of the user of the swap entry. The number is valid only
- * for swaps of anonymous pages.
- * If the entry is found on swap cache, the page is stored to pagep with
- * refcount of it being incremented.
- */
-int mem_cgroup_count_swap_user(swp_entry_t ent, struct page **pagep)
-{
-	struct page *page;
-	struct swap_info_struct *p;
-	int count = 0;
-
-	page = find_get_page(&swapper_space, ent.val);
-	if (page)
-		count += page_mapcount(page);
-	p = swap_info_get(ent);
-	if (p) {
-		count += swap_count(p->swap_map[swp_offset(ent)]);
-		spin_unlock(&swap_lock);
-	}
-
-	*pagep = page;
-	return count;
-}
-#endif
-
 #ifdef CONFIG_HIBERNATION
 /*
  * Find the swap type that corresponds to given device (if any).
-- 
1.7.4.1

Re: [PATCH] memcg: change behavior of moving charges at task move

[Michal Hocko]

On Fri 23-03-12 18:00:34, KAMEZAWA Hiroyuki wrote:
> (2012/03/23 17:53), Johannes Weiner wrote:
> 
> > On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
> >> 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 <kamezawa.hiroyu@jp.fujitsu.com>
> 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 <mhocko@suse.cz>

> 
> Suggested-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Michal Hocko]

On Fri 23-03-12 18:00:34, KAMEZAWA Hiroyuki wrote:
> (2012/03/23 17:53), Johannes Weiner wrote:
> 
> > On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
> >> 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 <kamezawa.hiroyu@jp.fujitsu.com>
> 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 <mhocko@suse.cz>

> 
> Suggested-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  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

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

Re: [PATCH] memcg: change behavior of moving charges at task move

[Michal Hocko]

On Fri 23-03-12 18:00:34, KAMEZAWA Hiroyuki wrote:
> (2012/03/23 17:53), Johannes Weiner wrote:
> 
> > On Fri, Mar 23, 2012 at 09:52:28AM +0900, KAMEZAWA Hiroyuki wrote:
> >> 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 <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 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 <mhocko-AlSwsSmVLrQ@public.gmane.org>

> 
> Suggested-by: Hugh Dickins <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
>  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