[PATCH] memcg: use for each online for making sum of percpu counter

[PATCH] memcg: use for each online for making sum of percpu counter

[KAMEZAWA Hiroyuki]

Tested on mmotm-2010-02-03.

Balbir-san, how about this patch ? It seems not so difficult as expected.

==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

memcg-use-for-each-online-cpus-for-making-sum-of-percpu-counter

Now, memcg's percpu coutner uses for_each_possible_cpus() for
handling cpu hotplug. But it adds some overhead on a server
which has an additonal cpu hotplug slot which is not used.

This patch adds cpu hotplug callback for memcg's percpu counter
and make use of for_each_online_cpu().

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/memcontrol.c |   35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Index: mmotm-2.6.33-Feb3/mm/memcontrol.c
===================================================================
--- mmotm-2.6.33-Feb3.orig/mm/memcontrol.c
+++ mmotm-2.6.33-Feb3/mm/memcontrol.c
@@ -223,6 +223,8 @@ struct mem_cgroup {
 	 */
 	unsigned long 	move_charge_at_immigrate;
 
+	/* list of all memcgs. currently used for cpu hotplug+percpu counter */
+	struct list_head list;
 	/*
 	 * percpu counter.
 	 */
@@ -504,7 +506,7 @@ static s64 mem_cgroup_read_stat(struct m
 	int cpu;
 	s64 val = 0;
 
-	for_each_possible_cpu(cpu)
+	for_each_online_cpu(cpu)
 		val += per_cpu(mem->stat->count[idx], cpu);
 	return val;
 }
@@ -1405,17 +1407,37 @@ static void drain_all_stock_sync(void)
 	atomic_dec(&memcg_drain_count);
 }
 
-static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
+DEFINE_MUTEX(memcg_hotcpu_lock);
+LIST_HEAD(memcg_hotcpu_list);
+
+static int __cpuinit memcg_cpu_unplug_callback(struct notifier_block *nb,
 					unsigned long action,
 					void *hcpu)
 {
 	int cpu = (unsigned long)hcpu;
+	struct mem_cgroup *memcg;
 	struct memcg_stock_pcp *stock;
+	int idx;
+	s64 val;
 
 	if (action != CPU_DEAD)
 		return NOTIFY_OK;
 	stock = &per_cpu(memcg_stock, cpu);
 	drain_stock(stock);
+
+	/* Move dead percpu counter's value to online cpu */
+	mutex_lock(&memcg_hotcpu_lock);
+	list_for_each_entry(memcg, &memcg_hotcpu_list, list) {
+		for (idx = MEM_CGROUP_STAT_CACHE;
+		     idx <= MEM_CGROUP_STAT_SWAPOUT;
+		     idx++) {
+			val = per_cpu(memcg->stat->count[idx], cpu);
+			per_cpu(memcg->stat->count[idx], cpu) = 0;
+			this_cpu_add(memcg->stat->count[idx], val);
+		}
+	}
+	mutex_unlock(&memcg_hotcpu_lock);
+
 	return NOTIFY_OK;
 }
 
@@ -3626,6 +3648,10 @@ static struct mem_cgroup *mem_cgroup_all
 		else
 			vfree(mem);
 		mem = NULL;
+	} else {
+		mutex_lock(&memcg_hotcpu_lock);
+		list_add(&mem->list, &memcg_hotcpu_list);
+		mutex_unlock(&memcg_hotcpu_lock);
 	}
 	return mem;
 }
@@ -3651,6 +3677,9 @@ static void __mem_cgroup_free(struct mem
 	for_each_node_state(node, N_POSSIBLE)
 		free_mem_cgroup_per_zone_info(mem, node);
 
+	mutex_lock(&memcg_hotcpu_lock);
+	list_del(&mem->list);
+	mutex_unlock(&memcg_hotcpu_lock);
 	free_percpu(mem->stat);
 	if (sizeof(struct mem_cgroup) < PAGE_SIZE)
 		kfree(mem);
@@ -3753,7 +3782,7 @@ mem_cgroup_create(struct cgroup_subsys *
 						&per_cpu(memcg_stock, cpu);
 			INIT_WORK(&stock->work, drain_local_stock);
 		}
-		hotcpu_notifier(memcg_stock_cpu_callback, 0);
+		hotcpu_notifier(memcg_cpu_unplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);
 		mem->use_hierarchy = parent->use_hierarchy;

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memcg: use for each online for making sum of percpu counter

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-02-04 14:36:45]:

> Tested on mmotm-2010-02-03.
> 
> Balbir-san, how about this patch ? It seems not so difficult as expected.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> memcg-use-for-each-online-cpus-for-making-sum-of-percpu-counter
> 
> Now, memcg's percpu coutner uses for_each_possible_cpus() for
> handling cpu hotplug. But it adds some overhead on a server
> which has an additonal cpu hotplug slot which is not used.
> 
> This patch adds cpu hotplug callback for memcg's percpu counter
> and make use of for_each_online_cpu().
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/memcontrol.c |   35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> Index: mmotm-2.6.33-Feb3/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.33-Feb3.orig/mm/memcontrol.c
> +++ mmotm-2.6.33-Feb3/mm/memcontrol.c
> @@ -223,6 +223,8 @@ struct mem_cgroup {
>  	 */
>  	unsigned long 	move_charge_at_immigrate;
> 
> +	/* list of all memcgs. currently used for cpu hotplug+percpu counter */
> +	struct list_head list;
>  	/*
>  	 * percpu counter.
>  	 */
> @@ -504,7 +506,7 @@ static s64 mem_cgroup_read_stat(struct m
>  	int cpu;
>  	s64 val = 0;
> 
> -	for_each_possible_cpu(cpu)
> +	for_each_online_cpu(cpu)
>  		val += per_cpu(mem->stat->count[idx], cpu);
>  	return val;
>  }
> @@ -1405,17 +1407,37 @@ static void drain_all_stock_sync(void)
>  	atomic_dec(&memcg_drain_count);
>  }
> 
> -static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
> +DEFINE_MUTEX(memcg_hotcpu_lock);
> +LIST_HEAD(memcg_hotcpu_list);
> +
> +static int __cpuinit memcg_cpu_unplug_callback(struct notifier_block *nb,
>  					unsigned long action,
>  					void *hcpu)
>  {
>  	int cpu = (unsigned long)hcpu;
> +	struct mem_cgroup *memcg;
>  	struct memcg_stock_pcp *stock;
> +	int idx;
> +	s64 val;
> 
>  	if (action != CPU_DEAD)
>  		return NOTIFY_OK;
>  	stock = &per_cpu(memcg_stock, cpu);
>  	drain_stock(stock);
> +
> +	/* Move dead percpu counter's value to online cpu */
> +	mutex_lock(&memcg_hotcpu_lock);
> +	list_for_each_entry(memcg, &memcg_hotcpu_list, list) {
> +		for (idx = MEM_CGROUP_STAT_CACHE;
> +		     idx <= MEM_CGROUP_STAT_SWAPOUT;
> +		     idx++) {

Should we add a for_each_stat_idx() macro?

> +			val = per_cpu(memcg->stat->count[idx], cpu);
> +			per_cpu(memcg->stat->count[idx], cpu) = 0;
> +			this_cpu_add(memcg->stat->count[idx], val);

So the CPU that deals with the hotplug notification moves the stats to
its own counter? Seems fair enough.

> +		}
> +	}
> +	mutex_unlock(&memcg_hotcpu_lock);
> +
>  	return NOTIFY_OK;
>  }
> 
> @@ -3626,6 +3648,10 @@ static struct mem_cgroup *mem_cgroup_all
>  		else
>  			vfree(mem);
>  		mem = NULL;
> +	} else {
> +		mutex_lock(&memcg_hotcpu_lock);
> +		list_add(&mem->list, &memcg_hotcpu_list);
> +		mutex_unlock(&memcg_hotcpu_lock);
>  	}
>  	return mem;
>  }
> @@ -3651,6 +3677,9 @@ static void __mem_cgroup_free(struct mem
>  	for_each_node_state(node, N_POSSIBLE)
>  		free_mem_cgroup_per_zone_info(mem, node);
> 
> +	mutex_lock(&memcg_hotcpu_lock);
> +	list_del(&mem->list);
> +	mutex_unlock(&memcg_hotcpu_lock);
>  	free_percpu(mem->stat);
>  	if (sizeof(struct mem_cgroup) < PAGE_SIZE)
>  		kfree(mem);
> @@ -3753,7 +3782,7 @@ mem_cgroup_create(struct cgroup_subsys *
>  						&per_cpu(memcg_stock, cpu);
>  			INIT_WORK(&stock->work, drain_local_stock);
>  		}
> -		hotcpu_notifier(memcg_stock_cpu_callback, 0);
> +		hotcpu_notifier(memcg_cpu_unplug_callback, 0);
>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		mem->use_hierarchy = parent->use_hierarchy;
>

Looks good, but I've not tested it yet. 

-- 
	Balbir

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memcg: use for each online for making sum of percpu counter

[KAMEZAWA Hiroyuki]

On Thu, 4 Feb 2010 13:57:43 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-02-04 14:36:45]:
> 
> > Tested on mmotm-2010-02-03.
> > 
> > Balbir-san, how about this patch ? It seems not so difficult as expected.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > memcg-use-for-each-online-cpus-for-making-sum-of-percpu-counter
> > 
> > Now, memcg's percpu coutner uses for_each_possible_cpus() for
> > handling cpu hotplug. But it adds some overhead on a server
> > which has an additonal cpu hotplug slot which is not used.
> > 
> > This patch adds cpu hotplug callback for memcg's percpu counter
> > and make use of for_each_online_cpu().
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  mm/memcontrol.c |   35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> > Index: mmotm-2.6.33-Feb3/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-2.6.33-Feb3.orig/mm/memcontrol.c
> > +++ mmotm-2.6.33-Feb3/mm/memcontrol.c
> > @@ -223,6 +223,8 @@ struct mem_cgroup {
> >  	 */
> >  	unsigned long 	move_charge_at_immigrate;
> > 
> > +	/* list of all memcgs. currently used for cpu hotplug+percpu counter */
> > +	struct list_head list;
> >  	/*
> >  	 * percpu counter.
> >  	 */
> > @@ -504,7 +506,7 @@ static s64 mem_cgroup_read_stat(struct m
> >  	int cpu;
> >  	s64 val = 0;
> > 
> > -	for_each_possible_cpu(cpu)
> > +	for_each_online_cpu(cpu)
> >  		val += per_cpu(mem->stat->count[idx], cpu);
> >  	return val;
> >  }
> > @@ -1405,17 +1407,37 @@ static void drain_all_stock_sync(void)
> >  	atomic_dec(&memcg_drain_count);
> >  }
> > 
> > -static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
> > +DEFINE_MUTEX(memcg_hotcpu_lock);
> > +LIST_HEAD(memcg_hotcpu_list);
> > +
> > +static int __cpuinit memcg_cpu_unplug_callback(struct notifier_block *nb,
> >  					unsigned long action,
> >  					void *hcpu)
> >  {
> >  	int cpu = (unsigned long)hcpu;
> > +	struct mem_cgroup *memcg;
> >  	struct memcg_stock_pcp *stock;
> > +	int idx;
> > +	s64 val;
> > 
> >  	if (action != CPU_DEAD)
> >  		return NOTIFY_OK;
> >  	stock = &per_cpu(memcg_stock, cpu);
> >  	drain_stock(stock);
> > +
> > +	/* Move dead percpu counter's value to online cpu */
> > +	mutex_lock(&memcg_hotcpu_lock);
> > +	list_for_each_entry(memcg, &memcg_hotcpu_list, list) {
> > +		for (idx = MEM_CGROUP_STAT_CACHE;
> > +		     idx <= MEM_CGROUP_STAT_SWAPOUT;
> > +		     idx++) {
> 
> Should we add a for_each_stat_idx() macro?
> 
I used bare codes because EVENT_COUNTERs are not target.


> > +			val = per_cpu(memcg->stat->count[idx], cpu);
> > +			per_cpu(memcg->stat->count[idx], cpu) = 0;
> > +			this_cpu_add(memcg->stat->count[idx], val);
> 
> So the CPU that deals with the hotplug notification moves the stats to
> its own counter? Seems fair enough.
> 
yes. I hope there will be no requests to get per-cpu statitstics...


> > +		}
> > +	}
> > +	mutex_unlock(&memcg_hotcpu_lock);
> > +
> >  	return NOTIFY_OK;
> >  }
> > 
> > @@ -3626,6 +3648,10 @@ static struct mem_cgroup *mem_cgroup_all
> >  		else
> >  			vfree(mem);
> >  		mem = NULL;
> > +	} else {
> > +		mutex_lock(&memcg_hotcpu_lock);
> > +		list_add(&mem->list, &memcg_hotcpu_list);
> > +		mutex_unlock(&memcg_hotcpu_lock);
> >  	}
> >  	return mem;
> >  }
> > @@ -3651,6 +3677,9 @@ static void __mem_cgroup_free(struct mem
> >  	for_each_node_state(node, N_POSSIBLE)
> >  		free_mem_cgroup_per_zone_info(mem, node);
> > 
> > +	mutex_lock(&memcg_hotcpu_lock);
> > +	list_del(&mem->list);
> > +	mutex_unlock(&memcg_hotcpu_lock);
> >  	free_percpu(mem->stat);
> >  	if (sizeof(struct mem_cgroup) < PAGE_SIZE)
> >  		kfree(mem);
> > @@ -3753,7 +3782,7 @@ mem_cgroup_create(struct cgroup_subsys *
> >  						&per_cpu(memcg_stock, cpu);
> >  			INIT_WORK(&stock->work, drain_local_stock);
> >  		}
> > -		hotcpu_notifier(memcg_stock_cpu_callback, 0);
> > +		hotcpu_notifier(memcg_cpu_unplug_callback, 0);
> >  	} else {
> >  		parent = mem_cgroup_from_cont(cont->parent);
> >  		mem->use_hierarchy = parent->use_hierarchy;
> >
> 
> Looks good, but I've not tested it yet. 
> 
Ok, I just did small test of online/offline cpus.

Thanks,
-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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>