[patch] mm: memcontrol: avoid charge statistics churn during page migration

[patch] mm: memcontrol: avoid charge statistics churn during page migration

[Johannes Weiner]

Charge migration currently disables IRQs twice to update the charge
statistics for the old page and then again for the new page.

But migration is a seemless transition of a charge from one physical
page to another one of the same size, so this should be a non-event
from an accounting point of view.  Leave the statistics alone.

Signed-off-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
---
 mm/memcontrol.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 475ecadd9646..8d65dadeec1b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2728,7 +2728,7 @@ static void unlock_page_lru(struct page *page, int isolated)
 }
 
 static void commit_charge(struct page *page, struct mem_cgroup *memcg,
-			  unsigned int nr_pages, bool lrucare)
+			  bool lrucare)
 {
 	struct page_cgroup *pc = lookup_page_cgroup(page);
 	int isolated;
@@ -2765,16 +2765,6 @@ static void commit_charge(struct page *page, struct mem_cgroup *memcg,
 
 	if (lrucare)
 		unlock_page_lru(page, isolated);
-
-	local_irq_disable();
-	mem_cgroup_charge_statistics(memcg, page, nr_pages);
-	/*
-	 * "charge_statistics" updated event counter. Then, check it.
-	 * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
-	 * if they exceeds softlimit.
-	 */
-	memcg_check_events(memcg, page);
-	local_irq_enable();
 }
 
 static DEFINE_MUTEX(set_limit_mutex);
@@ -6460,12 +6450,17 @@ void mem_cgroup_commit_charge(struct page *page, struct mem_cgroup *memcg,
 	if (!memcg)
 		return;
 
+	commit_charge(page, memcg, lrucare);
+
 	if (PageTransHuge(page)) {
 		nr_pages <<= compound_order(page);
 		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
 	}
 
-	commit_charge(page, memcg, nr_pages, lrucare);
+	local_irq_disable();
+	mem_cgroup_charge_statistics(memcg, page, nr_pages);
+	memcg_check_events(memcg, page);
+	local_irq_enable();
 
 	if (do_swap_account && PageSwapCache(page)) {
 		swp_entry_t entry = { .val = page_private(page) };
@@ -6651,7 +6646,6 @@ void mem_cgroup_uncharge_list(struct list_head *page_list)
 void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 			bool lrucare)
 {
-	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
 	int isolated;
 
@@ -6660,6 +6654,8 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 	VM_BUG_ON_PAGE(!lrucare && PageLRU(oldpage), oldpage);
 	VM_BUG_ON_PAGE(!lrucare && PageLRU(newpage), newpage);
 	VM_BUG_ON_PAGE(PageAnon(oldpage) != PageAnon(newpage), newpage);
+	VM_BUG_ON_PAGE(PageTransHuge(oldpage) != PageTransHuge(newpage),
+		       newpage);
 
 	if (mem_cgroup_disabled())
 		return;
@@ -6677,12 +6673,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 	VM_BUG_ON_PAGE(!(pc->flags & PCG_MEM), oldpage);
 	VM_BUG_ON_PAGE(do_swap_account && !(pc->flags & PCG_MEMSW), oldpage);
 
-	if (PageTransHuge(oldpage)) {
-		nr_pages <<= compound_order(oldpage);
-		VM_BUG_ON_PAGE(!PageTransHuge(oldpage), oldpage);
-		VM_BUG_ON_PAGE(!PageTransHuge(newpage), newpage);
-	}
-
 	if (lrucare)
 		lock_page_lru(oldpage, &isolated);
 
@@ -6691,12 +6681,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
 	if (lrucare)
 		unlock_page_lru(oldpage, isolated);
 
-	local_irq_disable();
-	mem_cgroup_charge_statistics(pc->mem_cgroup, oldpage, -nr_pages);
-	memcg_check_events(pc->mem_cgroup, oldpage);
-	local_irq_enable();
-
-	commit_charge(newpage, pc->mem_cgroup, nr_pages, lrucare);
+	commit_charge(newpage, pc->mem_cgroup, lrucare);
 }
 
 /*
-- 
2.0.3

Re: [patch] mm: memcontrol: avoid charge statistics churn during page migration

[Michal Hocko]

On Mon 04-08-14 16:34:29, Johannes Weiner wrote:
> Charge migration currently disables IRQs twice to update the charge
> statistics for the old page and then again for the new page.
> 
> But migration is a seemless transition of a charge from one physical
> page to another one of the same size, so this should be a non-event
> from an accounting point of view.  Leave the statistics alone.

Moving stats to mem_cgroup_commit_charge sounds logical to me but does
this work properly even for the fuse replace page cache case when old
and new pages can already live in different memcgs?

> Signed-off-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> ---
>  mm/memcontrol.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 475ecadd9646..8d65dadeec1b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2728,7 +2728,7 @@ static void unlock_page_lru(struct page *page, int isolated)
>  }
>  
>  static void commit_charge(struct page *page, struct mem_cgroup *memcg,
> -			  unsigned int nr_pages, bool lrucare)
> +			  bool lrucare)
>  {
>  	struct page_cgroup *pc = lookup_page_cgroup(page);
>  	int isolated;
> @@ -2765,16 +2765,6 @@ static void commit_charge(struct page *page, struct mem_cgroup *memcg,
>  
>  	if (lrucare)
>  		unlock_page_lru(page, isolated);
> -
> -	local_irq_disable();
> -	mem_cgroup_charge_statistics(memcg, page, nr_pages);
> -	/*
> -	 * "charge_statistics" updated event counter. Then, check it.
> -	 * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
> -	 * if they exceeds softlimit.
> -	 */
> -	memcg_check_events(memcg, page);
> -	local_irq_enable();
>  }
>  
>  static DEFINE_MUTEX(set_limit_mutex);
> @@ -6460,12 +6450,17 @@ void mem_cgroup_commit_charge(struct page *page, struct mem_cgroup *memcg,
>  	if (!memcg)
>  		return;
>  
> +	commit_charge(page, memcg, lrucare);
> +
>  	if (PageTransHuge(page)) {
>  		nr_pages <<= compound_order(page);
>  		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
>  	}
>  
> -	commit_charge(page, memcg, nr_pages, lrucare);
> +	local_irq_disable();
> +	mem_cgroup_charge_statistics(memcg, page, nr_pages);
> +	memcg_check_events(memcg, page);
> +	local_irq_enable();
>  
>  	if (do_swap_account && PageSwapCache(page)) {
>  		swp_entry_t entry = { .val = page_private(page) };
> @@ -6651,7 +6646,6 @@ void mem_cgroup_uncharge_list(struct list_head *page_list)
>  void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  			bool lrucare)
>  {
> -	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
>  	int isolated;
>  
> @@ -6660,6 +6654,8 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  	VM_BUG_ON_PAGE(!lrucare && PageLRU(oldpage), oldpage);
>  	VM_BUG_ON_PAGE(!lrucare && PageLRU(newpage), newpage);
>  	VM_BUG_ON_PAGE(PageAnon(oldpage) != PageAnon(newpage), newpage);
> +	VM_BUG_ON_PAGE(PageTransHuge(oldpage) != PageTransHuge(newpage),
> +		       newpage);
>  
>  	if (mem_cgroup_disabled())
>  		return;
> @@ -6677,12 +6673,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  	VM_BUG_ON_PAGE(!(pc->flags & PCG_MEM), oldpage);
>  	VM_BUG_ON_PAGE(do_swap_account && !(pc->flags & PCG_MEMSW), oldpage);
>  
> -	if (PageTransHuge(oldpage)) {
> -		nr_pages <<= compound_order(oldpage);
> -		VM_BUG_ON_PAGE(!PageTransHuge(oldpage), oldpage);
> -		VM_BUG_ON_PAGE(!PageTransHuge(newpage), newpage);
> -	}
> -
>  	if (lrucare)
>  		lock_page_lru(oldpage, &isolated);
>  
> @@ -6691,12 +6681,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
>  	if (lrucare)
>  		unlock_page_lru(oldpage, isolated);
>  
> -	local_irq_disable();
> -	mem_cgroup_charge_statistics(pc->mem_cgroup, oldpage, -nr_pages);
> -	memcg_check_events(pc->mem_cgroup, oldpage);
> -	local_irq_enable();
> -
> -	commit_charge(newpage, pc->mem_cgroup, nr_pages, lrucare);
> +	commit_charge(newpage, pc->mem_cgroup, lrucare);
>  }
>  
>  /*
> -- 
> 2.0.3
> 

-- 
Michal Hocko
SUSE Labs

Re: [patch] mm: memcontrol: avoid charge statistics churn during page migration

[Johannes Weiner]

On Tue, Aug 05, 2014 at 02:24:34PM +0200, Michal Hocko wrote:
> On Mon 04-08-14 16:34:29, Johannes Weiner wrote:
> > Charge migration currently disables IRQs twice to update the charge
> > statistics for the old page and then again for the new page.
> > 
> > But migration is a seemless transition of a charge from one physical
> > page to another one of the same size, so this should be a non-event
> > from an accounting point of view.  Leave the statistics alone.
> 
> Moving stats to mem_cgroup_commit_charge sounds logical to me but does
> this work properly even for the fuse replace page cache case when old
> and new pages can already live in different memcgs?

We don't migrate if the new page is already charged.

Re: [patch] mm: memcontrol: avoid charge statistics churn during page migration

[Michal Hocko]

On Tue 05-08-14 09:12:23, Johannes Weiner wrote:
> On Tue, Aug 05, 2014 at 02:24:34PM +0200, Michal Hocko wrote:
> > On Mon 04-08-14 16:34:29, Johannes Weiner wrote:
> > > Charge migration currently disables IRQs twice to update the charge
> > > statistics for the old page and then again for the new page.
> > > 
> > > But migration is a seemless transition of a charge from one physical
> > > page to another one of the same size, so this should be a non-event
> > > from an accounting point of view.  Leave the statistics alone.
> > 
> > Moving stats to mem_cgroup_commit_charge sounds logical to me but does
> > this work properly even for the fuse replace page cache case when old
> > and new pages can already live in different memcgs?
> 
> We don't migrate if the new page is already charged.

Right you are.

Acked-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>

-- 
Michal Hocko
SUSE Labs