[PATCH -mm 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[PATCH -mm 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[Tejun Heo]

From 92c2a5d90ecc5eeed0224a8f6ba533c621ac3ffa Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Tue, 2 Jun 2015 09:29:11 -0400

Since 4942642080ea ("mm: memcg: handle non-error OOM situations more
gracefully"), nobody uses mem_cgroup->oom_wakeups.  Remove it.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 mm/memcontrol.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 86648a7..9f39647 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -287,7 +287,6 @@ struct mem_cgroup {
 
 	bool		oom_lock;
 	atomic_t	under_oom;
-	atomic_t	oom_wakeups;
 
 	int	swappiness;
 	/* OOM-Killer disable */
@@ -1852,7 +1851,6 @@ static int memcg_oom_wake_function(wait_queue_t *wait,
 
 static void memcg_wakeup_oom(struct mem_cgroup *memcg)
 {
-	atomic_inc(&memcg->oom_wakeups);
 	/* for filtering, pass "memcg" as argument. */
 	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
 }
-- 
2.4.2

[PATCH -mm 2/2] memcg: convert mem_cgroup->under_oom from atomic_t to int

[Tejun Heo]

From 5456f353297d6f10b45fd794674b09dd5ab502ca Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Tue, 2 Jun 2015 09:29:11 -0400

memcg->under_oom tracks whether the memcg is under OOM conditions and
is an atomic_t counter managed with mem_cgroup_[un]mark_under_oom().
While atomic_t appears to be simple synchronization-wise, when used as
a synchronization construct like here, it's trickier and more
error-prone due to weak memory ordering rules, especially around
atomic_read(), and false sense of security.

For example, both non-trivial read sites of memcg->under_oom are a bit
problematic although not being actually broken.

* mem_cgroup_oom_register_event()

  It isn't explicit what guarantees the memory ordering between event
  addition and memcg->under_oom check.  This isn't broken only because
  memcg_oom_lock is used for both event list and memcg->oom_lock.

* memcg_oom_recover()

  The lockless test doesn't have any explanation why this would be
  safe.

mem_cgroup_[un]mark_under_oom() are very cold paths and there's no
point in avoiding locking memcg_oom_lock there.  This patch converts
memcg->under_oom from atomic_t to int, puts their modifications under
memcg_oom_lock and documents why the lockless test in
memcg_oom_recover() is safe.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 mm/memcontrol.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9f39647..4de6647 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -285,8 +285,9 @@ struct mem_cgroup {
 	 */
 	bool use_hierarchy;
 
+	/* protected by memcg_oom_lock */
 	bool		oom_lock;
-	atomic_t	under_oom;
+	int		under_oom;
 
 	int	swappiness;
 	/* OOM-Killer disable */
@@ -1809,8 +1810,10 @@ static void mem_cgroup_mark_under_oom(struct mem_cgroup *memcg)
 {
 	struct mem_cgroup *iter;
 
+	spin_lock(&memcg_oom_lock);
 	for_each_mem_cgroup_tree(iter, memcg)
-		atomic_inc(&iter->under_oom);
+		iter->under_oom++;
+	spin_unlock(&memcg_oom_lock);
 }
 
 static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
@@ -1819,11 +1822,13 @@ static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
 
 	/*
 	 * When a new child is created while the hierarchy is under oom,
-	 * mem_cgroup_oom_lock() may not be called. We have to use
-	 * atomic_add_unless() here.
+	 * mem_cgroup_oom_lock() may not be called. Watch for underflow.
 	 */
+	spin_lock(&memcg_oom_lock);
 	for_each_mem_cgroup_tree(iter, memcg)
-		atomic_add_unless(&iter->under_oom, -1, 0);
+		if (iter->under_oom > 0)
+			iter->under_oom--;
+	spin_unlock(&memcg_oom_lock);
 }
 
 static DECLARE_WAIT_QUEUE_HEAD(memcg_oom_waitq);
@@ -1857,7 +1862,15 @@ static void memcg_wakeup_oom(struct mem_cgroup *memcg)
 
 static void memcg_oom_recover(struct mem_cgroup *memcg)
 {
-	if (memcg && atomic_read(&memcg->under_oom))
+	/*
+	 * For the following lockless ->under_oom test, the only required
+	 * guarantee is that it must see the state asserted by an OOM when
+	 * this function is called as a result of userland actions
+	 * triggered by the notification of the OOM.  This is trivially
+	 * achieved by invoking mem_cgroup_mark_under_oom() before
+	 * triggering notification.
+	 */
+	if (memcg && memcg->under_oom)
 		memcg_wakeup_oom(memcg);
 }
 
@@ -3866,7 +3879,7 @@ static int mem_cgroup_oom_register_event(struct mem_cgroup *memcg,
 	list_add(&event->list, &memcg->oom_notify);
 
 	/* already in OOM ? */
-	if (atomic_read(&memcg->under_oom))
+	if (memcg->under_oom)
 		eventfd_signal(eventfd, 1);
 	spin_unlock(&memcg_oom_lock);
 
@@ -3895,7 +3908,7 @@ static int mem_cgroup_oom_control_read(struct seq_file *sf, void *v)
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
 
 	seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable);
-	seq_printf(sf, "under_oom %d\n", (bool)atomic_read(&memcg->under_oom));
+	seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
 	return 0;
 }
 
-- 
2.4.2

Re: [PATCH -mm 2/2] memcg: convert mem_cgroup->under_oom from atomic_t to int

[Michal Hocko]

On Wed 03-06-15 11:38:59, Tejun Heo wrote:
> From 5456f353297d6f10b45fd794674b09dd5ab502ca Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 2 Jun 2015 09:29:11 -0400
> 
> memcg->under_oom tracks whether the memcg is under OOM conditions and
> is an atomic_t counter managed with mem_cgroup_[un]mark_under_oom().
> While atomic_t appears to be simple synchronization-wise, when used as
> a synchronization construct like here, it's trickier and more
> error-prone due to weak memory ordering rules, especially around
> atomic_read(), and false sense of security.
> 
> For example, both non-trivial read sites of memcg->under_oom are a bit
> problematic although not being actually broken.
> 
> * mem_cgroup_oom_register_event()
> 
>   It isn't explicit what guarantees the memory ordering between event
>   addition and memcg->under_oom check.  This isn't broken only because
>   memcg_oom_lock is used for both event list and memcg->oom_lock.
> 
> * memcg_oom_recover()
> 
>   The lockless test doesn't have any explanation why this would be
>   safe.
> 
> mem_cgroup_[un]mark_under_oom() are very cold paths and there's no
> point in avoiding locking memcg_oom_lock there.  This patch converts
> memcg->under_oom from atomic_t to int, puts their modifications under
> memcg_oom_lock and documents why the lockless test in
> memcg_oom_recover() is safe.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>

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

> ---
>  mm/memcontrol.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9f39647..4de6647 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -285,8 +285,9 @@ struct mem_cgroup {
>  	 */
>  	bool use_hierarchy;
>  
> +	/* protected by memcg_oom_lock */
>  	bool		oom_lock;
> -	atomic_t	under_oom;
> +	int		under_oom;
>  
>  	int	swappiness;
>  	/* OOM-Killer disable */
> @@ -1809,8 +1810,10 @@ static void mem_cgroup_mark_under_oom(struct mem_cgroup *memcg)
>  {
>  	struct mem_cgroup *iter;
>  
> +	spin_lock(&memcg_oom_lock);
>  	for_each_mem_cgroup_tree(iter, memcg)
> -		atomic_inc(&iter->under_oom);
> +		iter->under_oom++;
> +	spin_unlock(&memcg_oom_lock);
>  }
>  
>  static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
> @@ -1819,11 +1822,13 @@ static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
>  
>  	/*
>  	 * When a new child is created while the hierarchy is under oom,
> -	 * mem_cgroup_oom_lock() may not be called. We have to use
> -	 * atomic_add_unless() here.
> +	 * mem_cgroup_oom_lock() may not be called. Watch for underflow.
>  	 */
> +	spin_lock(&memcg_oom_lock);
>  	for_each_mem_cgroup_tree(iter, memcg)
> -		atomic_add_unless(&iter->under_oom, -1, 0);
> +		if (iter->under_oom > 0)
> +			iter->under_oom--;
> +	spin_unlock(&memcg_oom_lock);
>  }
>  
>  static DECLARE_WAIT_QUEUE_HEAD(memcg_oom_waitq);
> @@ -1857,7 +1862,15 @@ static void memcg_wakeup_oom(struct mem_cgroup *memcg)
>  
>  static void memcg_oom_recover(struct mem_cgroup *memcg)
>  {
> -	if (memcg && atomic_read(&memcg->under_oom))
> +	/*
> +	 * For the following lockless ->under_oom test, the only required
> +	 * guarantee is that it must see the state asserted by an OOM when
> +	 * this function is called as a result of userland actions
> +	 * triggered by the notification of the OOM.  This is trivially
> +	 * achieved by invoking mem_cgroup_mark_under_oom() before
> +	 * triggering notification.
> +	 */
> +	if (memcg && memcg->under_oom)
>  		memcg_wakeup_oom(memcg);
>  }
>  
> @@ -3866,7 +3879,7 @@ static int mem_cgroup_oom_register_event(struct mem_cgroup *memcg,
>  	list_add(&event->list, &memcg->oom_notify);
>  
>  	/* already in OOM ? */
> -	if (atomic_read(&memcg->under_oom))
> +	if (memcg->under_oom)
>  		eventfd_signal(eventfd, 1);
>  	spin_unlock(&memcg_oom_lock);
>  
> @@ -3895,7 +3908,7 @@ static int mem_cgroup_oom_control_read(struct seq_file *sf, void *v)
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
>  
>  	seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable);
> -	seq_printf(sf, "under_oom %d\n", (bool)atomic_read(&memcg->under_oom));
> +	seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
>  	return 0;
>  }
>  
> -- 
> 2.4.2
> 

-- 
Michal Hocko
SUSE Labs

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

[PATCH v2 -mm 2/2] memcg: convert mem_cgroup->under_oom from atomic_t to int

[Tejun Heo]

memcg->under_oom tracks whether the memcg is under OOM conditions and
is an atomic_t counter managed with mem_cgroup_[un]mark_under_oom().
While atomic_t appears to be simple synchronization-wise, when used as
a synchronization construct like here, it's trickier and more
error-prone due to weak memory ordering rules, especially around
atomic_read(), and false sense of security.

For example, both non-trivial read sites of memcg->under_oom are a bit
problematic although not being actually broken.

* mem_cgroup_oom_register_event()

  It isn't explicit what guarantees the memory ordering between event
  addition and memcg->under_oom check.  This isn't broken only because
  memcg_oom_lock is used for both event list and memcg->oom_lock.

* memcg_oom_recover()

  The lockless test doesn't have any explanation why this would be
  safe.

mem_cgroup_[un]mark_under_oom() are very cold paths and there's no
point in avoiding locking memcg_oom_lock there.  This patch converts
memcg->under_oom from atomic_t to int, puts their modifications under
memcg_oom_lock and documents why the lockless test in
memcg_oom_recover() is safe.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Update of the 1/2 patch causes a trivial context conflict.  Refreshed.

Thanks.

 mm/memcontrol.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -285,8 +285,9 @@ struct mem_cgroup {
 	 */
 	bool use_hierarchy;
 
+	/* protected by memcg_oom_lock */
 	bool		oom_lock;
-	atomic_t	under_oom;
+	int		under_oom;
 
 	int	swappiness;
 	/* OOM-Killer disable */
@@ -1809,8 +1810,10 @@ static void mem_cgroup_mark_under_oom(st
 {
 	struct mem_cgroup *iter;
 
+	spin_lock(&memcg_oom_lock);
 	for_each_mem_cgroup_tree(iter, memcg)
-		atomic_inc(&iter->under_oom);
+		iter->under_oom++;
+	spin_unlock(&memcg_oom_lock);
 }
 
 static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
@@ -1819,11 +1822,13 @@ static void mem_cgroup_unmark_under_oom(
 
 	/*
 	 * When a new child is created while the hierarchy is under oom,
-	 * mem_cgroup_oom_lock() may not be called. We have to use
-	 * atomic_add_unless() here.
+	 * mem_cgroup_oom_lock() may not be called. Watch for underflow.
 	 */
+	spin_lock(&memcg_oom_lock);
 	for_each_mem_cgroup_tree(iter, memcg)
-		atomic_add_unless(&iter->under_oom, -1, 0);
+		if (iter->under_oom > 0)
+			iter->under_oom--;
+	spin_unlock(&memcg_oom_lock);
 }
 
 static DECLARE_WAIT_QUEUE_HEAD(memcg_oom_waitq);
@@ -1851,7 +1856,15 @@ static int memcg_oom_wake_function(wait_
 
 static void memcg_oom_recover(struct mem_cgroup *memcg)
 {
-	if (memcg && atomic_read(&memcg->under_oom))
+	/*
+	 * For the following lockless ->under_oom test, the only required
+	 * guarantee is that it must see the state asserted by an OOM when
+	 * this function is called as a result of userland actions
+	 * triggered by the notification of the OOM.  This is trivially
+	 * achieved by invoking mem_cgroup_mark_under_oom() before
+	 * triggering notification.
+	 */
+	if (memcg && memcg->under_oom)
 		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
 }
 
@@ -3860,7 +3873,7 @@ static int mem_cgroup_oom_register_event
 	list_add(&event->list, &memcg->oom_notify);
 
 	/* already in OOM ? */
-	if (atomic_read(&memcg->under_oom))
+	if (memcg->under_oom)
 		eventfd_signal(eventfd, 1);
 	spin_unlock(&memcg_oom_lock);
 
@@ -3889,7 +3902,7 @@ static int mem_cgroup_oom_control_read(s
 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
 
 	seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable);
-	seq_printf(sf, "under_oom %d\n", (bool)atomic_read(&memcg->under_oom));
+	seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
 	return 0;
 }
 

--
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 v2 -mm 2/2] memcg: convert mem_cgroup->under_oom from atomic_t to int

[Michal Hocko]

On Thu 04-06-15 00:21:01, Tejun Heo wrote:
> memcg->under_oom tracks whether the memcg is under OOM conditions and
> is an atomic_t counter managed with mem_cgroup_[un]mark_under_oom().
> While atomic_t appears to be simple synchronization-wise, when used as
> a synchronization construct like here, it's trickier and more
> error-prone due to weak memory ordering rules, especially around
> atomic_read(), and false sense of security.
> 
> For example, both non-trivial read sites of memcg->under_oom are a bit
> problematic although not being actually broken.
> 
> * mem_cgroup_oom_register_event()
> 
>   It isn't explicit what guarantees the memory ordering between event
>   addition and memcg->under_oom check.  This isn't broken only because
>   memcg_oom_lock is used for both event list and memcg->oom_lock.
> 
> * memcg_oom_recover()
> 
>   The lockless test doesn't have any explanation why this would be
>   safe.
> 
> mem_cgroup_[un]mark_under_oom() are very cold paths and there's no
> point in avoiding locking memcg_oom_lock there.  This patch converts
> memcg->under_oom from atomic_t to int, puts their modifications under
> memcg_oom_lock and documents why the lockless test in
> memcg_oom_recover() is safe.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

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

> ---
> Update of the 1/2 patch causes a trivial context conflict.  Refreshed.
> 
> Thanks.
> 
>  mm/memcontrol.c |   29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -285,8 +285,9 @@ struct mem_cgroup {
>  	 */
>  	bool use_hierarchy;
>  
> +	/* protected by memcg_oom_lock */
>  	bool		oom_lock;
> -	atomic_t	under_oom;
> +	int		under_oom;
>  
>  	int	swappiness;
>  	/* OOM-Killer disable */
> @@ -1809,8 +1810,10 @@ static void mem_cgroup_mark_under_oom(st
>  {
>  	struct mem_cgroup *iter;
>  
> +	spin_lock(&memcg_oom_lock);
>  	for_each_mem_cgroup_tree(iter, memcg)
> -		atomic_inc(&iter->under_oom);
> +		iter->under_oom++;
> +	spin_unlock(&memcg_oom_lock);
>  }
>  
>  static void mem_cgroup_unmark_under_oom(struct mem_cgroup *memcg)
> @@ -1819,11 +1822,13 @@ static void mem_cgroup_unmark_under_oom(
>  
>  	/*
>  	 * When a new child is created while the hierarchy is under oom,
> -	 * mem_cgroup_oom_lock() may not be called. We have to use
> -	 * atomic_add_unless() here.
> +	 * mem_cgroup_oom_lock() may not be called. Watch for underflow.
>  	 */
> +	spin_lock(&memcg_oom_lock);
>  	for_each_mem_cgroup_tree(iter, memcg)
> -		atomic_add_unless(&iter->under_oom, -1, 0);
> +		if (iter->under_oom > 0)
> +			iter->under_oom--;
> +	spin_unlock(&memcg_oom_lock);
>  }
>  
>  static DECLARE_WAIT_QUEUE_HEAD(memcg_oom_waitq);
> @@ -1851,7 +1856,15 @@ static int memcg_oom_wake_function(wait_
>  
>  static void memcg_oom_recover(struct mem_cgroup *memcg)
>  {
> -	if (memcg && atomic_read(&memcg->under_oom))
> +	/*
> +	 * For the following lockless ->under_oom test, the only required
> +	 * guarantee is that it must see the state asserted by an OOM when
> +	 * this function is called as a result of userland actions
> +	 * triggered by the notification of the OOM.  This is trivially
> +	 * achieved by invoking mem_cgroup_mark_under_oom() before
> +	 * triggering notification.
> +	 */
> +	if (memcg && memcg->under_oom)
>  		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
>  }
>  
> @@ -3860,7 +3873,7 @@ static int mem_cgroup_oom_register_event
>  	list_add(&event->list, &memcg->oom_notify);
>  
>  	/* already in OOM ? */
> -	if (atomic_read(&memcg->under_oom))
> +	if (memcg->under_oom)
>  		eventfd_signal(eventfd, 1);
>  	spin_unlock(&memcg_oom_lock);
>  
> @@ -3889,7 +3902,7 @@ static int mem_cgroup_oom_control_read(s
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
>  
>  	seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable);
> -	seq_printf(sf, "under_oom %d\n", (bool)atomic_read(&memcg->under_oom));
> +	seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
>  	return 0;
>  }
>  

-- 
Michal Hocko
SUSE Labs

--
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 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[Michal Hocko]

On Wed 03-06-15 11:38:24, Tejun Heo wrote:
> From 92c2a5d90ecc5eeed0224a8f6ba533c621ac3ffa Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Tue, 2 Jun 2015 09:29:11 -0400
> 
> Since 4942642080ea ("mm: memcg: handle non-error OOM situations more
> gracefully"), nobody uses mem_cgroup->oom_wakeups.  Remove it.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

> ---
>  mm/memcontrol.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 86648a7..9f39647 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -287,7 +287,6 @@ struct mem_cgroup {
>  
>  	bool		oom_lock;
>  	atomic_t	under_oom;
> -	atomic_t	oom_wakeups;
>  
>  	int	swappiness;
>  	/* OOM-Killer disable */
> @@ -1852,7 +1851,6 @@ static int memcg_oom_wake_function(wait_queue_t *wait,
>  
>  static void memcg_wakeup_oom(struct mem_cgroup *memcg)
>  {
> -	atomic_inc(&memcg->oom_wakeups);
>  	/* for filtering, pass "memcg" as argument. */
>  	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
>  }
> -- 
> 2.4.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[Michal Hocko]

On Wed 03-06-15 15:48:30, Michal Hocko wrote:
> On Wed 03-06-15 11:38:24, Tejun Heo wrote:
> > From 92c2a5d90ecc5eeed0224a8f6ba533c621ac3ffa Mon Sep 17 00:00:00 2001
> > From: Tejun Heo <tj@kernel.org>
> > Date: Tue, 2 Jun 2015 09:29:11 -0400
> > 
> > Since 4942642080ea ("mm: memcg: handle non-error OOM situations more
> > gracefully"), nobody uses mem_cgroup->oom_wakeups.  Remove it.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> Acked-by: Michal Hocko <mhocko@suse.cz>

Could you also inline __wake_up from memcg_wakeup_oom into its only
caller while you are touching that code, please?

> 
> > ---
> >  mm/memcontrol.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 86648a7..9f39647 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -287,7 +287,6 @@ struct mem_cgroup {
> >  
> >  	bool		oom_lock;
> >  	atomic_t	under_oom;
> > -	atomic_t	oom_wakeups;
> >  
> >  	int	swappiness;
> >  	/* OOM-Killer disable */
> > @@ -1852,7 +1851,6 @@ static int memcg_oom_wake_function(wait_queue_t *wait,
> >  
> >  static void memcg_wakeup_oom(struct mem_cgroup *memcg)
> >  {
> > -	atomic_inc(&memcg->oom_wakeups);
> >  	/* for filtering, pass "memcg" as argument. */
> >  	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
> >  }
> > -- 
> > 2.4.2
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[PATCH v2 -mm 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[Tejun Heo]

Since 4942642080ea ("mm: memcg: handle non-error OOM situations more
gracefully"), nobody uses mem_cgroup->oom_wakeups.  Remove it.

While at it, also fold memcg_wakeup_oom() into memcg_oom_recover()
which is its only user.  This cleanup was suggested by Michal.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
---
Patch updated.  I dropped the comment as it's kinda obvious from the
context and the use of __wake_up().

Thanks.

 mm/memcontrol.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -287,7 +287,6 @@ struct mem_cgroup {
 
 	bool		oom_lock;
 	atomic_t	under_oom;
-	atomic_t	oom_wakeups;
 
 	int	swappiness;
 	/* OOM-Killer disable */
@@ -1850,17 +1849,10 @@ static int memcg_oom_wake_function(wait_
 	return autoremove_wake_function(wait, mode, sync, arg);
 }
 
-static void memcg_wakeup_oom(struct mem_cgroup *memcg)
-{
-	atomic_inc(&memcg->oom_wakeups);
-	/* for filtering, pass "memcg" as argument. */
-	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
-}
-
 static void memcg_oom_recover(struct mem_cgroup *memcg)
 {
 	if (memcg && atomic_read(&memcg->under_oom))
-		memcg_wakeup_oom(memcg);
+		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
 }
 
 static void mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order)

Re: [PATCH v2 -mm 1/2] memcg: remove unused mem_cgroup->oom_wakeups

[Michal Hocko]

On Thu 04-06-15 00:19:53, Tejun Heo wrote:
> Since 4942642080ea ("mm: memcg: handle non-error OOM situations more
> gracefully"), nobody uses mem_cgroup->oom_wakeups.  Remove it.
> 
> While at it, also fold memcg_wakeup_oom() into memcg_oom_recover()
> which is its only user.  This cleanup was suggested by Michal.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Michal Hocko <mhocko@suse.cz>

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

Thanks!

> ---
> Patch updated.  I dropped the comment as it's kinda obvious from the
> context and the use of __wake_up().
> 
> Thanks.
> 
>  mm/memcontrol.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -287,7 +287,6 @@ struct mem_cgroup {
>  
>  	bool		oom_lock;
>  	atomic_t	under_oom;
> -	atomic_t	oom_wakeups;
>  
>  	int	swappiness;
>  	/* OOM-Killer disable */
> @@ -1850,17 +1849,10 @@ static int memcg_oom_wake_function(wait_
>  	return autoremove_wake_function(wait, mode, sync, arg);
>  }
>  
> -static void memcg_wakeup_oom(struct mem_cgroup *memcg)
> -{
> -	atomic_inc(&memcg->oom_wakeups);
> -	/* for filtering, pass "memcg" as argument. */
> -	__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
> -}
> -
>  static void memcg_oom_recover(struct mem_cgroup *memcg)
>  {
>  	if (memcg && atomic_read(&memcg->under_oom))
> -		memcg_wakeup_oom(memcg);
> +		__wake_up(&memcg_oom_waitq, TASK_NORMAL, 0, memcg);
>  }
>  
>  static void mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order)
> --
> 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

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