[PATCH 1/6] memcg: fix unused variable warning

[PATCH 1/6] memcg: fix unused variable warning

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

mm/memcontrol.c: In function ‘memcg_check_events’:
mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d643bd6..a5e92bd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
 	/* threshold event is triggered in finer grain than soft limit */
 	if (unlikely(mem_cgroup_event_ratelimit(memcg,
 						MEM_CGROUP_TARGET_THRESH))) {
-		bool do_softlimit, do_numainfo;
+		bool do_softlimit;
 
-		do_softlimit = mem_cgroup_event_ratelimit(memcg,
-						MEM_CGROUP_TARGET_SOFTLIMIT);
 #if MAX_NUMNODES > 1
+		bool do_numainfo;
 		do_numainfo = mem_cgroup_event_ratelimit(memcg,
 						MEM_CGROUP_TARGET_NUMAINFO);
 #endif
+		do_softlimit = mem_cgroup_event_ratelimit(memcg,
+						MEM_CGROUP_TARGET_SOFTLIMIT);
 		preempt_enable();
 
 		mem_cgroup_threshold(memcg);
-- 
1.7.7.3

[PATCH 2/6] memcg: mark more functions/variables as static

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

Based on sparse output.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a5e92bd..4bac3a2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -59,7 +59,7 @@
 
 struct cgroup_subsys mem_cgroup_subsys __read_mostly;
 #define MEM_CGROUP_RECLAIM_RETRIES	5
-struct mem_cgroup *root_mem_cgroup __read_mostly;
+static struct mem_cgroup *root_mem_cgroup __read_mostly;
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 /* Turned on only when memory cgroup is enabled && really_do_swap_account = 1 */
@@ -1573,7 +1573,7 @@ int mem_cgroup_select_victim_node(struct mem_cgroup *memcg)
  * unused nodes. But scan_nodes is lazily updated and may not cotain
  * enough new information. We need to do double check.
  */
-bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
+static bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
 {
 	int nid;
 
@@ -1608,7 +1608,7 @@ int mem_cgroup_select_victim_node(struct mem_cgroup *memcg)
 	return 0;
 }
 
-bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
+static bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
 {
 	return test_mem_cgroup_node_reclaimable(memcg, 0, noswap);
 }
@@ -1782,7 +1782,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
 /*
  * try to call OOM killer. returns false if we should exit memory-reclaim loop.
  */
-bool mem_cgroup_handle_oom(struct mem_cgroup *memcg, gfp_t mask)
+static bool mem_cgroup_handle_oom(struct mem_cgroup *memcg, gfp_t mask)
 {
 	struct oom_wait_info owait;
 	bool locked, need_to_kill;
@@ -3765,7 +3765,7 @@ try_to_free:
 	goto move_account;
 }
 
-int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
+static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
 {
 	return mem_cgroup_force_empty(mem_cgroup_from_cont(cont), true);
 }
@@ -4044,7 +4044,7 @@ struct mcs_total_stat {
 	s64 stat[NR_MCS_STAT];
 };
 
-struct {
+static struct {
 	char *local_name;
 	char *total_name;
 } memcg_stat_strings[NR_MCS_STAT] = {
-- 
1.7.7.3

Re: [PATCH 2/6] memcg: mark more functions/variables as static

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:15 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> Based on sparse output.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH 2/6] memcg: mark more functions/variables as static

[Michal Hocko]

On Sat 24-12-11 05:00:15, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> Based on sparse output.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Looks good.
Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks
> ---
>  mm/memcontrol.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a5e92bd..4bac3a2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -59,7 +59,7 @@
>  
>  struct cgroup_subsys mem_cgroup_subsys __read_mostly;
>  #define MEM_CGROUP_RECLAIM_RETRIES	5
> -struct mem_cgroup *root_mem_cgroup __read_mostly;
> +static struct mem_cgroup *root_mem_cgroup __read_mostly;
>  
>  #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
>  /* Turned on only when memory cgroup is enabled && really_do_swap_account = 1 */
> @@ -1573,7 +1573,7 @@ int mem_cgroup_select_victim_node(struct mem_cgroup *memcg)
>   * unused nodes. But scan_nodes is lazily updated and may not cotain
>   * enough new information. We need to do double check.
>   */
> -bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
> +static bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
>  {
>  	int nid;
>  
> @@ -1608,7 +1608,7 @@ int mem_cgroup_select_victim_node(struct mem_cgroup *memcg)
>  	return 0;
>  }
>  
> -bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
> +static bool mem_cgroup_reclaimable(struct mem_cgroup *memcg, bool noswap)
>  {
>  	return test_mem_cgroup_node_reclaimable(memcg, 0, noswap);
>  }
> @@ -1782,7 +1782,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
>  /*
>   * try to call OOM killer. returns false if we should exit memory-reclaim loop.
>   */
> -bool mem_cgroup_handle_oom(struct mem_cgroup *memcg, gfp_t mask)
> +static bool mem_cgroup_handle_oom(struct mem_cgroup *memcg, gfp_t mask)
>  {
>  	struct oom_wait_info owait;
>  	bool locked, need_to_kill;
> @@ -3765,7 +3765,7 @@ try_to_free:
>  	goto move_account;
>  }
>  
> -int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
> +static int mem_cgroup_force_empty_write(struct cgroup *cont, unsigned int event)
>  {
>  	return mem_cgroup_force_empty(mem_cgroup_from_cont(cont), true);
>  }
> @@ -4044,7 +4044,7 @@ struct mcs_total_stat {
>  	s64 stat[NR_MCS_STAT];
>  };
>  
> -struct {
> +static struct {
>  	char *local_name;
>  	char *total_name;
>  } memcg_stat_strings[NR_MCS_STAT] = {
> -- 
> 1.7.7.3
> 

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

[PATCH 3/6] memcg: remove unused variable

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

mm/memcontrol.c: In function ‘mc_handle_file_pte’:
mm/memcontrol.c:5206:16: warning: variable ‘inode’ set but not used [-Wunused-but-set-variable]

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4bac3a2..627c19e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5203,7 +5203,6 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 			unsigned long addr, pte_t ptent, swp_entry_t *entry)
 {
 	struct page *page = NULL;
-	struct inode *inode;
 	struct address_space *mapping;
 	pgoff_t pgoff;
 
@@ -5212,7 +5211,6 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 	if (!move_file())
 		return NULL;
 
-	inode = vma->vm_file->f_path.dentry->d_inode;
 	mapping = vma->vm_file->f_mapping;
 	if (pte_none(ptent))
 		pgoff = linear_page_index(vma, addr);
-- 
1.7.7.3

Re: [PATCH 3/6] memcg: remove unused variable

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:16 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> mm/memcontrol.c: In function ‘mc_handle_file_pte’:
> mm/memcontrol.c:5206:16: warning: variable ‘inode’ set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

nice.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH 3/6] memcg: remove unused variable

[Michal Hocko]

On Sat 24-12-11 05:00:16, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> mm/memcontrol.c: In function ‘mc_handle_file_pte’:
> mm/memcontrol.c:5206:16: warning: variable ‘inode’ set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Looks good.
Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!
> ---
>  mm/memcontrol.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4bac3a2..627c19e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5203,7 +5203,6 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
>  			unsigned long addr, pte_t ptent, swp_entry_t *entry)
>  {
>  	struct page *page = NULL;
> -	struct inode *inode;
>  	struct address_space *mapping;
>  	pgoff_t pgoff;
>  
> @@ -5212,7 +5211,6 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
>  	if (!move_file())
>  		return NULL;
>  
> -	inode = vma->vm_file->f_path.dentry->d_inode;
>  	mapping = vma->vm_file->f_mapping;
>  	if (pte_none(ptent))
>  		pgoff = linear_page_index(vma, addr);
> -- 
> 1.7.7.3
> 

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

[PATCH 4/6] memcg: mark stat field of mem_cgroup struct as __percpu

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

It fixes a lot of sparse warnings.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 627c19e..b27ce0f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -291,7 +291,7 @@ struct mem_cgroup {
 	/*
 	 * percpu counter.
 	 */
-	struct mem_cgroup_stat_cpu *stat;
+	struct mem_cgroup_stat_cpu __percpu *stat;
 	/*
 	 * used when a cpu is offlined or other synchronizations
 	 * See mem_cgroup_read_stat().
-- 
1.7.7.3

Re: [PATCH 4/6] memcg: mark stat field of mem_cgroup struct as __percpu

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:17 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> It fixes a lot of sparse warnings.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH 4/6] memcg: mark stat field of mem_cgroup struct as __percpu

[Michal Hocko]

On Sat 24-12-11 05:00:17, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> It fixes a lot of sparse warnings.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Looks good.
Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks

> ---
>  mm/memcontrol.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 627c19e..b27ce0f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -291,7 +291,7 @@ struct mem_cgroup {
>  	/*
>  	 * percpu counter.
>  	 */
> -	struct mem_cgroup_stat_cpu *stat;
> +	struct mem_cgroup_stat_cpu __percpu *stat;
>  	/*
>  	 * used when a cpu is offlined or other synchronizations
>  	 * See mem_cgroup_read_stat().
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[PATCH 5/6] memcg: fix broken boolen expression

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b27ce0f..3833a7b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
 		return NOTIFY_OK;
 	}
 
-	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
+	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
 		return NOTIFY_OK;
 
 	for_each_mem_cgroup(iter)
-- 
1.7.7.3

Re: [PATCH 5/6] memcg: fix broken boolen expression

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:18 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

maybe this should go stable..

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


> ---
>  mm/memcontrol.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b27ce0f..3833a7b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
>  		return NOTIFY_OK;
>  	}
>  
> -	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
> +	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
>  		return NOTIFY_OK;
>  
>  	for_each_mem_cgroup(iter)
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] memcg: fix broken boolen expression

[Kirill A. Shutemov]

On Mon, Dec 26, 2011 at 03:31:38PM +0900, KAMEZAWA Hiroyuki wrote:
> On Sat, 24 Dec 2011 05:00:18 +0200
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > 
> > action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> 
> maybe this should go stable..

CC stable@

> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> 
> > ---
> >  mm/memcontrol.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index b27ce0f..3833a7b 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
> >  		return NOTIFY_OK;
> >  	}
> >  
> > -	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
> > +	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
> >  		return NOTIFY_OK;
> >  
> >  	for_each_mem_cgroup(iter)
> > -- 
> > 1.7.7.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
 Kirill A. Shutemov

Re: [PATCH 5/6] memcg: fix broken boolen expression

[Greg KH]

On Mon, Dec 26, 2011 at 08:57:24AM +0200, Kirill A. Shutemov wrote:
> On Mon, Dec 26, 2011 at 03:31:38PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Sat, 24 Dec 2011 05:00:18 +0200
> > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > 
> > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > 
> > > action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > 
> > maybe this should go stable..
> 
> CC stable@

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH 5/6] memcg: fix broken boolen expression

[Michal Hocko]

On Sat 24-12-11 05:00:18, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!

> ---
>  mm/memcontrol.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b27ce0f..3833a7b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
>  		return NOTIFY_OK;
>  	}
>  
> -	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
> +	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
>  		return NOTIFY_OK;
>  
>  	for_each_mem_cgroup(iter)
> -- 
> 1.7.7.3
> 

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

Re: [PATCH 5/6] memcg: fix broken boolen expression

[Andrew Morton]

On Sat, 24 Dec 2011 05:00:18 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> ---
>  mm/memcontrol.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b27ce0f..3833a7b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
>  		return NOTIFY_OK;
>  	}
>  
> -	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
> +	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
>  		return NOTIFY_OK;
>  
>  	for_each_mem_cgroup(iter)

This spent too long in the backlog, sorry.

I don't want to merge this patch into either mainline or -stable until
I find out what it does!

afacit the patch will newly cause the kernel to drain various resource
counters away from the target CPU when the CPU_DEAD or CPU_DEAD_FROZEN
events occur for thet CPU, yes?

So the user-visible effects of the bug whcih was just fixed is that
these counters will be somewhat inaccurate after a CPU is taken down,
yes?

Why wasn't this bug noticed before?  Has anyone tested the patch and
confirmed that the numbers are now correct?

Given that this bug has been present for 1.5 years and nobody noticed,
I don't think a backport into -stable is warranted?

Re: [PATCH 5/6] memcg: fix broken boolen expression

[Kirill A. Shutemov]

On Wed, Apr 04, 2012 at 02:34:03PM -0700, Andrew Morton wrote:
> On Sat, 24 Dec 2011 05:00:18 +0200
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > 
> > action != CPU_DEAD || action != CPU_DEAD_FROZEN is always true.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > ---
> >  mm/memcontrol.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index b27ce0f..3833a7b 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2100,7 +2100,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
> >  		return NOTIFY_OK;
> >  	}
> >  
> > -	if ((action != CPU_DEAD) || action != CPU_DEAD_FROZEN)
> > +	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
> >  		return NOTIFY_OK;
> >  
> >  	for_each_mem_cgroup(iter)
> 
> This spent too long in the backlog, sorry.
> 
> I don't want to merge this patch into either mainline or -stable until
> I find out what it does!
> 
> afacit the patch will newly cause the kernel to drain various resource
> counters away from the target CPU when the CPU_DEAD or CPU_DEAD_FROZEN
> events occur for thet CPU, yes?

Yes.

> So the user-visible effects of the bug whcih was just fixed is that
> these counters will be somewhat inaccurate after a CPU is taken down,
> yes?

Correct.

> Why wasn't this bug noticed before?

I guess CPU hotplug is not a usual test case for memcg changes. And the
result of the bug is inaccurate statistics, but not something dramatic
(oops, panic, etc.).

> Has anyone tested the patch and
> confirmed that the numbers are now correct?

I haven't. I found the bug with sparse.

> Given that this bug has been present for 1.5 years and nobody noticed,
> I don't think a backport into -stable is warranted?
> 

-- 
 Kirill A. Shutemov

[PATCH 6/6] memcg: drop redundant brackets

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3833a7b..48cba05 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -73,7 +73,7 @@ static int really_do_swap_account __initdata = 0;
 #endif
 
 #else
-#define do_swap_account		(0)
+#define do_swap_account		0
 #endif
 
 
@@ -113,9 +113,9 @@ enum mem_cgroup_events_target {
 	MEM_CGROUP_TARGET_NUMAINFO,
 	MEM_CGROUP_NTARGETS,
 };
-#define THRESHOLDS_EVENTS_TARGET (128)
-#define SOFTLIMIT_EVENTS_TARGET (1024)
-#define NUMAINFO_EVENTS_TARGET	(1024)
+#define THRESHOLDS_EVENTS_TARGET 128
+#define SOFTLIMIT_EVENTS_TARGET 1024
+#define NUMAINFO_EVENTS_TARGET	1024
 
 struct mem_cgroup_stat_cpu {
 	long count[MEM_CGROUP_STAT_NSTATS];
@@ -148,7 +148,7 @@ struct mem_cgroup_per_zone {
 						/* use container_of	   */
 };
 /* Macro for accessing counter */
-#define MEM_CGROUP_ZSTAT(mz, idx)	((mz)->count[(idx)])
+#define MEM_CGROUP_ZSTAT(mz, idx)	((mz)->count[idx])
 
 struct mem_cgroup_per_node {
 	struct mem_cgroup_per_zone zoneinfo[MAX_NR_ZONES];
@@ -346,8 +346,8 @@ static bool move_file(void)
  * Maximum loops in mem_cgroup_hierarchical_reclaim(), used for soft
  * limit reclaim to prevent infinite loops, if they ever occur.
  */
-#define	MEM_CGROUP_MAX_RECLAIM_LOOPS		(100)
-#define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	(2)
+#define	MEM_CGROUP_MAX_RECLAIM_LOOPS		100
+#define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	2
 
 enum charge_type {
 	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
@@ -368,11 +368,11 @@ enum mem_type {
 	_KMEM,
 };
 
-#define MEMFILE_PRIVATE(x, val)	(((x) << 16) | (val))
-#define MEMFILE_TYPE(val)	(((val) >> 16) & 0xffff)
+#define MEMFILE_PRIVATE(x, val)	((x) << 16 | (val))
+#define MEMFILE_TYPE(val)	((val) >> 16 & 0xffff)
 #define MEMFILE_ATTR(val)	((val) & 0xffff)
 /* Used for OOM nofiier */
-#define OOM_CONTROL		(0)
+#define OOM_CONTROL		0
 
 /*
  * Reclaim flags for mem_cgroup_hierarchical_reclaim
@@ -1913,7 +1913,7 @@ struct memcg_stock_pcp {
 	unsigned int nr_pages;
 	struct work_struct work;
 	unsigned long flags;
-#define FLUSHING_CACHED_CHARGE	(0)
+#define FLUSHING_CACHED_CHARGE	0
 };
 static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock);
 static DEFINE_MUTEX(percpu_charge_mutex);
@@ -2094,7 +2094,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
 	struct memcg_stock_pcp *stock;
 	struct mem_cgroup *iter;
 
-	if ((action == CPU_ONLINE)) {
+	if (action == CPU_ONLINE) {
 		for_each_mem_cgroup(iter)
 			synchronize_mem_cgroup_on_move(iter, cpu);
 		return NOTIFY_OK;
@@ -2458,8 +2458,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 
-#define PCGF_NOCOPY_AT_SPLIT ((1 << PCG_LOCK) | (1 << PCG_MOVE_LOCK) |\
-			(1 << PCG_MIGRATION))
+#define PCGF_NOCOPY_AT_SPLIT (1 << PCG_LOCK | 1 << PCG_MOVE_LOCK |\
+		1 << PCG_MIGRATION)
 /*
  * Because tail pages are not marked as "used", set it. We're under
  * zone->lru_lock, 'splitting on pmd' and compound_lock.
-- 
1.7.7.3

Re: [PATCH 6/6] memcg: drop redundant brackets

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:19 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Maybe I tend to add too many braces at using macro.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH 6/6] memcg: drop redundant brackets

[Michal Hocko]

On Sat 24-12-11 05:00:19, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

I wasn't very convinced at first but it makes some sense as we should be
consistent. So
Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/memcontrol.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3833a7b..48cba05 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -73,7 +73,7 @@ static int really_do_swap_account __initdata = 0;
>  #endif
>  
>  #else
> -#define do_swap_account		(0)
> +#define do_swap_account		0
>  #endif
>  
>  
> @@ -113,9 +113,9 @@ enum mem_cgroup_events_target {
>  	MEM_CGROUP_TARGET_NUMAINFO,
>  	MEM_CGROUP_NTARGETS,
>  };
> -#define THRESHOLDS_EVENTS_TARGET (128)
> -#define SOFTLIMIT_EVENTS_TARGET (1024)
> -#define NUMAINFO_EVENTS_TARGET	(1024)
> +#define THRESHOLDS_EVENTS_TARGET 128
> +#define SOFTLIMIT_EVENTS_TARGET 1024
> +#define NUMAINFO_EVENTS_TARGET	1024
>  
>  struct mem_cgroup_stat_cpu {
>  	long count[MEM_CGROUP_STAT_NSTATS];
> @@ -148,7 +148,7 @@ struct mem_cgroup_per_zone {
>  						/* use container_of	   */
>  };
>  /* Macro for accessing counter */
> -#define MEM_CGROUP_ZSTAT(mz, idx)	((mz)->count[(idx)])
> +#define MEM_CGROUP_ZSTAT(mz, idx)	((mz)->count[idx])
>  
>  struct mem_cgroup_per_node {
>  	struct mem_cgroup_per_zone zoneinfo[MAX_NR_ZONES];
> @@ -346,8 +346,8 @@ static bool move_file(void)
>   * Maximum loops in mem_cgroup_hierarchical_reclaim(), used for soft
>   * limit reclaim to prevent infinite loops, if they ever occur.
>   */
> -#define	MEM_CGROUP_MAX_RECLAIM_LOOPS		(100)
> -#define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	(2)
> +#define	MEM_CGROUP_MAX_RECLAIM_LOOPS		100
> +#define	MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS	2
>  
>  enum charge_type {
>  	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
> @@ -368,11 +368,11 @@ enum mem_type {
>  	_KMEM,
>  };
>  
> -#define MEMFILE_PRIVATE(x, val)	(((x) << 16) | (val))
> -#define MEMFILE_TYPE(val)	(((val) >> 16) & 0xffff)
> +#define MEMFILE_PRIVATE(x, val)	((x) << 16 | (val))
> +#define MEMFILE_TYPE(val)	((val) >> 16 & 0xffff)
>  #define MEMFILE_ATTR(val)	((val) & 0xffff)
>  /* Used for OOM nofiier */
> -#define OOM_CONTROL		(0)
> +#define OOM_CONTROL		0
>  
>  /*
>   * Reclaim flags for mem_cgroup_hierarchical_reclaim
> @@ -1913,7 +1913,7 @@ struct memcg_stock_pcp {
>  	unsigned int nr_pages;
>  	struct work_struct work;
>  	unsigned long flags;
> -#define FLUSHING_CACHED_CHARGE	(0)
> +#define FLUSHING_CACHED_CHARGE	0
>  };
>  static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock);
>  static DEFINE_MUTEX(percpu_charge_mutex);
> @@ -2094,7 +2094,7 @@ static int __cpuinit memcg_cpu_hotplug_callback(struct notifier_block *nb,
>  	struct memcg_stock_pcp *stock;
>  	struct mem_cgroup *iter;
>  
> -	if ((action == CPU_ONLINE)) {
> +	if (action == CPU_ONLINE) {
>  		for_each_mem_cgroup(iter)
>  			synchronize_mem_cgroup_on_move(iter, cpu);
>  		return NOTIFY_OK;
> @@ -2458,8 +2458,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  
> -#define PCGF_NOCOPY_AT_SPLIT ((1 << PCG_LOCK) | (1 << PCG_MOVE_LOCK) |\
> -			(1 << PCG_MIGRATION))
> +#define PCGF_NOCOPY_AT_SPLIT (1 << PCG_LOCK | 1 << PCG_MOVE_LOCK |\
> +		1 << PCG_MIGRATION)
>  /*
>   * Because tail pages are not marked as "used", set it. We're under
>   * zone->lru_lock, 'splitting on pmd' and compound_lock.
> -- 
> 1.7.7.3
> 

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

Re: [PATCH 1/6] memcg: fix unused variable warning

[KAMEZAWA Hiroyuki]

On Sat, 24 Dec 2011 05:00:14 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> mm/memcontrol.c: In function ‘memcg_check_events’:
> mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

Hmm ? Doesn't this fix cause a new Warning ?

mm/memcontrol.c: In function ?memcg_check_events?:
mm/memcontrol.c:789: warning: ISO C90 forbids mixed declarations and code

Thanks,
-Kame
> ---
>  mm/memcontrol.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d643bd6..a5e92bd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
>  	/* threshold event is triggered in finer grain than soft limit */
>  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_THRESH))) {
> -		bool do_softlimit, do_numainfo;
> +		bool do_softlimit;
>  
> -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> -						MEM_CGROUP_TARGET_SOFTLIMIT);
>  #if MAX_NUMNODES > 1
> +		bool do_numainfo;
>  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_NUMAINFO);
>  #endif
> +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> +						MEM_CGROUP_TARGET_SOFTLIMIT);
>  		preempt_enable();
>  
>  		mem_cgroup_threshold(memcg);
> -- 
> 1.7.7.3
> 

Re: [PATCH 1/6] memcg: fix unused variable warning

[Kirill A. Shutemov]

On Mon, Dec 26, 2011 at 03:25:31PM +0900, KAMEZAWA Hiroyuki wrote:
> On Sat, 24 Dec 2011 05:00:14 +0200
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > 
> > mm/memcontrol.c: In function ‘memcg_check_events’:
> > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> 
> Hmm ? Doesn't this fix cause a new Warning ?
> 
> mm/memcontrol.c: In function ?memcg_check_events?:
> mm/memcontrol.c:789: warning: ISO C90 forbids mixed declarations and code

I don't see how. The result code is:

	if (unlikely(mem_cgroup_event_ratelimit(memcg,
						MEM_CGROUP_TARGET_THRESH))) {
		bool do_softlimit;

#if MAX_NUMNODES > 1
		bool do_numainfo;
		do_numainfo = mem_cgroup_event_ratelimit(memcg,
						MEM_CGROUP_TARGET_NUMAINFO);
#endif
		do_softlimit = mem_cgroup_event_ratelimit(memcg,
						MEM_CGROUP_TARGET_SOFTLIMIT);
		preempt_enable();

		mem_cgroup_threshold(memcg);


> 
> Thanks,
> -Kame
> > ---
> >  mm/memcontrol.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index d643bd6..a5e92bd 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> >  	/* threshold event is triggered in finer grain than soft limit */
> >  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_THRESH))) {
> > -		bool do_softlimit, do_numainfo;
> > +		bool do_softlimit;
> >  
> > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > -						MEM_CGROUP_TARGET_SOFTLIMIT);
> >  #if MAX_NUMNODES > 1
> > +		bool do_numainfo;
> >  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_NUMAINFO);
> >  #endif
> > +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > +						MEM_CGROUP_TARGET_SOFTLIMIT);
> >  		preempt_enable();
> >  
> >  		mem_cgroup_threshold(memcg);
> > -- 
> > 1.7.7.3
> > 
> 

-- 
 Kirill A. Shutemov

Re: [PATCH 1/6] memcg: fix unused variable warning

[KAMEZAWA Hiroyuki]

On Mon, 26 Dec 2011 08:36:52 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Mon, Dec 26, 2011 at 03:25:31PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Sat, 24 Dec 2011 05:00:14 +0200
> > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > 
> > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > 
> > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > 
> > Hmm ? Doesn't this fix cause a new Warning ?
> > 
> > mm/memcontrol.c: In function ?memcg_check_events?:
> > mm/memcontrol.c:789: warning: ISO C90 forbids mixed declarations and code
> 
> I don't see how. The result code is:
> 
> 	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> 						MEM_CGROUP_TARGET_THRESH))) {
> 		bool do_softlimit;
> 
> #if MAX_NUMNODES > 1
> 		bool do_numainfo;
> 		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> 						MEM_CGROUP_TARGET_NUMAINFO);
> #endif
> 		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> 						MEM_CGROUP_TARGET_SOFTLIMIT);
> 		preempt_enable();
> 
> 		mem_cgroup_threshold(memcg);
> 

Ah. please see linux-next and rebase onto that.

Thanks,
-Kame

Re: [PATCH 1/6] memcg: fix unused variable warning

[Kirill A. Shutemov]

On Mon, Dec 26, 2011 at 03:42:52PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 26 Dec 2011 08:36:52 +0200
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Mon, Dec 26, 2011 at 03:25:31PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Sat, 24 Dec 2011 05:00:14 +0200
> > > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > > 
> > > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > 
> > > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > > 
> > > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > 
> > > Hmm ? Doesn't this fix cause a new Warning ?
> > > 
> > > mm/memcontrol.c: In function ?memcg_check_events?:
> > > mm/memcontrol.c:789: warning: ISO C90 forbids mixed declarations and code
> > 
> > I don't see how. The result code is:
> > 
> > 	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > 						MEM_CGROUP_TARGET_THRESH))) {
> > 		bool do_softlimit;
> > 
> > #if MAX_NUMNODES > 1
> > 		bool do_numainfo;
> > 		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > 						MEM_CGROUP_TARGET_NUMAINFO);
> > #endif
> > 		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > 						MEM_CGROUP_TARGET_SOFTLIMIT);
> > 		preempt_enable();
> > 
> > 		mem_cgroup_threshold(memcg);
> > 
> 
> Ah. please see linux-next and rebase onto that.

The patchset is on top of next-20111222. Have I missed something?

> 
> Thanks,
> -Kame
> 

-- 
 Kirill A. Shutemov

Re: [PATCH 1/6] memcg: fix unused variable warning

[KAMEZAWA Hiroyuki]

On Mon, 26 Dec 2011 08:47:34 +0200
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Mon, Dec 26, 2011 at 03:42:52PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 26 Dec 2011 08:36:52 +0200
> > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > 
> > > On Mon, Dec 26, 2011 at 03:25:31PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Sat, 24 Dec 2011 05:00:14 +0200
> > > > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > > > 
> > > > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > > 
> > > > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > > > 
> > > > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > > 
> > > > Hmm ? Doesn't this fix cause a new Warning ?
> > > > 
> > > > mm/memcontrol.c: In function ?memcg_check_events?:
> > > > mm/memcontrol.c:789: warning: ISO C90 forbids mixed declarations and code
> > > 
> > > I don't see how. The result code is:
> > > 
> > > 	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > > 						MEM_CGROUP_TARGET_THRESH))) {
> > > 		bool do_softlimit;
> > > 
> > > #if MAX_NUMNODES > 1
> > > 		bool do_numainfo;
> > > 		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > > 						MEM_CGROUP_TARGET_NUMAINFO);
> > > #endif
> > > 		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > 						MEM_CGROUP_TARGET_SOFTLIMIT);
> > > 		preempt_enable();
> > > 
> > > 		mem_cgroup_threshold(memcg);
> > > 
> > 
> > Ah. please see linux-next and rebase onto that.
> 
> The patchset is on top of next-20111222. Have I missed something?
> 
Ah, ok. my mistake. Sorry.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

[PATCH] Makefiles: Disable unused-variable warning (was: Re: [PATCH 1/6] memcg: fix unused variable warning)

[Michal Hocko]

On Sat 24-12-11 05:00:14, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> mm/memcontrol.c: In function ‘memcg_check_events’:
> mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> ---
>  mm/memcontrol.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d643bd6..a5e92bd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
>  	/* threshold event is triggered in finer grain than soft limit */
>  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_THRESH))) {
> -		bool do_softlimit, do_numainfo;
> +		bool do_softlimit;
>  
> -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> -						MEM_CGROUP_TARGET_SOFTLIMIT);
>  #if MAX_NUMNODES > 1
> +		bool do_numainfo;
>  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_NUMAINFO);
>  #endif
> +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> +						MEM_CGROUP_TARGET_SOFTLIMIT);

I don't like this very much. Maybe we should get rid of both do_* and
do it with flags? But maybe it is not worth the additional code at
all...

Anyway, I am wondering why unused-but-set-variable is disabled while
unused-variable is enabled. Shouldn't we just disable it as well rather
than workaround this in the code? The warning is just pure noise in this
case.
What about something like:
---
>From e1136891fe86eacf9212b2144f80ff6b75b10194 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 27 Dec 2011 14:53:06 +0100
Subject: [PATCH] Makefiles: Disable unused-variable warning

We are already disabling unused-but-set-variable and Wunused-variable
produces some noise as well.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index ea51081..25c76f3 100644
--- a/Makefile
+++ b/Makefile
@@ -578,6 +578,7 @@ endif
 # This warning generated too much noise in a regular build.
 # Use make W=1 to enable this warning (see scripts/Makefile.build)
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
+KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
 
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
-- 
1.7.7.3



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

Re: [PATCH] Makefiles: Disable unused-variable warning (was: Re: [PATCH 1/6] memcg: fix unused variable warning)

[Kirill A. Shutemov]

On Tue, Dec 27, 2011 at 02:57:52PM +0100, Michal Hocko wrote:
> On Sat 24-12-11 05:00:14, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > 
> > mm/memcontrol.c: In function ‘memcg_check_events’:
> > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > ---
> >  mm/memcontrol.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index d643bd6..a5e92bd 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> >  	/* threshold event is triggered in finer grain than soft limit */
> >  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_THRESH))) {
> > -		bool do_softlimit, do_numainfo;
> > +		bool do_softlimit;
> >  
> > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > -						MEM_CGROUP_TARGET_SOFTLIMIT);
> >  #if MAX_NUMNODES > 1
> > +		bool do_numainfo;
> >  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_NUMAINFO);
> >  #endif
> > +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > +						MEM_CGROUP_TARGET_SOFTLIMIT);
> 
> I don't like this very much. Maybe we should get rid of both do_* and
> do it with flags? But maybe it is not worth the additional code at
> all...

Something like this (untested):

====
>From f57e1a2e1aaaa167c75b963d5bf12fcbdd3331b8 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill@shutemov.name>
Date: Tue, 27 Dec 2011 20:17:13 +0200
Subject: [PATCH] memcg: cleanup memcg_check_events()

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |   42 ++++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d643bd6..40c2236 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -108,11 +108,12 @@ enum mem_cgroup_events_index {
  * than using jiffies etc. to handle periodic memcg event.
  */
 enum mem_cgroup_events_target {
-	MEM_CGROUP_TARGET_THRESH,
-	MEM_CGROUP_TARGET_SOFTLIMIT,
-	MEM_CGROUP_TARGET_NUMAINFO,
-	MEM_CGROUP_NTARGETS,
+	MEM_CGROUP_TARGET_THRESH	= BIT(1),
+	MEM_CGROUP_TARGET_SOFTLIMIT	= BIT(2),
+	MEM_CGROUP_TARGET_NUMAINFO	= BIT(3),
 };
+#define MEM_CGROUP_NTARGETS 3
+
 #define THRESHOLDS_EVENTS_TARGET (128)
 #define SOFTLIMIT_EVENTS_TARGET (1024)
 #define NUMAINFO_EVENTS_TARGET	(1024)
@@ -743,7 +744,7 @@ static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
 	return total;
 }
 
-static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
+static int mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
 				       enum mem_cgroup_events_target target)
 {
 	unsigned long val, next;
@@ -766,9 +767,9 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
 			break;
 		}
 		__this_cpu_write(memcg->stat->targets[target], next);
-		return true;
+		return target;
 	}
-	return false;
+	return 0;
 }
 
 /*
@@ -777,29 +778,34 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
  */
 static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
 {
+	int flags;
+
 	preempt_disable();
-	/* threshold event is triggered in finer grain than soft limit */
-	if (unlikely(mem_cgroup_event_ratelimit(memcg,
-						MEM_CGROUP_TARGET_THRESH))) {
-		bool do_softlimit, do_numainfo;
+	flags = mem_cgroup_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH);
 
-		do_softlimit = mem_cgroup_event_ratelimit(memcg,
+	/*
+	 * Threshold event is triggered in finer grain than soft limit
+	 * and numainfo
+	 */
+	if (unlikely(flags)) {
+		flags |= mem_cgroup_event_ratelimit(memcg,
 						MEM_CGROUP_TARGET_SOFTLIMIT);
 #if MAX_NUMNODES > 1
-		do_numainfo = mem_cgroup_event_ratelimit(memcg,
+		flags |= mem_cgroup_event_ratelimit(memcg,
 						MEM_CGROUP_TARGET_NUMAINFO);
 #endif
-		preempt_enable();
+	}
+	preempt_enable();
 
+	if (unlikely(flags)) {
 		mem_cgroup_threshold(memcg);
-		if (unlikely(do_softlimit))
+		if (unlikely(flags & MEM_CGROUP_TARGET_SOFTLIMIT))
 			mem_cgroup_update_tree(memcg, page);
 #if MAX_NUMNODES > 1
-		if (unlikely(do_numainfo))
+		if (unlikely(flags & MEM_CGROUP_TARGET_NUMAINFO))
 			atomic_inc(&memcg->numainfo_events);
 #endif
-	} else
-		preempt_enable();
+	}
 }
 
 struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
-- 
1.7.7.3

-- 
 Kirill A. Shutemov

Re: [PATCH] Makefiles: Disable unused-variable warning (was: Re: [PATCH 1/6] memcg: fix unused variable warning)

[Michal Hocko]

On Tue 27-12-11 20:26:13, Kirill A. Shutemov wrote:
> On Tue, Dec 27, 2011 at 02:57:52PM +0100, Michal Hocko wrote:
> > On Sat 24-12-11 05:00:14, Kirill A. Shutemov wrote:
> > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > 
> > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > ---
> > >  mm/memcontrol.c |    7 ++++---
> > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index d643bd6..a5e92bd 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> > >  	/* threshold event is triggered in finer grain than soft limit */
> > >  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > >  						MEM_CGROUP_TARGET_THRESH))) {
> > > -		bool do_softlimit, do_numainfo;
> > > +		bool do_softlimit;
> > >  
> > > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > -						MEM_CGROUP_TARGET_SOFTLIMIT);
> > >  #if MAX_NUMNODES > 1
> > > +		bool do_numainfo;
> > >  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > >  						MEM_CGROUP_TARGET_NUMAINFO);
> > >  #endif
> > > +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > +						MEM_CGROUP_TARGET_SOFTLIMIT);
> > 
> > I don't like this very much. Maybe we should get rid of both do_* and
> > do it with flags? But maybe it is not worth the additional code at
> > all...
> 
> Something like this (untested):
> ====
> From f57e1a2e1aaaa167c75b963d5bf12fcbdd3331b8 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> Date: Tue, 27 Dec 2011 20:17:13 +0200
> Subject: [PATCH] memcg: cleanup memcg_check_events()
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>

The patch looks correct but I am still not sure this is worth fixing in
the code rather than disabling Wunused-variable.

> ---
>  mm/memcontrol.c |   42 ++++++++++++++++++++++++------------------
>  1 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d643bd6..40c2236 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -108,11 +108,12 @@ enum mem_cgroup_events_index {
>   * than using jiffies etc. to handle periodic memcg event.
>   */
>  enum mem_cgroup_events_target {
> -	MEM_CGROUP_TARGET_THRESH,
> -	MEM_CGROUP_TARGET_SOFTLIMIT,
> -	MEM_CGROUP_TARGET_NUMAINFO,
> -	MEM_CGROUP_NTARGETS,
> +	MEM_CGROUP_TARGET_THRESH	= BIT(1),
> +	MEM_CGROUP_TARGET_SOFTLIMIT	= BIT(2),
> +	MEM_CGROUP_TARGET_NUMAINFO	= BIT(3),
>  };
> +#define MEM_CGROUP_NTARGETS 3
> +
>  #define THRESHOLDS_EVENTS_TARGET (128)
>  #define SOFTLIMIT_EVENTS_TARGET (1024)
>  #define NUMAINFO_EVENTS_TARGET	(1024)
> @@ -743,7 +744,7 @@ static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
>  	return total;
>  }
>  
> -static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> +static int mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
>  				       enum mem_cgroup_events_target target)
>  {
>  	unsigned long val, next;
> @@ -766,9 +767,9 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
>  			break;
>  		}
>  		__this_cpu_write(memcg->stat->targets[target], next);
> -		return true;
> +		return target;
>  	}
> -	return false;
> +	return 0;
>  }
>  
>  /*
> @@ -777,29 +778,34 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
>   */
>  static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
>  {
> +	int flags;
> +
>  	preempt_disable();
> -	/* threshold event is triggered in finer grain than soft limit */
> -	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> -						MEM_CGROUP_TARGET_THRESH))) {
> -		bool do_softlimit, do_numainfo;
> +	flags = mem_cgroup_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH);
>  
> -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> +	/*
> +	 * Threshold event is triggered in finer grain than soft limit
> +	 * and numainfo
> +	 */
> +	if (unlikely(flags)) {
> +		flags |= mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_SOFTLIMIT);
>  #if MAX_NUMNODES > 1
> -		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> +		flags |= mem_cgroup_event_ratelimit(memcg,
>  						MEM_CGROUP_TARGET_NUMAINFO);
>  #endif
> -		preempt_enable();
> +	}
> +	preempt_enable();
>  
> +	if (unlikely(flags)) {
>  		mem_cgroup_threshold(memcg);
> -		if (unlikely(do_softlimit))
> +		if (unlikely(flags & MEM_CGROUP_TARGET_SOFTLIMIT))
>  			mem_cgroup_update_tree(memcg, page);
>  #if MAX_NUMNODES > 1
> -		if (unlikely(do_numainfo))
> +		if (unlikely(flags & MEM_CGROUP_TARGET_NUMAINFO))
>  			atomic_inc(&memcg->numainfo_events);
>  #endif
> -	} else
> -		preempt_enable();
> +	}
>  }
>  
>  struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
> -- 
> 1.7.7.3

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

Re: [PATCH] Makefiles: Disable unused-variable warning (was: Re: [PATCH 1/6] memcg: fix unused variable warning)

[Kirill A. Shutemov]

On Thu, Dec 29, 2011 at 11:42:30AM +0100, Michal Hocko wrote:
> On Tue 27-12-11 20:26:13, Kirill A. Shutemov wrote:
> > On Tue, Dec 27, 2011 at 02:57:52PM +0100, Michal Hocko wrote:
> > > On Sat 24-12-11 05:00:14, Kirill A. Shutemov wrote:
> > > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > 
> > > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > > 
> > > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > > ---
> > > >  mm/memcontrol.c |    7 ++++---
> > > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index d643bd6..a5e92bd 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> > > >  	/* threshold event is triggered in finer grain than soft limit */
> > > >  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > > >  						MEM_CGROUP_TARGET_THRESH))) {
> > > > -		bool do_softlimit, do_numainfo;
> > > > +		bool do_softlimit;
> > > >  
> > > > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > > -						MEM_CGROUP_TARGET_SOFTLIMIT);
> > > >  #if MAX_NUMNODES > 1
> > > > +		bool do_numainfo;
> > > >  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > > >  						MEM_CGROUP_TARGET_NUMAINFO);
> > > >  #endif
> > > > +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > > +						MEM_CGROUP_TARGET_SOFTLIMIT);
> > > 
> > > I don't like this very much. Maybe we should get rid of both do_* and
> > > do it with flags? But maybe it is not worth the additional code at
> > > all...
> > 
> > Something like this (untested):
> > ====
> > From f57e1a2e1aaaa167c75b963d5bf12fcbdd3331b8 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Date: Tue, 27 Dec 2011 20:17:13 +0200
> > Subject: [PATCH] memcg: cleanup memcg_check_events()
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> 
> The patch looks correct but I am still not sure this is worth fixing in
> the code rather than disabling Wunused-variable.

Does it look better then original code from your POV?

It's a bit cleaner, I think.

> 
> > ---
> >  mm/memcontrol.c |   42 ++++++++++++++++++++++++------------------
> >  1 files changed, 24 insertions(+), 18 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index d643bd6..40c2236 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -108,11 +108,12 @@ enum mem_cgroup_events_index {
> >   * than using jiffies etc. to handle periodic memcg event.
> >   */
> >  enum mem_cgroup_events_target {
> > -	MEM_CGROUP_TARGET_THRESH,
> > -	MEM_CGROUP_TARGET_SOFTLIMIT,
> > -	MEM_CGROUP_TARGET_NUMAINFO,
> > -	MEM_CGROUP_NTARGETS,
> > +	MEM_CGROUP_TARGET_THRESH	= BIT(1),
> > +	MEM_CGROUP_TARGET_SOFTLIMIT	= BIT(2),
> > +	MEM_CGROUP_TARGET_NUMAINFO	= BIT(3),
> >  };
> > +#define MEM_CGROUP_NTARGETS 3
> > +
> >  #define THRESHOLDS_EVENTS_TARGET (128)
> >  #define SOFTLIMIT_EVENTS_TARGET (1024)
> >  #define NUMAINFO_EVENTS_TARGET	(1024)
> > @@ -743,7 +744,7 @@ static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
> >  	return total;
> >  }
> >  
> > -static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> > +static int mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> >  				       enum mem_cgroup_events_target target)
> >  {
> >  	unsigned long val, next;
> > @@ -766,9 +767,9 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> >  			break;
> >  		}
> >  		__this_cpu_write(memcg->stat->targets[target], next);
> > -		return true;
> > +		return target;
> >  	}
> > -	return false;
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -777,29 +778,34 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> >   */
> >  static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> >  {
> > +	int flags;
> > +
> >  	preempt_disable();
> > -	/* threshold event is triggered in finer grain than soft limit */
> > -	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > -						MEM_CGROUP_TARGET_THRESH))) {
> > -		bool do_softlimit, do_numainfo;
> > +	flags = mem_cgroup_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH);
> >  
> > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > +	/*
> > +	 * Threshold event is triggered in finer grain than soft limit
> > +	 * and numainfo
> > +	 */
> > +	if (unlikely(flags)) {
> > +		flags |= mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_SOFTLIMIT);
> >  #if MAX_NUMNODES > 1
> > -		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > +		flags |= mem_cgroup_event_ratelimit(memcg,
> >  						MEM_CGROUP_TARGET_NUMAINFO);
> >  #endif
> > -		preempt_enable();
> > +	}
> > +	preempt_enable();
> >  
> > +	if (unlikely(flags)) {
> >  		mem_cgroup_threshold(memcg);
> > -		if (unlikely(do_softlimit))
> > +		if (unlikely(flags & MEM_CGROUP_TARGET_SOFTLIMIT))
> >  			mem_cgroup_update_tree(memcg, page);
> >  #if MAX_NUMNODES > 1
> > -		if (unlikely(do_numainfo))
> > +		if (unlikely(flags & MEM_CGROUP_TARGET_NUMAINFO))
> >  			atomic_inc(&memcg->numainfo_events);
> >  #endif
> > -	} else
> > -		preempt_enable();
> > +	}
> >  }
> >  
> >  struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
> > -- 
> > 1.7.7.3
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic

-- 
 Kirill A. Shutemov

Re: [PATCH] Makefiles: Disable unused-variable warning (was: Re: [PATCH 1/6] memcg: fix unused variable warning)

[Michal Hocko]

On Thu 29-12-11 13:08:50, Kirill A. Shutemov wrote:
> On Thu, Dec 29, 2011 at 11:42:30AM +0100, Michal Hocko wrote:
> > On Tue 27-12-11 20:26:13, Kirill A. Shutemov wrote:
> > > On Tue, Dec 27, 2011 at 02:57:52PM +0100, Michal Hocko wrote:
> > > > On Sat 24-12-11 05:00:14, Kirill A. Shutemov wrote:
> > > > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > > 
> > > > > mm/memcontrol.c: In function ‘memcg_check_events’:
> > > > > mm/memcontrol.c:784:22: warning: unused variable ‘do_numainfo’ [-Wunused-variable]
> > > > > 
> > > > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > > > ---
> > > > >  mm/memcontrol.c |    7 ++++---
> > > > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > > index d643bd6..a5e92bd 100644
> > > > > --- a/mm/memcontrol.c
> > > > > +++ b/mm/memcontrol.c
> > > > > @@ -781,14 +781,15 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> > > > >  	/* threshold event is triggered in finer grain than soft limit */
> > > > >  	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > > > >  						MEM_CGROUP_TARGET_THRESH))) {
> > > > > -		bool do_softlimit, do_numainfo;
> > > > > +		bool do_softlimit;
> > > > >  
> > > > > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > > > -						MEM_CGROUP_TARGET_SOFTLIMIT);
> > > > >  #if MAX_NUMNODES > 1
> > > > > +		bool do_numainfo;
> > > > >  		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > > > >  						MEM_CGROUP_TARGET_NUMAINFO);
> > > > >  #endif
> > > > > +		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > > > +						MEM_CGROUP_TARGET_SOFTLIMIT);
> > > > 
> > > > I don't like this very much. Maybe we should get rid of both do_* and
> > > > do it with flags? But maybe it is not worth the additional code at
> > > > all...
> > > 
> > > Something like this (untested):
> > > ====
> > > From f57e1a2e1aaaa167c75b963d5bf12fcbdd3331b8 Mon Sep 17 00:00:00 2001
> > > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > Date: Tue, 27 Dec 2011 20:17:13 +0200
> > > Subject: [PATCH] memcg: cleanup memcg_check_events()
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> > 
> > The patch looks correct but I am still not sure this is worth fixing in
> > the code rather than disabling Wunused-variable.
> 
> Does it look better then original code from your POV?
> 
> It's a bit cleaner, I think.

Yes it is less hakish than the previous one which relied on having
#ifdef block before any other code in the block.

> 
> > 
> > > ---
> > >  mm/memcontrol.c |   42 ++++++++++++++++++++++++------------------
> > >  1 files changed, 24 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index d643bd6..40c2236 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -108,11 +108,12 @@ enum mem_cgroup_events_index {
> > >   * than using jiffies etc. to handle periodic memcg event.
> > >   */
> > >  enum mem_cgroup_events_target {
> > > -	MEM_CGROUP_TARGET_THRESH,
> > > -	MEM_CGROUP_TARGET_SOFTLIMIT,
> > > -	MEM_CGROUP_TARGET_NUMAINFO,
> > > -	MEM_CGROUP_NTARGETS,
> > > +	MEM_CGROUP_TARGET_THRESH	= BIT(1),
> > > +	MEM_CGROUP_TARGET_SOFTLIMIT	= BIT(2),
> > > +	MEM_CGROUP_TARGET_NUMAINFO	= BIT(3),
> > >  };
> > > +#define MEM_CGROUP_NTARGETS 3
> > > +
> > >  #define THRESHOLDS_EVENTS_TARGET (128)
> > >  #define SOFTLIMIT_EVENTS_TARGET (1024)
> > >  #define NUMAINFO_EVENTS_TARGET	(1024)
> > > @@ -743,7 +744,7 @@ static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
> > >  	return total;
> > >  }
> > >  
> > > -static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> > > +static int mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> > >  				       enum mem_cgroup_events_target target)
> > >  {
> > >  	unsigned long val, next;
> > > @@ -766,9 +767,9 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> > >  			break;
> > >  		}
> > >  		__this_cpu_write(memcg->stat->targets[target], next);
> > > -		return true;
> > > +		return target;
> > >  	}
> > > -	return false;
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -777,29 +778,34 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,
> > >   */
> > >  static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
> > >  {
> > > +	int flags;
> > > +
> > >  	preempt_disable();
> > > -	/* threshold event is triggered in finer grain than soft limit */
> > > -	if (unlikely(mem_cgroup_event_ratelimit(memcg,
> > > -						MEM_CGROUP_TARGET_THRESH))) {
> > > -		bool do_softlimit, do_numainfo;
> > > +	flags = mem_cgroup_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH);
> > >  
> > > -		do_softlimit = mem_cgroup_event_ratelimit(memcg,
> > > +	/*
> > > +	 * Threshold event is triggered in finer grain than soft limit
> > > +	 * and numainfo
> > > +	 */
> > > +	if (unlikely(flags)) {
> > > +		flags |= mem_cgroup_event_ratelimit(memcg,
> > >  						MEM_CGROUP_TARGET_SOFTLIMIT);
> > >  #if MAX_NUMNODES > 1
> > > -		do_numainfo = mem_cgroup_event_ratelimit(memcg,
> > > +		flags |= mem_cgroup_event_ratelimit(memcg,
> > >  						MEM_CGROUP_TARGET_NUMAINFO);
> > >  #endif
> > > -		preempt_enable();
> > > +	}
> > > +	preempt_enable();
> > >  
> > > +	if (unlikely(flags)) {
> > >  		mem_cgroup_threshold(memcg);
> > > -		if (unlikely(do_softlimit))
> > > +		if (unlikely(flags & MEM_CGROUP_TARGET_SOFTLIMIT))
> > >  			mem_cgroup_update_tree(memcg, page);
> > >  #if MAX_NUMNODES > 1
> > > -		if (unlikely(do_numainfo))
> > > +		if (unlikely(flags & MEM_CGROUP_TARGET_NUMAINFO))
> > >  			atomic_inc(&memcg->numainfo_events);
> > >  #endif
> > > -	} else
> > > -		preempt_enable();
> > > +	}
> > >  }
> > >  
> > >  struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
> > > -- 
> > > 1.7.7.3
> > 
> > -- 
> > Michal Hocko
> > SUSE Labs
> > SUSE LINUX s.r.o.
> > Lihovarska 1060/12
> > 190 00 Praha 9    
> > Czech Republic
> 
> -- 
>  Kirill A. Shutemov
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] Makefiles: Disable unused-variable warning

[Michal Marek]

Dne 27.12.2011 14:57, Michal Hocko napsal(a):
> Anyway, I am wondering why unused-but-set-variable is disabled while
> unused-variable is enabled.

unused-but-set-variable was disabled, because it was a new warning in
gcc 4.6 and produced too much noise relatively to its severity. A make
W=1 build of x86_64_defconfig gives:
$ grep -c 'Wunused-but-set-variable' log
77
$ grep -c 'Wunused-variable' log
0

More exotic configuration will probably result in a couple of unused
variable warnings, but that IMO no reason to disable them globally.

> Shouldn't we just disable it as well rather
> than workaround this in the code? The warning is just pure noise in this
> case.

If it's noise in a particular case, there is always the option to add

CFLAGS_memcontrol.o := $(call cc-disable-warning, unused-variable)

to the respective Makefile.

Michal

Re: [PATCH] Makefiles: Disable unused-variable warning

[Michal Hocko]

On Sun 08-01-12 16:01:17, Michal Marek wrote:
> Dne 27.12.2011 14:57, Michal Hocko napsal(a):
> > Anyway, I am wondering why unused-but-set-variable is disabled while
> > unused-variable is enabled.
> 
> unused-but-set-variable was disabled, because it was a new warning in
> gcc 4.6 and produced too much noise relatively to its severity. A make
> W=1 build of x86_64_defconfig gives:
> $ grep -c 'Wunused-but-set-variable' log
> 77
> $ grep -c 'Wunused-variable' log
> 0
> 
> More exotic configuration will probably result in a couple of unused
> variable warnings, but that IMO no reason to disable them globally.

OK.

> > Shouldn't we just disable it as well rather
> > than workaround this in the code? The warning is just pure noise in this
> > case.
> 
> If it's noise in a particular case, there is always the option to add
> 
> CFLAGS_memcontrol.o := $(call cc-disable-warning, unused-variable)

I would like to prevent from local cflags hacks. Moreover the code will
go away so I guess it doesn't make much sense to play tricks here.

> 
> to the respective Makefile.
> 
> Michal

Thanks

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