[PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.

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

Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
It's just because cpu hotplug handler doesn't handle them.
On the other hand, per-cpu usage counter cache is maintained per cpu and
it's cpu hotplug aware.

This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
with for_each_online_cpu(). And this merges new callbacks with old
callbacks.(IOW, memcg has only one cpu-hotplug handler.)

For this purpose, mem_cgroup_walk_all() is added.

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

Index: mmotm-0915/mm/memcontrol.c
===================================================================
--- mmotm-0915.orig/mm/memcontrol.c
+++ mmotm-0915/mm/memcontrol.c
@@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
 	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
 	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
 	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
-	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
+	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
+
+	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
+				/* incremented at every  pagein/pageout */
 	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
 
 	MEM_CGROUP_STAT_NSTATS,
@@ -537,7 +540,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;
 }
@@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
 	return (mem == root_mem_cgroup);
 }
 
+static int mem_cgroup_walk_all(void *data,
+		int (*func)(struct mem_cgroup *, void *))
+{
+	int found, ret, nextid;
+	struct cgroup_subsys_state *css;
+	struct mem_cgroup *mem;
+
+	nextid = 1;
+	do {
+		ret = 0;
+		mem = NULL;
+
+		rcu_read_lock();
+		css = css_get_next(&mem_cgroup_subsys, nextid,
+				&root_mem_cgroup->css, &found);
+		if (css && css_tryget(css))
+			mem = container_of(css, struct mem_cgroup, css);
+		rcu_read_unlock();
+
+		if (mem) {
+			ret = (*func)(mem, data);
+			css_put(&mem->css);
+		}
+		nextid = found + 1;
+	} while (!ret && css);
+
+	return ret;
+}
+
 /*
  * Following LRU functions are allowed to be used without PCG_LOCK.
  * Operations are called by routine of global LRU independently from memcg.
@@ -1056,11 +1088,12 @@ static void mem_cgroup_start_move(struct
 {
 	int cpu;
 	/* Because this is for moving account, reuse mc.lock */
+	get_online_cpus();
 	spin_lock(&mc.lock);
-	for_each_possible_cpu(cpu)
+	for_each_online_cpu(cpu)
 		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) += 1;
 	spin_unlock(&mc.lock);
-
+	put_online_cpus();
 	synchronize_rcu();
 }
 
@@ -1070,10 +1103,12 @@ static void mem_cgroup_end_move(struct m
 
 	if (!mem)
 		return;
+	get_online_cpus();
 	spin_lock(&mc.lock);
-	for_each_possible_cpu(cpu)
+	for_each_online_cpu(cpu)
 		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) -= 1;
 	spin_unlock(&mc.lock);
+	put_online_cpus();
 }
 /*
  * 2 routines for checking "mem" is under move_account() or not.
@@ -1673,20 +1708,6 @@ static void drain_all_stock_sync(void)
 	atomic_dec(&memcg_drain_count);
 }
 
-static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
-					unsigned long action,
-					void *hcpu)
-{
-	int cpu = (unsigned long)hcpu;
-	struct memcg_stock_pcp *stock;
-
-	if (action != CPU_DEAD)
-		return NOTIFY_OK;
-	stock = &per_cpu(memcg_stock, cpu);
-	drain_stock(stock);
-	return NOTIFY_OK;
-}
-
 
 /* See __mem_cgroup_try_charge() for details */
 enum {
@@ -3465,6 +3486,7 @@ static int mem_cgroup_get_local_stat(str
 	s64 val;
 
 	/* per cpu stat */
+	get_online_cpus();
 	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_CACHE);
 	s->stat[MCS_CACHE] += val * PAGE_SIZE;
 	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_RSS);
@@ -3479,6 +3501,7 @@ static int mem_cgroup_get_local_stat(str
 		val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_SWAPOUT);
 		s->stat[MCS_SWAP] += val * PAGE_SIZE;
 	}
+	put_online_cpus();
 
 	/* per zone stat */
 	val = mem_cgroup_get_local_zonestat(mem, LRU_INACTIVE_ANON);
@@ -3508,7 +3531,9 @@ static int mem_control_stat_show(struct 
 	int i;
 
 	memset(&mystat, 0, sizeof(mystat));
+	get_online_cpus();
 	mem_cgroup_get_local_stat(mem_cont, &mystat);
+	put_online_cpus();
 
 	for (i = 0; i < NR_MCS_STAT; i++) {
 		if (i == MCS_SWAP && !do_swap_account)
@@ -3526,7 +3551,9 @@ static int mem_control_stat_show(struct 
 	}
 
 	memset(&mystat, 0, sizeof(mystat));
+	get_online_cpus();
 	mem_cgroup_get_total_stat(mem_cont, &mystat);
+	put_online_cpus();
 	for (i = 0; i < NR_MCS_STAT; i++) {
 		if (i == MCS_SWAP && !do_swap_account)
 			continue;
@@ -4036,6 +4063,57 @@ static int register_memsw_files(struct c
 }
 #endif
 
+/*
+ * CPU Hotplug handling.
+ */
+static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
+{
+	long cpu = (long)data;
+	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
+	/* All cpus should have the same value */
+	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
+	return 0;
+}
+
+static int drain_all_percpu(struct mem_cgroup *mem, void *data)
+{
+	long cpu = (long)(data);
+	int i;
+	/* Drain data from dying cpu and move to local cpu */
+	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
+		s64 data = per_cpu(mem->stat->count[i], cpu);
+		per_cpu(mem->stat->count[i], cpu) = 0;
+		this_cpu_add(mem->stat->count[i], data);
+	}
+	/* Reset Move Count */
+	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
+	return 0;
+}
+
+static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
+					unsigned long action,
+					void *hcpu)
+{
+	long cpu = (unsigned long)hcpu;
+	struct memcg_stock_pcp *stock;
+
+	if (action == CPU_ONLINE) {
+		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
+		return NOTIFY_OK;
+	}
+	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
+		return NOTIFY_OK;
+
+	/* Drain counters...for all memcgs. */
+	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
+
+	/* Drain Cached resources */
+	stock = &per_cpu(memcg_stock, cpu);
+	drain_stock(stock);
+
+	return NOTIFY_OK;
+}
+
 static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
 {
 	struct mem_cgroup_per_node *pn;
@@ -4224,7 +4302,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_cpuhotplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);
 		mem->use_hierarchy = parent->use_hierarchy;

[PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.

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

Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
It's just because cpu hotplug handler doesn't handle them.
On the other hand, per-cpu usage counter cache is maintained per cpu and
it's cpu hotplug aware.

This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
with for_each_online_cpu(). And this merges new callbacks with old
callbacks.(IOW, memcg has only one cpu-hotplug handler.)

For this purpose, mem_cgroup_walk_all() is added.

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

Index: mmotm-0915/mm/memcontrol.c
===================================================================
--- mmotm-0915.orig/mm/memcontrol.c
+++ mmotm-0915/mm/memcontrol.c
@@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
 	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
 	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
 	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
-	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
+	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
+
+	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
+				/* incremented at every  pagein/pageout */
 	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
 
 	MEM_CGROUP_STAT_NSTATS,
@@ -537,7 +540,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;
 }
@@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
 	return (mem == root_mem_cgroup);
 }
 
+static int mem_cgroup_walk_all(void *data,
+		int (*func)(struct mem_cgroup *, void *))
+{
+	int found, ret, nextid;
+	struct cgroup_subsys_state *css;
+	struct mem_cgroup *mem;
+
+	nextid = 1;
+	do {
+		ret = 0;
+		mem = NULL;
+
+		rcu_read_lock();
+		css = css_get_next(&mem_cgroup_subsys, nextid,
+				&root_mem_cgroup->css, &found);
+		if (css && css_tryget(css))
+			mem = container_of(css, struct mem_cgroup, css);
+		rcu_read_unlock();
+
+		if (mem) {
+			ret = (*func)(mem, data);
+			css_put(&mem->css);
+		}
+		nextid = found + 1;
+	} while (!ret && css);
+
+	return ret;
+}
+
 /*
  * Following LRU functions are allowed to be used without PCG_LOCK.
  * Operations are called by routine of global LRU independently from memcg.
@@ -1056,11 +1088,12 @@ static void mem_cgroup_start_move(struct
 {
 	int cpu;
 	/* Because this is for moving account, reuse mc.lock */
+	get_online_cpus();
 	spin_lock(&mc.lock);
-	for_each_possible_cpu(cpu)
+	for_each_online_cpu(cpu)
 		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) += 1;
 	spin_unlock(&mc.lock);
-
+	put_online_cpus();
 	synchronize_rcu();
 }
 
@@ -1070,10 +1103,12 @@ static void mem_cgroup_end_move(struct m
 
 	if (!mem)
 		return;
+	get_online_cpus();
 	spin_lock(&mc.lock);
-	for_each_possible_cpu(cpu)
+	for_each_online_cpu(cpu)
 		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) -= 1;
 	spin_unlock(&mc.lock);
+	put_online_cpus();
 }
 /*
  * 2 routines for checking "mem" is under move_account() or not.
@@ -1673,20 +1708,6 @@ static void drain_all_stock_sync(void)
 	atomic_dec(&memcg_drain_count);
 }
 
-static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
-					unsigned long action,
-					void *hcpu)
-{
-	int cpu = (unsigned long)hcpu;
-	struct memcg_stock_pcp *stock;
-
-	if (action != CPU_DEAD)
-		return NOTIFY_OK;
-	stock = &per_cpu(memcg_stock, cpu);
-	drain_stock(stock);
-	return NOTIFY_OK;
-}
-
 
 /* See __mem_cgroup_try_charge() for details */
 enum {
@@ -3465,6 +3486,7 @@ static int mem_cgroup_get_local_stat(str
 	s64 val;
 
 	/* per cpu stat */
+	get_online_cpus();
 	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_CACHE);
 	s->stat[MCS_CACHE] += val * PAGE_SIZE;
 	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_RSS);
@@ -3479,6 +3501,7 @@ static int mem_cgroup_get_local_stat(str
 		val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_SWAPOUT);
 		s->stat[MCS_SWAP] += val * PAGE_SIZE;
 	}
+	put_online_cpus();
 
 	/* per zone stat */
 	val = mem_cgroup_get_local_zonestat(mem, LRU_INACTIVE_ANON);
@@ -3508,7 +3531,9 @@ static int mem_control_stat_show(struct 
 	int i;
 
 	memset(&mystat, 0, sizeof(mystat));
+	get_online_cpus();
 	mem_cgroup_get_local_stat(mem_cont, &mystat);
+	put_online_cpus();
 
 	for (i = 0; i < NR_MCS_STAT; i++) {
 		if (i == MCS_SWAP && !do_swap_account)
@@ -3526,7 +3551,9 @@ static int mem_control_stat_show(struct 
 	}
 
 	memset(&mystat, 0, sizeof(mystat));
+	get_online_cpus();
 	mem_cgroup_get_total_stat(mem_cont, &mystat);
+	put_online_cpus();
 	for (i = 0; i < NR_MCS_STAT; i++) {
 		if (i == MCS_SWAP && !do_swap_account)
 			continue;
@@ -4036,6 +4063,57 @@ static int register_memsw_files(struct c
 }
 #endif
 
+/*
+ * CPU Hotplug handling.
+ */
+static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
+{
+	long cpu = (long)data;
+	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
+	/* All cpus should have the same value */
+	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
+	return 0;
+}
+
+static int drain_all_percpu(struct mem_cgroup *mem, void *data)
+{
+	long cpu = (long)(data);
+	int i;
+	/* Drain data from dying cpu and move to local cpu */
+	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
+		s64 data = per_cpu(mem->stat->count[i], cpu);
+		per_cpu(mem->stat->count[i], cpu) = 0;
+		this_cpu_add(mem->stat->count[i], data);
+	}
+	/* Reset Move Count */
+	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
+	return 0;
+}
+
+static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
+					unsigned long action,
+					void *hcpu)
+{
+	long cpu = (unsigned long)hcpu;
+	struct memcg_stock_pcp *stock;
+
+	if (action == CPU_ONLINE) {
+		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
+		return NOTIFY_OK;
+	}
+	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
+		return NOTIFY_OK;
+
+	/* Drain counters...for all memcgs. */
+	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
+
+	/* Drain Cached resources */
+	stock = &per_cpu(memcg_stock, cpu);
+	drain_stock(stock);
+
+	return NOTIFY_OK;
+}
+
 static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
 {
 	struct mem_cgroup_per_node *pn;
@@ -4224,7 +4302,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_cpuhotplug_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][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 14:46:18]:

> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> It's just because cpu hotplug handler doesn't handle them.
> On the other hand, per-cpu usage counter cache is maintained per cpu and
> it's cpu hotplug aware.
> 
> This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> with for_each_online_cpu(). And this merges new callbacks with old
> callbacks.(IOW, memcg has only one cpu-hotplug handler.)
>

Thanks for accepting my suggestion on get_online_cpus() and for
working on these patches, this is the right way forward
 
> For this purpose, mem_cgroup_walk_all() is added.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/memcontrol.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 98 insertions(+), 20 deletions(-)
> 
> Index: mmotm-0915/mm/memcontrol.c
> ===================================================================
> --- mmotm-0915.orig/mm/memcontrol.c
> +++ mmotm-0915/mm/memcontrol.c
> @@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
>  	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
>  	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
>  	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
> -	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
> +	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
> +
> +	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
> +				/* incremented at every  pagein/pageout */
>  	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
> 
>  	MEM_CGROUP_STAT_NSTATS,
> @@ -537,7 +540,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;
>  }
> @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
>  	return (mem == root_mem_cgroup);
>  }
> 
> +static int mem_cgroup_walk_all(void *data,
> +		int (*func)(struct mem_cgroup *, void *))

Can we call this for_each_mem_cgroup()?

> +{
> +	int found, ret, nextid;
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *mem;
> +
> +	nextid = 1;
> +	do {
> +		ret = 0;
> +		mem = NULL;
> +
> +		rcu_read_lock();
> +		css = css_get_next(&mem_cgroup_subsys, nextid,
> +				&root_mem_cgroup->css, &found);
> +		if (css && css_tryget(css))
> +			mem = container_of(css, struct mem_cgroup, css);
> +		rcu_read_unlock();
> +
> +		if (mem) {
> +			ret = (*func)(mem, data);
> +			css_put(&mem->css);
> +		}
> +		nextid = found + 1;
> +	} while (!ret && css);
> +
> +	return ret;
> +}
> +
>  /*
>   * Following LRU functions are allowed to be used without PCG_LOCK.
>   * Operations are called by routine of global LRU independently from memcg.
> @@ -1056,11 +1088,12 @@ static void mem_cgroup_start_move(struct
>  {
>  	int cpu;
>  	/* Because this is for moving account, reuse mc.lock */
> +	get_online_cpus();
>  	spin_lock(&mc.lock);
> -	for_each_possible_cpu(cpu)
> +	for_each_online_cpu(cpu)
>  		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) += 1;
>  	spin_unlock(&mc.lock);
> -
> +	put_online_cpus();
>  	synchronize_rcu();
>  }
> 
> @@ -1070,10 +1103,12 @@ static void mem_cgroup_end_move(struct m
> 
>  	if (!mem)
>  		return;
> +	get_online_cpus();
>  	spin_lock(&mc.lock);
> -	for_each_possible_cpu(cpu)
> +	for_each_online_cpu(cpu)
>  		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) -= 1;
>  	spin_unlock(&mc.lock);
> +	put_online_cpus();
>  }
>  /*
>   * 2 routines for checking "mem" is under move_account() or not.
> @@ -1673,20 +1708,6 @@ static void drain_all_stock_sync(void)
>  	atomic_dec(&memcg_drain_count);
>  }
> 
> -static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
> -					unsigned long action,
> -					void *hcpu)
> -{
> -	int cpu = (unsigned long)hcpu;
> -	struct memcg_stock_pcp *stock;
> -
> -	if (action != CPU_DEAD)
> -		return NOTIFY_OK;
> -	stock = &per_cpu(memcg_stock, cpu);
> -	drain_stock(stock);
> -	return NOTIFY_OK;
> -}
> -
> 
>  /* See __mem_cgroup_try_charge() for details */
>  enum {
> @@ -3465,6 +3486,7 @@ static int mem_cgroup_get_local_stat(str
>  	s64 val;
> 
>  	/* per cpu stat */
> +	get_online_cpus();
>  	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_CACHE);
>  	s->stat[MCS_CACHE] += val * PAGE_SIZE;
>  	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_RSS);
> @@ -3479,6 +3501,7 @@ static int mem_cgroup_get_local_stat(str
>  		val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_SWAPOUT);
>  		s->stat[MCS_SWAP] += val * PAGE_SIZE;
>  	}
> +	put_online_cpus();
> 
>  	/* per zone stat */
>  	val = mem_cgroup_get_local_zonestat(mem, LRU_INACTIVE_ANON);
> @@ -3508,7 +3531,9 @@ static int mem_control_stat_show(struct 
>  	int i;
> 
>  	memset(&mystat, 0, sizeof(mystat));
> +	get_online_cpus();
>  	mem_cgroup_get_local_stat(mem_cont, &mystat);
> +	put_online_cpus();
> 
>  	for (i = 0; i < NR_MCS_STAT; i++) {
>  		if (i == MCS_SWAP && !do_swap_account)
> @@ -3526,7 +3551,9 @@ static int mem_control_stat_show(struct 
>  	}
> 
>  	memset(&mystat, 0, sizeof(mystat));
> +	get_online_cpus();
>  	mem_cgroup_get_total_stat(mem_cont, &mystat);
> +	put_online_cpus();
>  	for (i = 0; i < NR_MCS_STAT; i++) {
>  		if (i == MCS_SWAP && !do_swap_account)
>  			continue;
> @@ -4036,6 +4063,57 @@ static int register_memsw_files(struct c
>  }
>  #endif
> 
> +/*
> + * CPU Hotplug handling.
> + */
> +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)data;
> +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> +	/* All cpus should have the same value */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> +	return 0;
> +}
> +
> +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)(data);
> +	int i;
> +	/* Drain data from dying cpu and move to local cpu */
> +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> +		s64 data = per_cpu(mem->stat->count[i], cpu);
> +		per_cpu(mem->stat->count[i], cpu) = 0;
> +		this_cpu_add(mem->stat->count[i], data);
> +	}
> +	/* Reset Move Count */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> +	return 0;
> +}
> +
> +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> +					unsigned long action,
> +					void *hcpu)
> +{
> +	long cpu = (unsigned long)hcpu;
> +	struct memcg_stock_pcp *stock;
> +
> +	if (action == CPU_ONLINE) {
> +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
> +		return NOTIFY_OK;
> +	}
> +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> +		return NOTIFY_OK;
> +
> +	/* Drain counters...for all memcgs. */
> +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> +
> +	/* Drain Cached resources */
> +	stock = &per_cpu(memcg_stock, cpu);
> +	drain_stock(stock);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
> @@ -4224,7 +4302,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_cpuhotplug_callback, 0);
>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		mem->use_hierarchy = parent->use_hierarchy;
> 

-- 
	Three Cheers,
	Balbir

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 14:46:18]:

> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> It's just because cpu hotplug handler doesn't handle them.
> On the other hand, per-cpu usage counter cache is maintained per cpu and
> it's cpu hotplug aware.
> 
> This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> with for_each_online_cpu(). And this merges new callbacks with old
> callbacks.(IOW, memcg has only one cpu-hotplug handler.)
>

Thanks for accepting my suggestion on get_online_cpus() and for
working on these patches, this is the right way forward
 
> For this purpose, mem_cgroup_walk_all() is added.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/memcontrol.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 98 insertions(+), 20 deletions(-)
> 
> Index: mmotm-0915/mm/memcontrol.c
> ===================================================================
> --- mmotm-0915.orig/mm/memcontrol.c
> +++ mmotm-0915/mm/memcontrol.c
> @@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
>  	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
>  	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
>  	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
> -	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
> +	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
> +
> +	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
> +				/* incremented at every  pagein/pageout */
>  	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
> 
>  	MEM_CGROUP_STAT_NSTATS,
> @@ -537,7 +540,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;
>  }
> @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
>  	return (mem == root_mem_cgroup);
>  }
> 
> +static int mem_cgroup_walk_all(void *data,
> +		int (*func)(struct mem_cgroup *, void *))

Can we call this for_each_mem_cgroup()?

> +{
> +	int found, ret, nextid;
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *mem;
> +
> +	nextid = 1;
> +	do {
> +		ret = 0;
> +		mem = NULL;
> +
> +		rcu_read_lock();
> +		css = css_get_next(&mem_cgroup_subsys, nextid,
> +				&root_mem_cgroup->css, &found);
> +		if (css && css_tryget(css))
> +			mem = container_of(css, struct mem_cgroup, css);
> +		rcu_read_unlock();
> +
> +		if (mem) {
> +			ret = (*func)(mem, data);
> +			css_put(&mem->css);
> +		}
> +		nextid = found + 1;
> +	} while (!ret && css);
> +
> +	return ret;
> +}
> +
>  /*
>   * Following LRU functions are allowed to be used without PCG_LOCK.
>   * Operations are called by routine of global LRU independently from memcg.
> @@ -1056,11 +1088,12 @@ static void mem_cgroup_start_move(struct
>  {
>  	int cpu;
>  	/* Because this is for moving account, reuse mc.lock */
> +	get_online_cpus();
>  	spin_lock(&mc.lock);
> -	for_each_possible_cpu(cpu)
> +	for_each_online_cpu(cpu)
>  		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) += 1;
>  	spin_unlock(&mc.lock);
> -
> +	put_online_cpus();
>  	synchronize_rcu();
>  }
> 
> @@ -1070,10 +1103,12 @@ static void mem_cgroup_end_move(struct m
> 
>  	if (!mem)
>  		return;
> +	get_online_cpus();
>  	spin_lock(&mc.lock);
> -	for_each_possible_cpu(cpu)
> +	for_each_online_cpu(cpu)
>  		per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) -= 1;
>  	spin_unlock(&mc.lock);
> +	put_online_cpus();
>  }
>  /*
>   * 2 routines for checking "mem" is under move_account() or not.
> @@ -1673,20 +1708,6 @@ static void drain_all_stock_sync(void)
>  	atomic_dec(&memcg_drain_count);
>  }
> 
> -static int __cpuinit memcg_stock_cpu_callback(struct notifier_block *nb,
> -					unsigned long action,
> -					void *hcpu)
> -{
> -	int cpu = (unsigned long)hcpu;
> -	struct memcg_stock_pcp *stock;
> -
> -	if (action != CPU_DEAD)
> -		return NOTIFY_OK;
> -	stock = &per_cpu(memcg_stock, cpu);
> -	drain_stock(stock);
> -	return NOTIFY_OK;
> -}
> -
> 
>  /* See __mem_cgroup_try_charge() for details */
>  enum {
> @@ -3465,6 +3486,7 @@ static int mem_cgroup_get_local_stat(str
>  	s64 val;
> 
>  	/* per cpu stat */
> +	get_online_cpus();
>  	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_CACHE);
>  	s->stat[MCS_CACHE] += val * PAGE_SIZE;
>  	val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_RSS);
> @@ -3479,6 +3501,7 @@ static int mem_cgroup_get_local_stat(str
>  		val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_SWAPOUT);
>  		s->stat[MCS_SWAP] += val * PAGE_SIZE;
>  	}
> +	put_online_cpus();
> 
>  	/* per zone stat */
>  	val = mem_cgroup_get_local_zonestat(mem, LRU_INACTIVE_ANON);
> @@ -3508,7 +3531,9 @@ static int mem_control_stat_show(struct 
>  	int i;
> 
>  	memset(&mystat, 0, sizeof(mystat));
> +	get_online_cpus();
>  	mem_cgroup_get_local_stat(mem_cont, &mystat);
> +	put_online_cpus();
> 
>  	for (i = 0; i < NR_MCS_STAT; i++) {
>  		if (i == MCS_SWAP && !do_swap_account)
> @@ -3526,7 +3551,9 @@ static int mem_control_stat_show(struct 
>  	}
> 
>  	memset(&mystat, 0, sizeof(mystat));
> +	get_online_cpus();
>  	mem_cgroup_get_total_stat(mem_cont, &mystat);
> +	put_online_cpus();
>  	for (i = 0; i < NR_MCS_STAT; i++) {
>  		if (i == MCS_SWAP && !do_swap_account)
>  			continue;
> @@ -4036,6 +4063,57 @@ static int register_memsw_files(struct c
>  }
>  #endif
> 
> +/*
> + * CPU Hotplug handling.
> + */
> +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)data;
> +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> +	/* All cpus should have the same value */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> +	return 0;
> +}
> +
> +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)(data);
> +	int i;
> +	/* Drain data from dying cpu and move to local cpu */
> +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> +		s64 data = per_cpu(mem->stat->count[i], cpu);
> +		per_cpu(mem->stat->count[i], cpu) = 0;
> +		this_cpu_add(mem->stat->count[i], data);
> +	}
> +	/* Reset Move Count */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> +	return 0;
> +}
> +
> +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> +					unsigned long action,
> +					void *hcpu)
> +{
> +	long cpu = (unsigned long)hcpu;
> +	struct memcg_stock_pcp *stock;
> +
> +	if (action == CPU_ONLINE) {
> +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
> +		return NOTIFY_OK;
> +	}
> +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> +		return NOTIFY_OK;
> +
> +	/* Drain counters...for all memcgs. */
> +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> +
> +	/* Drain Cached resources */
> +	stock = &per_cpu(memcg_stock, cpu);
> +	drain_stock(stock);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
> @@ -4224,7 +4302,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_cpuhotplug_callback, 0);
>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		mem->use_hierarchy = parent->use_hierarchy;
> 

-- 
	Three Cheers,
	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][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 11:51:59 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 14:46:18]:
> 
> > This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> > It's just because cpu hotplug handler doesn't handle them.
> > On the other hand, per-cpu usage counter cache is maintained per cpu and
> > it's cpu hotplug aware.
> > 
> > This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> > with for_each_online_cpu(). And this merges new callbacks with old
> > callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> >
> 
> Thanks for accepting my suggestion on get_online_cpus() and for
> working on these patches, this is the right way forward
>  
I just like step-by-step patches.


> > For this purpose, mem_cgroup_walk_all() is added.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  mm/memcontrol.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 98 insertions(+), 20 deletions(-)
> > 
> > Index: mmotm-0915/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-0915.orig/mm/memcontrol.c
> > +++ mmotm-0915/mm/memcontrol.c
> > @@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
> >  	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
> >  	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
> >  	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
> > -	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
> > +	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
> > +
> > +	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
> > +				/* incremented at every  pagein/pageout */
> >  	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
> > 
> >  	MEM_CGROUP_STAT_NSTATS,
> > @@ -537,7 +540,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;
> >  }
> > @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
> >  	return (mem == root_mem_cgroup);
> >  }
> > 
> > +static int mem_cgroup_walk_all(void *data,
> > +		int (*func)(struct mem_cgroup *, void *))
> 
> Can we call this for_each_mem_cgroup()?
> 

This naming is from mem_cgroup_walk_tree(). Now we have

  mem_cgroup_walk_tree();
  mem_cgroup_walk_all();

Rename both ? But it should be in separated patch.

Thanks,
-Kame

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 11:51:59 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 14:46:18]:
> 
> > This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> > It's just because cpu hotplug handler doesn't handle them.
> > On the other hand, per-cpu usage counter cache is maintained per cpu and
> > it's cpu hotplug aware.
> > 
> > This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> > with for_each_online_cpu(). And this merges new callbacks with old
> > callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> >
> 
> Thanks for accepting my suggestion on get_online_cpus() and for
> working on these patches, this is the right way forward
>  
I just like step-by-step patches.


> > For this purpose, mem_cgroup_walk_all() is added.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  mm/memcontrol.c |  118 ++++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 98 insertions(+), 20 deletions(-)
> > 
> > Index: mmotm-0915/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-0915.orig/mm/memcontrol.c
> > +++ mmotm-0915/mm/memcontrol.c
> > @@ -89,7 +89,10 @@ enum mem_cgroup_stat_index {
> >  	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
> >  	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
> >  	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
> > -	MEM_CGROUP_EVENTS,	/* incremented at every  pagein/pageout */
> > +	MEM_CGROUP_STAT_DATA,    /* stat above this is for statistics */
> > +
> > +	MEM_CGROUP_EVENTS = MEM_CGROUP_STAT_DATA,
> > +				/* incremented at every  pagein/pageout */
> >  	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
> > 
> >  	MEM_CGROUP_STAT_NSTATS,
> > @@ -537,7 +540,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;
> >  }
> > @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
> >  	return (mem == root_mem_cgroup);
> >  }
> > 
> > +static int mem_cgroup_walk_all(void *data,
> > +		int (*func)(struct mem_cgroup *, void *))
> 
> Can we call this for_each_mem_cgroup()?
> 

This naming is from mem_cgroup_walk_tree(). Now we have

  mem_cgroup_walk_tree();
  mem_cgroup_walk_all();

Rename both ? But it should be in separated patch.

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>

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 15:22:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
 
> This naming is from mem_cgroup_walk_tree(). Now we have
> 
>   mem_cgroup_walk_tree();
>   mem_cgroup_walk_all();
> 
> Rename both ? But it should be in separated patch.
> 

Considering a bit ...but..

#define for_each_mem_cgroup(mem) \
	for (mem = mem_cgroup_get_first(); \
	     mem; \
	     mem = mem_cgroup_get_next(mem);) \

seems to need some helper functions. I'll consider about this clean up
but it requires some amount of patch because css_get()/css_put()/rcu...etc..
are problematic.

Thanks,
-Kame

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 15:22:04 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
 
> This naming is from mem_cgroup_walk_tree(). Now we have
> 
>   mem_cgroup_walk_tree();
>   mem_cgroup_walk_all();
> 
> Rename both ? But it should be in separated patch.
> 

Considering a bit ...but..

#define for_each_mem_cgroup(mem) \
	for (mem = mem_cgroup_get_first(); \
	     mem; \
	     mem = mem_cgroup_get_next(mem);) \

seems to need some helper functions. I'll consider about this clean up
but it requires some amount of patch because css_get()/css_put()/rcu...etc..
are problematic.

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>

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 16:17:27 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 16 Sep 2010 15:22:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>  
> > This naming is from mem_cgroup_walk_tree(). Now we have
> > 
> >   mem_cgroup_walk_tree();
> >   mem_cgroup_walk_all();
> > 
> > Rename both ? But it should be in separated patch.
> > 
> 
> Considering a bit ...but..
> 
> #define for_each_mem_cgroup(mem) \
> 	for (mem = mem_cgroup_get_first(); \
> 	     mem; \
> 	     mem = mem_cgroup_get_next(mem);) \
> 
> seems to need some helper functions. I'll consider about this clean up
> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> are problematic.

Hmm...css_put() at break from loop is a problem...

Do you have anything good idea to handle "exit-from-loop" operation
with dropping reference count ? I don't like "the caller must take care of"
approach.

Thanks,
-Kame

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[KAMEZAWA Hiroyuki]

On Thu, 16 Sep 2010 16:17:27 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 16 Sep 2010 15:22:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>  
> > This naming is from mem_cgroup_walk_tree(). Now we have
> > 
> >   mem_cgroup_walk_tree();
> >   mem_cgroup_walk_all();
> > 
> > Rename both ? But it should be in separated patch.
> > 
> 
> Considering a bit ...but..
> 
> #define for_each_mem_cgroup(mem) \
> 	for (mem = mem_cgroup_get_first(); \
> 	     mem; \
> 	     mem = mem_cgroup_get_next(mem);) \
> 
> seems to need some helper functions. I'll consider about this clean up
> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> are problematic.

Hmm...css_put() at break from loop is a problem...

Do you have anything good idea to handle "exit-from-loop" operation
with dropping reference count ? I don't like "the caller must take care of"
approach.

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>

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Andrew Morton]

On Thu, 16 Sep 2010 14:46:18 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> It's just because cpu hotplug handler doesn't handle them.
> On the other hand, per-cpu usage counter cache is maintained per cpu and
> it's cpu hotplug aware.
> 
> This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> with for_each_online_cpu(). And this merges new callbacks with old
> callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> 
> For this purpose, mem_cgroup_walk_all() is added.
> 
> ...
> 
> @@ -537,7 +540,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);

Can someone remind me again why all this code couldn't use
percpu-counters?

>  	return val;
>  }
> @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
>  	return (mem == root_mem_cgroup);
>  }
>  
> +static int mem_cgroup_walk_all(void *data,
> +		int (*func)(struct mem_cgroup *, void *))
> +{
> +	int found, ret, nextid;
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *mem;
> +
> +	nextid = 1;
> +	do {
> +		ret = 0;
> +		mem = NULL;
> +
> +		rcu_read_lock();
> +		css = css_get_next(&mem_cgroup_subsys, nextid,
> +				&root_mem_cgroup->css, &found);
> +		if (css && css_tryget(css))
> +			mem = container_of(css, struct mem_cgroup, css);
> +		rcu_read_unlock();
> +
> +		if (mem) {
> +			ret = (*func)(mem, data);
> +			css_put(&mem->css);
> +		}
> +		nextid = found + 1;
> +	} while (!ret && css);
> +
> +	return ret;
> +}

It would be better to convert `void *data' to `unsigned cpu' within the
caller of this function rather than adding the typecast to each
function which this function calls.  So this becomes

static int mem_cgroup_walk_all(unsigned cpu,
		int (*func)(struct mem_cgroup *memcg, unsigned cpu))


> +/*
> + * CPU Hotplug handling.
> + */
> +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)data;
> +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> +	/* All cpus should have the same value */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> +	return 0;
> +}
> +
> +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)(data);
> +	int i;
> +	/* Drain data from dying cpu and move to local cpu */
> +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> +		s64 data = per_cpu(mem->stat->count[i], cpu);
> +		per_cpu(mem->stat->count[i], cpu) = 0;
> +		this_cpu_add(mem->stat->count[i], data);
> +	}
> +	/* Reset Move Count */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> +	return 0;
> +}

Some nice comments would be nice.

I don't immediately see anything which guarantees that preemption (and
cpu migration) are disabled here.  It would be an odd thing to permit
migration within a cpu-hotplug handler, but where did we guarantee it?

Also, the code appears to assume that the current CPU is the one which
is being onlined.  What guaranteed that?  This is not the case for
enable_nonboot_cpus().

It's conventional to put a blank line between end-of-locals and
start-of-code.  This patch ignored that convention rather a lot.

The comments in this patch Have Rather Strange Capitalisation Decisions.

> +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> +					unsigned long action,
> +					void *hcpu)
> +{
> +	long cpu = (unsigned long)hcpu;
> +	struct memcg_stock_pcp *stock;
> +
> +	if (action == CPU_ONLINE) {
> +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);

More typecasts which can go away if we make the above change to
mem_cgroup_walk_all().

> +		return NOTIFY_OK;
> +	}
> +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> +		return NOTIFY_OK;
> +
> +	/* Drain counters...for all memcgs. */
> +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> +
> +	/* Drain Cached resources */
> +	stock = &per_cpu(memcg_stock, cpu);
> +	drain_stock(stock);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
> @@ -4224,7 +4302,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_cpuhotplug_callback, 0);
>  	} else {
>  		parent = mem_cgroup_from_cont(cont->parent);
>  		mem->use_hierarchy = parent->use_hierarchy;

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Andrew Morton]

On Thu, 16 Sep 2010 14:46:18 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> It's just because cpu hotplug handler doesn't handle them.
> On the other hand, per-cpu usage counter cache is maintained per cpu and
> it's cpu hotplug aware.
> 
> This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> with for_each_online_cpu(). And this merges new callbacks with old
> callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> 
> For this purpose, mem_cgroup_walk_all() is added.
> 
> ...
> 
> @@ -537,7 +540,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);

Can someone remind me again why all this code couldn't use
percpu-counters?

>  	return val;
>  }
> @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
>  	return (mem == root_mem_cgroup);
>  }
>  
> +static int mem_cgroup_walk_all(void *data,
> +		int (*func)(struct mem_cgroup *, void *))
> +{
> +	int found, ret, nextid;
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *mem;
> +
> +	nextid = 1;
> +	do {
> +		ret = 0;
> +		mem = NULL;
> +
> +		rcu_read_lock();
> +		css = css_get_next(&mem_cgroup_subsys, nextid,
> +				&root_mem_cgroup->css, &found);
> +		if (css && css_tryget(css))
> +			mem = container_of(css, struct mem_cgroup, css);
> +		rcu_read_unlock();
> +
> +		if (mem) {
> +			ret = (*func)(mem, data);
> +			css_put(&mem->css);
> +		}
> +		nextid = found + 1;
> +	} while (!ret && css);
> +
> +	return ret;
> +}

It would be better to convert `void *data' to `unsigned cpu' within the
caller of this function rather than adding the typecast to each
function which this function calls.  So this becomes

static int mem_cgroup_walk_all(unsigned cpu,
		int (*func)(struct mem_cgroup *memcg, unsigned cpu))


> +/*
> + * CPU Hotplug handling.
> + */
> +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)data;
> +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> +	/* All cpus should have the same value */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> +	return 0;
> +}
> +
> +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> +{
> +	long cpu = (long)(data);
> +	int i;
> +	/* Drain data from dying cpu and move to local cpu */
> +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> +		s64 data = per_cpu(mem->stat->count[i], cpu);
> +		per_cpu(mem->stat->count[i], cpu) = 0;
> +		this_cpu_add(mem->stat->count[i], data);
> +	}
> +	/* Reset Move Count */
> +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> +	return 0;
> +}

Some nice comments would be nice.

I don't immediately see anything which guarantees that preemption (and
cpu migration) are disabled here.  It would be an odd thing to permit
migration within a cpu-hotplug handler, but where did we guarantee it?

Also, the code appears to assume that the current CPU is the one which
is being onlined.  What guaranteed that?  This is not the case for
enable_nonboot_cpus().

It's conventional to put a blank line between end-of-locals and
start-of-code.  This patch ignored that convention rather a lot.

The comments in this patch Have Rather Strange Capitalisation Decisions.

> +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> +					unsigned long action,
> +					void *hcpu)
> +{
> +	long cpu = (unsigned long)hcpu;
> +	struct memcg_stock_pcp *stock;
> +
> +	if (action == CPU_ONLINE) {
> +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);

More typecasts which can go away if we make the above change to
mem_cgroup_walk_all().

> +		return NOTIFY_OK;
> +	}
> +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> +		return NOTIFY_OK;
> +
> +	/* Drain counters...for all memcgs. */
> +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> +
> +	/* Drain Cached resources */
> +	stock = &per_cpu(memcg_stock, cpu);
> +	drain_stock(stock);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
> @@ -4224,7 +4302,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_cpuhotplug_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][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* Andrew Morton <akpm@linux-foundation.org> [2010-09-16 13:14:32]:

> On Thu, 16 Sep 2010 14:46:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> > It's just because cpu hotplug handler doesn't handle them.
> > On the other hand, per-cpu usage counter cache is maintained per cpu and
> > it's cpu hotplug aware.
> > 
> > This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> > with for_each_online_cpu(). And this merges new callbacks with old
> > callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> > 
> > For this purpose, mem_cgroup_walk_all() is added.
> > 
> > ...
> > 
> > @@ -537,7 +540,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);
> 
> Can someone remind me again why all this code couldn't use
> percpu-counters?
>

We use the same per_cpu data structure (almost an array of counters)
mem_cgroup_charge_statistics() illustrates using these counters
together under a single preempt disable.
 
> >  	return val;
> >  }
> > @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
> >  	return (mem == root_mem_cgroup);
> >  }
> >  
> > +static int mem_cgroup_walk_all(void *data,
> > +		int (*func)(struct mem_cgroup *, void *))
> > +{
> > +	int found, ret, nextid;
> > +	struct cgroup_subsys_state *css;
> > +	struct mem_cgroup *mem;
> > +
> > +	nextid = 1;
> > +	do {
> > +		ret = 0;
> > +		mem = NULL;
> > +
> > +		rcu_read_lock();
> > +		css = css_get_next(&mem_cgroup_subsys, nextid,
> > +				&root_mem_cgroup->css, &found);
> > +		if (css && css_tryget(css))
> > +			mem = container_of(css, struct mem_cgroup, css);
> > +		rcu_read_unlock();
> > +
> > +		if (mem) {
> > +			ret = (*func)(mem, data);
> > +			css_put(&mem->css);
> > +		}
> > +		nextid = found + 1;
> > +	} while (!ret && css);
> > +
> > +	return ret;
> > +}
> 
> It would be better to convert `void *data' to `unsigned cpu' within the
> caller of this function rather than adding the typecast to each
> function which this function calls.  So this becomes
> 
> static int mem_cgroup_walk_all(unsigned cpu,
> 		int (*func)(struct mem_cgroup *memcg, unsigned cpu))
> 

I think the goal was to keep this callback generic, we infact wanted
to call this function for_each_mem_cgroup()

> 
> > +/*
> > + * CPU Hotplug handling.
> > + */
> > +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> > +{
> > +	long cpu = (long)data;
> > +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> > +	/* All cpus should have the same value */
> > +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> > +	return 0;
> > +}
> > +
> > +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> > +{
> > +	long cpu = (long)(data);
> > +	int i;
> > +	/* Drain data from dying cpu and move to local cpu */
> > +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> > +		s64 data = per_cpu(mem->stat->count[i], cpu);
> > +		per_cpu(mem->stat->count[i], cpu) = 0;
> > +		this_cpu_add(mem->stat->count[i], data);
> > +	}
> > +	/* Reset Move Count */
> > +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> > +	return 0;
> > +}
> 
> Some nice comments would be nice.
> 
> I don't immediately see anything which guarantees that preemption (and
> cpu migration) are disabled here.  It would be an odd thing to permit
> migration within a cpu-hotplug handler, but where did we guarantee it?
> 
> Also, the code appears to assume that the current CPU is the one which
> is being onlined.  What guaranteed that?  This is not the case for
> enable_nonboot_cpus().
> 
> It's conventional to put a blank line between end-of-locals and
> start-of-code.  This patch ignored that convention rather a lot.
> 
> The comments in this patch Have Rather Strange Capitalisation Decisions.
> 
> > +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> > +					unsigned long action,
> > +					void *hcpu)
> > +{
> > +	long cpu = (unsigned long)hcpu;
> > +	struct memcg_stock_pcp *stock;
> > +
> > +	if (action == CPU_ONLINE) {
> > +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
> 
> More typecasts which can go away if we make the above change to
> mem_cgroup_walk_all().
> 
> > +		return NOTIFY_OK;
> > +	}
> > +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> > +		return NOTIFY_OK;
> > +
> > +	/* Drain counters...for all memcgs. */
> > +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> > +
> > +	/* Drain Cached resources */
> > +	stock = &per_cpu(memcg_stock, cpu);
> > +	drain_stock(stock);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> >  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
> >  {
> >  	struct mem_cgroup_per_node *pn;
> > @@ -4224,7 +4302,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_cpuhotplug_callback, 0);
> >  	} else {
> >  		parent = mem_cgroup_from_cont(cont->parent);
> >  		mem->use_hierarchy = parent->use_hierarchy;

-- 
	Three Cheers,
	Balbir

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* Andrew Morton <akpm@linux-foundation.org> [2010-09-16 13:14:32]:

> On Thu, 16 Sep 2010 14:46:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
> > It's just because cpu hotplug handler doesn't handle them.
> > On the other hand, per-cpu usage counter cache is maintained per cpu and
> > it's cpu hotplug aware.
> > 
> > This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
> > with for_each_online_cpu(). And this merges new callbacks with old
> > callbacks.(IOW, memcg has only one cpu-hotplug handler.)
> > 
> > For this purpose, mem_cgroup_walk_all() is added.
> > 
> > ...
> > 
> > @@ -537,7 +540,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);
> 
> Can someone remind me again why all this code couldn't use
> percpu-counters?
>

We use the same per_cpu data structure (almost an array of counters)
mem_cgroup_charge_statistics() illustrates using these counters
together under a single preempt disable.
 
> >  	return val;
> >  }
> > @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
> >  	return (mem == root_mem_cgroup);
> >  }
> >  
> > +static int mem_cgroup_walk_all(void *data,
> > +		int (*func)(struct mem_cgroup *, void *))
> > +{
> > +	int found, ret, nextid;
> > +	struct cgroup_subsys_state *css;
> > +	struct mem_cgroup *mem;
> > +
> > +	nextid = 1;
> > +	do {
> > +		ret = 0;
> > +		mem = NULL;
> > +
> > +		rcu_read_lock();
> > +		css = css_get_next(&mem_cgroup_subsys, nextid,
> > +				&root_mem_cgroup->css, &found);
> > +		if (css && css_tryget(css))
> > +			mem = container_of(css, struct mem_cgroup, css);
> > +		rcu_read_unlock();
> > +
> > +		if (mem) {
> > +			ret = (*func)(mem, data);
> > +			css_put(&mem->css);
> > +		}
> > +		nextid = found + 1;
> > +	} while (!ret && css);
> > +
> > +	return ret;
> > +}
> 
> It would be better to convert `void *data' to `unsigned cpu' within the
> caller of this function rather than adding the typecast to each
> function which this function calls.  So this becomes
> 
> static int mem_cgroup_walk_all(unsigned cpu,
> 		int (*func)(struct mem_cgroup *memcg, unsigned cpu))
> 

I think the goal was to keep this callback generic, we infact wanted
to call this function for_each_mem_cgroup()

> 
> > +/*
> > + * CPU Hotplug handling.
> > + */
> > +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
> > +{
> > +	long cpu = (long)data;
> > +	s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
> > +	/* All cpus should have the same value */
> > +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
> > +	return 0;
> > +}
> > +
> > +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
> > +{
> > +	long cpu = (long)(data);
> > +	int i;
> > +	/* Drain data from dying cpu and move to local cpu */
> > +	for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
> > +		s64 data = per_cpu(mem->stat->count[i], cpu);
> > +		per_cpu(mem->stat->count[i], cpu) = 0;
> > +		this_cpu_add(mem->stat->count[i], data);
> > +	}
> > +	/* Reset Move Count */
> > +	per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
> > +	return 0;
> > +}
> 
> Some nice comments would be nice.
> 
> I don't immediately see anything which guarantees that preemption (and
> cpu migration) are disabled here.  It would be an odd thing to permit
> migration within a cpu-hotplug handler, but where did we guarantee it?
> 
> Also, the code appears to assume that the current CPU is the one which
> is being onlined.  What guaranteed that?  This is not the case for
> enable_nonboot_cpus().
> 
> It's conventional to put a blank line between end-of-locals and
> start-of-code.  This patch ignored that convention rather a lot.
> 
> The comments in this patch Have Rather Strange Capitalisation Decisions.
> 
> > +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
> > +					unsigned long action,
> > +					void *hcpu)
> > +{
> > +	long cpu = (unsigned long)hcpu;
> > +	struct memcg_stock_pcp *stock;
> > +
> > +	if (action == CPU_ONLINE) {
> > +		mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
> 
> More typecasts which can go away if we make the above change to
> mem_cgroup_walk_all().
> 
> > +		return NOTIFY_OK;
> > +	}
> > +	if ((action != CPU_DEAD) || (action != CPU_DEAD_FROZEN))
> > +		return NOTIFY_OK;
> > +
> > +	/* Drain counters...for all memcgs. */
> > +	mem_cgroup_walk_all((void *)cpu, drain_all_percpu);
> > +
> > +	/* Drain Cached resources */
> > +	stock = &per_cpu(memcg_stock, cpu);
> > +	drain_stock(stock);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> >  static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *mem, int node)
> >  {
> >  	struct mem_cgroup_per_node *pn;
> > @@ -4224,7 +4302,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_cpuhotplug_callback, 0);
> >  	} else {
> >  		parent = mem_cgroup_from_cont(cont->parent);
> >  		mem->use_hierarchy = parent->use_hierarchy;

-- 
	Three Cheers,
	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][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:

> On Thu, 16 Sep 2010 15:22:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > This naming is from mem_cgroup_walk_tree(). Now we have
> > 
> >   mem_cgroup_walk_tree();
> >   mem_cgroup_walk_all();
> > 
> > Rename both ? But it should be in separated patch.
> > 
> 
> Considering a bit ...but..
> 
> #define for_each_mem_cgroup(mem) \
> 	for (mem = mem_cgroup_get_first(); \
> 	     mem; \
> 	     mem = mem_cgroup_get_next(mem);) \
> 
> seems to need some helper functions. I'll consider about this clean up
> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> are problematic.
>

Why does this need to be a macro (I know we use this for lists and
other places), assuming for now we don't use the iterator pattern, we
can rename mem_cgroup_walk_all() to for_each_mem_cgroup(). 

-- 
	Three Cheers,
	Balbir

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:

> On Thu, 16 Sep 2010 15:22:04 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > This naming is from mem_cgroup_walk_tree(). Now we have
> > 
> >   mem_cgroup_walk_tree();
> >   mem_cgroup_walk_all();
> > 
> > Rename both ? But it should be in separated patch.
> > 
> 
> Considering a bit ...but..
> 
> #define for_each_mem_cgroup(mem) \
> 	for (mem = mem_cgroup_get_first(); \
> 	     mem; \
> 	     mem = mem_cgroup_get_next(mem);) \
> 
> seems to need some helper functions. I'll consider about this clean up
> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> are problematic.
>

Why does this need to be a macro (I know we use this for lists and
other places), assuming for now we don't use the iterator pattern, we
can rename mem_cgroup_walk_all() to for_each_mem_cgroup(). 

-- 
	Three Cheers,
	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][-mm] memcg : memory cgroup cpu hotplug support update.

[Hiroyuki Kamezawa]

2010/9/17 Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 16 Sep 2010 14:46:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
>> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
>>
>> ==
>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling.
>> It's just because cpu hotplug handler doesn't handle them.
>> On the other hand, per-cpu usage counter cache is maintained per cpu and
>> it's cpu hotplug aware.
>>
>> This patch adds a cpu hotplug hanlder and replaces for_each_possible_cpu()
>> with for_each_online_cpu(). And this merges new callbacks with old
>> callbacks.(IOW, memcg has only one cpu-hotplug handler.)
>>
>> For this purpose, mem_cgroup_walk_all() is added.
>>
>> ...
>>
>> @@ -537,7 +540,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);
>
> Can someone remind me again why all this code couldn't use
> percpu-counters?
>

The design was based on vmstat[] and some other reasons.
IIUC,  it doesn't has good memory layout when it used as "array".

  spinlock
  counter
  list_head
  percpu pointer

This seems big and not cache friendly to me. I want a memory layout
like vmstat[].
If someone requests, I may able to write a patch of percpu_coutner_array.

And, percpu counter is used with core value + percpu value and does
synchronization
with some thresholds.

memcg's counter is used for 2 purposes as
 - counters #
 - per cpu event counter # don't need any synchronization.

Then, this is as it is now.

>>       return val;
>>  }
>> @@ -700,6 +703,35 @@ static inline bool mem_cgroup_is_root(st
>>       return (mem == root_mem_cgroup);
>>  }
>>
>> +static int mem_cgroup_walk_all(void *data,
>> +             int (*func)(struct mem_cgroup *, void *))
>> +{
>> +     int found, ret, nextid;
>> +     struct cgroup_subsys_state *css;
>> +     struct mem_cgroup *mem;
>> +
>> +     nextid = 1;
>> +     do {
>> +             ret = 0;
>> +             mem = NULL;
>> +
>> +             rcu_read_lock();
>> +             css = css_get_next(&mem_cgroup_subsys, nextid,
>> +                             &root_mem_cgroup->css, &found);
>> +             if (css && css_tryget(css))
>> +                     mem = container_of(css, struct mem_cgroup, css);
>> +             rcu_read_unlock();
>> +
>> +             if (mem) {
>> +                     ret = (*func)(mem, data);
>> +                     css_put(&mem->css);
>> +             }
>> +             nextid = found + 1;
>> +     } while (!ret && css);
>> +
>> +     return ret;
>> +}
>
> It would be better to convert `void *data' to `unsigned cpu' within the
> caller of this function rather than adding the typecast to each
> function which this function calls.  So this becomes
>
> static int mem_cgroup_walk_all(unsigned cpu,
>                int (*func)(struct mem_cgroup *memcg, unsigned cpu))
>

Hmm. As generic function, I may have to add void *data...we already have

 - mem_cgroup_walk_tree() # check hierarchy subtree, not walk all.

This function itself doesn't assume any context of its caller.
(But see below)

>
>> +/*
>> + * CPU Hotplug handling.
>> + */
>> +static int synchronize_move_stat(struct mem_cgroup *mem, void *data)
>> +{
>> +     long cpu = (long)data;
>> +     s64 x = this_cpu_read(mem->stat->count[MEM_CGROUP_ON_MOVE]);
>> +     /* All cpus should have the same value */
>> +     per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = x;
>> +     return 0;
>> +}
>> +
>> +static int drain_all_percpu(struct mem_cgroup *mem, void *data)
>> +{
>> +     long cpu = (long)(data);
>> +     int i;
>> +     /* Drain data from dying cpu and move to local cpu */
>> +     for (i = 0; i < MEM_CGROUP_STAT_DATA; i++) {
>> +             s64 data = per_cpu(mem->stat->count[i], cpu);
>> +             per_cpu(mem->stat->count[i], cpu) = 0;
>> +             this_cpu_add(mem->stat->count[i], data);
>> +     }
>> +     /* Reset Move Count */
>> +     per_cpu(mem->stat->count[MEM_CGROUP_ON_MOVE], cpu) = 0;
>> +     return 0;
>> +}
>
> Some nice comments would be nice.
>
> I don't immediately see anything which guarantees that preemption (and
> cpu migration) are disabled here.  It would be an odd thing to permit
> migration within a cpu-hotplug handler, but where did we guarantee it?

Above code doesn't assume preempt_disable(). Just modify a DEAD cpu's counter.
this_cpu_add() is preempt-safe. I'll add a comment.


> Also, the code appears to assume that the current CPU is the one which
> is being onlined.  What guaranteed that?  This is not the case for
> enable_nonboot_cpus().
>
I thought DEAD cpu is not on scheduler. DEAD notify is done after
cpu_disable().
Hmm, ONLINE handler may have some trouble, I'll write a fix. It's easy.


> It's conventional to put a blank line between end-of-locals and
> start-of-code.  This patch ignored that convention rather a lot.
>
I tend to do that, my mistake.

> The comments in this patch Have Rather Strange Capitalisation Decisions.
>
Ah, sorry.


>> +static int __cpuinit memcg_cpuhotplug_callback(struct notifier_block *nb,
>> +                                     unsigned long action,
>> +                                     void *hcpu)
>> +{
>> +     long cpu = (unsigned long)hcpu;
>> +     struct memcg_stock_pcp *stock;
>> +
>> +     if (action == CPU_ONLINE) {
>> +             mem_cgroup_walk_all((void *)cpu, synchronize_move_stat);
>
> More typecasts which can go away if we make the above change to
> mem_cgroup_walk_all().
>
hmm, I'll rename the function as mem_cgroup_walk_all_cpu().

Thank you for review.
I'll write an update but it may take 3-4days, sorry.

-Kame

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Hiroyuki Kamezawa]

2010/9/17 Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 16 Sep 2010 14:46:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
>> This is onto The mm-of-the-moment snapshot 2010-09-15-16-21.
>>
>> ==
>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> Now, memory cgroup uses for_each_possible_cpu() for percpu stat handling

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Hiroyuki Kamezawa]

2010/9/17 Balbir Singh <balbir@linux.vnet.ibm.com>:
> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:
>
>> On Thu, 16 Sep 2010 15:22:04 +0900
>> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>>
>> > This naming is from mem_cgroup_walk_tree(). Now we have
>> >
>> >   mem_cgroup_walk_tree();
>> >   mem_cgroup_walk_all();
>> >
>> > Rename both ? But it should be in separated patch.
>> >
>>
>> Considering a bit ...but..
>>
>> #define for_each_mem_cgroup(mem) \
>>       for (mem = mem_cgroup_get_first(); \
>>            mem; \
>>            mem = mem_cgroup_get_next(mem);) \
>>
>> seems to need some helper functions. I'll consider about this clean up
>> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
>> are problematic.
>>
>
> Why does this need to be a macro (I know we use this for lists and
> other places), assuming for now we don't use the iterator pattern, we
> can rename mem_cgroup_walk_all() to for_each_mem_cgroup().
>

When I see for_each in the kernel source, I expect iterator and macro.
When I see "walk" in the kernel source, I expect callback and visit function.

Thanks,
-Kame

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Hiroyuki Kamezawa]

2010/9/17 Balbir Singh <balbir@linux.vnet.ibm.com>:
> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:
>
>> On Thu, 16 Sep 2010 15:22:04 +0900
>> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>>
>> > This naming is from mem_cgroup_walk_tree(). Now we have
>> >
>> >   mem_cgroup_walk_tree();
>> >   mem_cgroup_walk_all();
>> >
>> > Rename both ? But it should be in separated patch.
>> >
>>
>> Considering a bit ...but..
>>
>> #define for_each_mem_cgroup(mem) \
>>       for (mem = mem_cgroup_get_first(); \
>>            mem; \
>>            mem = mem_cgroup_get_next(mem);) \
>>
>> seems to need some helper functions. I'll consider about this clean up
>> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
>> are problematic.
>>
>
> Why does this need to be a macro (I know we use this for lists and
> other places), assuming for now we don't use the iterator pattern, we
> can rename mem_cgroup_walk_all() to for_each_mem_cgroup().
>

When I see for_each in the kernel source, I expect iterator and macro.
When I see "walk" in the kernel source, I expect callback and visit function.

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>

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* Hiroyuki Kamezawa <kamezawa.hiroyuki@gmail.com> [2010-09-17 20:49:09]:

> 2010/9/17 Balbir Singh <balbir@linux.vnet.ibm.com>:
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:
> >
> >> On Thu, 16 Sep 2010 15:22:04 +0900
> >> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >>
> >> > This naming is from mem_cgroup_walk_tree(). Now we have
> >> >
> >> >   mem_cgroup_walk_tree();
> >> >   mem_cgroup_walk_all();
> >> >
> >> > Rename both ? But it should be in separated patch.
> >> >
> >>
> >> Considering a bit ...but..
> >>
> >> #define for_each_mem_cgroup(mem) \
> >>       for (mem = mem_cgroup_get_first(); \
> >>            mem; \
> >>            mem = mem_cgroup_get_next(mem);) \
> >>
> >> seems to need some helper functions. I'll consider about this clean up
> >> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> >> are problematic.
> >>
> >
> > Why does this need to be a macro (I know we use this for lists and
> > other places), assuming for now we don't use the iterator pattern, we
> > can rename mem_cgroup_walk_all() to for_each_mem_cgroup().
> >
> 
> When I see for_each in the kernel source, I expect iterator and macro.
> When I see "walk" in the kernel source, I expect callback and visit function.
>

I understand that is the convention we used thus far. When I see
for_each for walk, I presume iterators, doesn't matter if we have a
call back or not. I'll leave the decision to you. 

-- 
	Three Cheers,
	Balbir

Re: [PATCH][-mm] memcg : memory cgroup cpu hotplug support update.

[Balbir Singh]

* Hiroyuki Kamezawa <kamezawa.hiroyuki@gmail.com> [2010-09-17 20:49:09]:

> 2010/9/17 Balbir Singh <balbir@linux.vnet.ibm.com>:
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2010-09-16 16:17:27]:
> >
> >> On Thu, 16 Sep 2010 15:22:04 +0900
> >> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >>
> >> > This naming is from mem_cgroup_walk_tree(). Now we have
> >> >
> >> >   mem_cgroup_walk_tree();
> >> >   mem_cgroup_walk_all();
> >> >
> >> > Rename both ? But it should be in separated patch.
> >> >
> >>
> >> Considering a bit ...but..
> >>
> >> #define for_each_mem_cgroup(mem) \
> >>       for (mem = mem_cgroup_get_first(); \
> >>            mem; \
> >>            mem = mem_cgroup_get_next(mem);) \
> >>
> >> seems to need some helper functions. I'll consider about this clean up
> >> but it requires some amount of patch because css_get()/css_put()/rcu...etc..
> >> are problematic.
> >>
> >
> > Why does this need to be a macro (I know we use this for lists and
> > other places), assuming for now we don't use the iterator pattern, we
> > can rename mem_cgroup_walk_all() to for_each_mem_cgroup().
> >
> 
> When I see for_each in the kernel source, I expect iterator and macro.
> When I see "walk" in the kernel source, I expect callback and visit function.
>

I understand that is the convention we used thus far. When I see
for_each for walk, I presume iterators, doesn't matter if we have a
call back or not. I'll leave the decision to you. 

-- 
	Three Cheers,
	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>