linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup
@ 2016-08-01 13:26 Vladimir Davydov
  2016-08-01 13:26 ` [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move Vladimir Davydov
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Vladimir Davydov @ 2016-08-01 13:26 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Johannes Weiner, Michal Hocko, linux-mm, linux-kernel

An offline memory cgroup might have anonymous memory or shmem left
charged to it and no swap. Since only swap entries pin the id of an
offline cgroup, such a cgroup will have no id and so an attempt to
swapout its anon/shmem will not store memory cgroup info in the swap
cgroup map. As a result, memcg->swap or memcg->memsw will never get
uncharged from it and any of its ascendants.

Fix this by always charging swapout to the first ancestor cgroup that
hasn't released its id yet.

Fixes: 73f576c04b941 ("mm: memcontrol: fix cgroup creation failure after many small jobs")
Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 mm/memcontrol.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b5804e4e6324..5fe285f27ea7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4035,6 +4035,13 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
 	atomic_inc(&memcg->id.ref);
 }
 
+static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
+{
+	while (!atomic_inc_not_zero(&memcg->id.ref))
+		memcg = parent_mem_cgroup(memcg);
+	return memcg;
+}
+
 static void mem_cgroup_id_put(struct mem_cgroup *memcg)
 {
 	if (atomic_dec_and_test(&memcg->id.ref)) {
@@ -5751,7 +5758,7 @@ subsys_initcall(mem_cgroup_init);
  */
 void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 {
-	struct mem_cgroup *memcg;
+	struct mem_cgroup *memcg, *swap_memcg;
 	unsigned short oldid;
 
 	VM_BUG_ON_PAGE(PageLRU(page), page);
@@ -5766,15 +5773,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	if (!memcg)
 		return;
 
-	mem_cgroup_id_get(memcg);
-	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
+	swap_memcg = mem_cgroup_id_get_active(memcg);
+	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
 	VM_BUG_ON_PAGE(oldid, page);
-	mem_cgroup_swap_statistics(memcg, true);
+	mem_cgroup_swap_statistics(swap_memcg, true);
 
 	page->mem_cgroup = NULL;
 
 	if (!mem_cgroup_is_root(memcg))
 		page_counter_uncharge(&memcg->memory, 1);
+	if (memcg != swap_memcg) {
+		if (!mem_cgroup_is_root(swap_memcg))
+			page_counter_charge(&swap_memcg->memsw, 1);
+		page_counter_uncharge(&memcg->memsw, 1);
+	}
 
 	/*
 	 * Interrupts should be disabled here because the caller holds the
@@ -5814,11 +5826,14 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 	if (!memcg)
 		return 0;
 
+	memcg = mem_cgroup_id_get_active(memcg);
+
 	if (!mem_cgroup_is_root(memcg) &&
-	    !page_counter_try_charge(&memcg->swap, 1, &counter))
+	    !page_counter_try_charge(&memcg->swap, 1, &counter)) {
+		mem_cgroup_id_put(memcg);
 		return -ENOMEM;
+	}
 
-	mem_cgroup_id_get(memcg);
 	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
 	VM_BUG_ON_PAGE(oldid, page);
 	mem_cgroup_swap_statistics(memcg, true);
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move
  2016-08-01 13:26 [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Vladimir Davydov
@ 2016-08-01 13:26 ` Vladimir Davydov
  2016-08-02 12:34   ` Michal Hocko
  2016-08-01 13:26 ` [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put Vladimir Davydov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Vladimir Davydov @ 2016-08-01 13:26 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Johannes Weiner, Michal Hocko, linux-mm, linux-kernel

Since commit 73f576c04b941 swap entries do not pin memcg->css.refcnt
directly. Instead, they pin memcg->id.ref. So we should adjust the
reference counters accordingly when moving swap charges between cgroups.

Fixes: 73f576c04b941 ("mm: memcontrol: fix cgroup creation failure after many small jobs")
Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 mm/memcontrol.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5fe285f27ea7..58c229071fb1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4030,9 +4030,9 @@ static struct cftype mem_cgroup_legacy_files[] = {
 
 static DEFINE_IDR(mem_cgroup_idr);
 
-static void mem_cgroup_id_get(struct mem_cgroup *memcg)
+static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
 {
-	atomic_inc(&memcg->id.ref);
+	atomic_add(n, &memcg->id.ref);
 }
 
 static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
@@ -4042,9 +4042,9 @@ static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
 	return memcg;
 }
 
-static void mem_cgroup_id_put(struct mem_cgroup *memcg)
+static void mem_cgroup_id_put_many(struct mem_cgroup *memcg, unsigned int n)
 {
-	if (atomic_dec_and_test(&memcg->id.ref)) {
+	if (atomic_sub_and_test(n, &memcg->id.ref)) {
 		idr_remove(&mem_cgroup_idr, memcg->id.id);
 		memcg->id.id = 0;
 
@@ -4053,6 +4053,16 @@ static void mem_cgroup_id_put(struct mem_cgroup *memcg)
 	}
 }
 
+static inline void mem_cgroup_id_get(struct mem_cgroup *memcg)
+{
+	mem_cgroup_id_get_many(memcg, 1);
+}
+
+static inline void mem_cgroup_id_put(struct mem_cgroup *memcg)
+{
+	mem_cgroup_id_put_many(memcg, 1);
+}
+
 /**
  * mem_cgroup_from_id - look up a memcg from a memcg id
  * @id: the memcg id to look up
@@ -4687,6 +4697,8 @@ static void __mem_cgroup_clear_mc(void)
 		if (!mem_cgroup_is_root(mc.from))
 			page_counter_uncharge(&mc.from->memsw, mc.moved_swap);
 
+		mem_cgroup_id_put_many(mc.from, mc.moved_swap);
+
 		/*
 		 * we charged both to->memory and to->memsw, so we
 		 * should uncharge to->memory.
@@ -4694,9 +4706,9 @@ static void __mem_cgroup_clear_mc(void)
 		if (!mem_cgroup_is_root(mc.to))
 			page_counter_uncharge(&mc.to->memory, mc.moved_swap);
 
-		css_put_many(&mc.from->css, mc.moved_swap);
+		mem_cgroup_id_get_many(mc.to, mc.moved_swap);
+		css_put_many(&mc.to->css, mc.moved_swap);
 
-		/* we've already done css_get(mc.to) */
 		mc.moved_swap = 0;
 	}
 	memcg_oom_recover(from);
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put
  2016-08-01 13:26 [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Vladimir Davydov
  2016-08-01 13:26 ` [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move Vladimir Davydov
@ 2016-08-01 13:26 ` Vladimir Davydov
  2016-08-02 12:45   ` Michal Hocko
  2016-08-02 12:23 ` [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Michal Hocko
  2016-08-02 12:42 ` Michal Hocko
  3 siblings, 1 reply; 9+ messages in thread
From: Vladimir Davydov @ 2016-08-01 13:26 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Johannes Weiner, Michal Hocko, linux-mm, linux-kernel

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 mm/memcontrol.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 58c229071fb1..cf7fb63860e5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4032,18 +4032,22 @@ static DEFINE_IDR(mem_cgroup_idr);
 
 static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
 {
+	VM_BUG_ON(atomic_read(&memcg->id.ref) <= 0);
 	atomic_add(n, &memcg->id.ref);
 }
 
 static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
 {
-	while (!atomic_inc_not_zero(&memcg->id.ref))
+	while (!atomic_inc_not_zero(&memcg->id.ref)) {
+		VM_BUG_ON(mem_cgroup_is_root(memcg));
 		memcg = parent_mem_cgroup(memcg);
+	}
 	return memcg;
 }
 
 static void mem_cgroup_id_put_many(struct mem_cgroup *memcg, unsigned int n)
 {
+	VM_BUG_ON(atomic_read(&memcg->id.ref) < n);
 	if (atomic_sub_and_test(n, &memcg->id.ref)) {
 		idr_remove(&mem_cgroup_idr, memcg->id.id);
 		memcg->id.id = 0;
@@ -4164,6 +4168,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
 	INIT_LIST_HEAD(&memcg->cgwb_list);
 #endif
 	idr_replace(&mem_cgroup_idr, memcg, memcg->id.id);
+	atomic_set(&memcg->id.ref, 1);
 	return memcg;
 fail:
 	if (memcg->id.id > 0)
@@ -4233,7 +4238,6 @@ fail:
 static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 {
 	/* Online state pins memcg ID, memcg ID pins CSS */
-	mem_cgroup_id_get(mem_cgroup_from_css(css));
 	css_get(css);
 	return 0;
 }
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup
  2016-08-01 13:26 [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Vladimir Davydov
  2016-08-01 13:26 ` [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move Vladimir Davydov
  2016-08-01 13:26 ` [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put Vladimir Davydov
@ 2016-08-02 12:23 ` Michal Hocko
  2016-08-02 12:42 ` Michal Hocko
  3 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2016-08-02 12:23 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel

On Mon 01-08-16 16:26:24, Vladimir Davydov wrote:
> An offline memory cgroup might have anonymous memory or shmem left
> charged to it and no swap. Since only swap entries pin the id of an
> offline cgroup, such a cgroup will have no id and so an attempt to
> swapout its anon/shmem will not store memory cgroup info in the swap
> cgroup map. As a result, memcg->swap or memcg->memsw will never get
> uncharged from it and any of its ascendants.

Ahh, OK, very good point! Multiple ref. counts are always very
confusing. I guess it would be good to mention the impact which would
be pre-mature memcg oom or excessive reclaim in deeper hierarchies with
the memsw limit or the swap limit enforced in a upper hierarchy AFAIU.
v2 swap limit would be little bit more subtle because it would prevent
swapout too early.

> Fix this by always charging swapout to the first ancestor cgroup that
> hasn't released its id yet.

This is a bit ugly but I guess the easiest way to fix this.

> Fixes: 73f576c04b941 ("mm: memcontrol: fix cgroup creation failure after many small jobs")

The original commit was marked for stable so this should go to stable as
well. I already have the above backported for 4.4 so weill queue this
one up as well when submitting.

> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>

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

Thanks!

> ---
>  mm/memcontrol.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b5804e4e6324..5fe285f27ea7 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4035,6 +4035,13 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
>  	atomic_inc(&memcg->id.ref);
>  }
>  
> +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> +{
> +	while (!atomic_inc_not_zero(&memcg->id.ref))
> +		memcg = parent_mem_cgroup(memcg);
> +	return memcg;
> +}
> +
>  static void mem_cgroup_id_put(struct mem_cgroup *memcg)
>  {
>  	if (atomic_dec_and_test(&memcg->id.ref)) {
> @@ -5751,7 +5758,7 @@ subsys_initcall(mem_cgroup_init);
>   */
>  void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  {
> -	struct mem_cgroup *memcg;
> +	struct mem_cgroup *memcg, *swap_memcg;
>  	unsigned short oldid;
>  
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
> @@ -5766,15 +5773,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	if (!memcg)
>  		return;
>  
> -	mem_cgroup_id_get(memcg);
> -	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> +	swap_memcg = mem_cgroup_id_get_active(memcg);
> +	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
>  	VM_BUG_ON_PAGE(oldid, page);
> -	mem_cgroup_swap_statistics(memcg, true);
> +	mem_cgroup_swap_statistics(swap_memcg, true);
>  
>  	page->mem_cgroup = NULL;
>  
>  	if (!mem_cgroup_is_root(memcg))
>  		page_counter_uncharge(&memcg->memory, 1);
> +	if (memcg != swap_memcg) {
> +		if (!mem_cgroup_is_root(swap_memcg))
> +			page_counter_charge(&swap_memcg->memsw, 1);
> +		page_counter_uncharge(&memcg->memsw, 1);
> +	}
>  
>  	/*
>  	 * Interrupts should be disabled here because the caller holds the
> @@ -5814,11 +5826,14 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>  	if (!memcg)
>  		return 0;
>  
> +	memcg = mem_cgroup_id_get_active(memcg);
> +
>  	if (!mem_cgroup_is_root(memcg) &&
> -	    !page_counter_try_charge(&memcg->swap, 1, &counter))
> +	    !page_counter_try_charge(&memcg->swap, 1, &counter)) {
> +		mem_cgroup_id_put(memcg);
>  		return -ENOMEM;
> +	}
>  
> -	mem_cgroup_id_get(memcg);
>  	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
>  	VM_BUG_ON_PAGE(oldid, page);
>  	mem_cgroup_swap_statistics(memcg, true);
> -- 
> 2.1.4
> 
> --
> 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>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move
  2016-08-01 13:26 ` [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move Vladimir Davydov
@ 2016-08-02 12:34   ` Michal Hocko
  0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2016-08-02 12:34 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel

On Mon 01-08-16 16:26:25, Vladimir Davydov wrote:
> Since commit 73f576c04b941 swap entries do not pin memcg->css.refcnt
> directly. Instead, they pin memcg->id.ref. So we should adjust the
> reference counters accordingly when moving swap charges between cgroups.
> 
> Fixes: 73f576c04b941 ("mm: memcontrol: fix cgroup creation failure after many small jobs")

Same as the previous patch. It should be marked for stable along with
73f576c04b941.

> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>

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

Thanks!

> ---
>  mm/memcontrol.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5fe285f27ea7..58c229071fb1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4030,9 +4030,9 @@ static struct cftype mem_cgroup_legacy_files[] = {
>  
>  static DEFINE_IDR(mem_cgroup_idr);
>  
> -static void mem_cgroup_id_get(struct mem_cgroup *memcg)
> +static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
>  {
> -	atomic_inc(&memcg->id.ref);
> +	atomic_add(n, &memcg->id.ref);
>  }
>  
>  static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> @@ -4042,9 +4042,9 @@ static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
>  	return memcg;
>  }
>  
> -static void mem_cgroup_id_put(struct mem_cgroup *memcg)
> +static void mem_cgroup_id_put_many(struct mem_cgroup *memcg, unsigned int n)
>  {
> -	if (atomic_dec_and_test(&memcg->id.ref)) {
> +	if (atomic_sub_and_test(n, &memcg->id.ref)) {
>  		idr_remove(&mem_cgroup_idr, memcg->id.id);
>  		memcg->id.id = 0;
>  
> @@ -4053,6 +4053,16 @@ static void mem_cgroup_id_put(struct mem_cgroup *memcg)
>  	}
>  }
>  
> +static inline void mem_cgroup_id_get(struct mem_cgroup *memcg)
> +{
> +	mem_cgroup_id_get_many(memcg, 1);
> +}
> +
> +static inline void mem_cgroup_id_put(struct mem_cgroup *memcg)
> +{
> +	mem_cgroup_id_put_many(memcg, 1);
> +}
> +
>  /**
>   * mem_cgroup_from_id - look up a memcg from a memcg id
>   * @id: the memcg id to look up
> @@ -4687,6 +4697,8 @@ static void __mem_cgroup_clear_mc(void)
>  		if (!mem_cgroup_is_root(mc.from))
>  			page_counter_uncharge(&mc.from->memsw, mc.moved_swap);
>  
> +		mem_cgroup_id_put_many(mc.from, mc.moved_swap);
> +
>  		/*
>  		 * we charged both to->memory and to->memsw, so we
>  		 * should uncharge to->memory.
> @@ -4694,9 +4706,9 @@ static void __mem_cgroup_clear_mc(void)
>  		if (!mem_cgroup_is_root(mc.to))
>  			page_counter_uncharge(&mc.to->memory, mc.moved_swap);
>  
> -		css_put_many(&mc.from->css, mc.moved_swap);
> +		mem_cgroup_id_get_many(mc.to, mc.moved_swap);
> +		css_put_many(&mc.to->css, mc.moved_swap);
>  
> -		/* we've already done css_get(mc.to) */
>  		mc.moved_swap = 0;
>  	}
>  	memcg_oom_recover(from);
> -- 
> 2.1.4
> 
> --
> 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>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup
  2016-08-01 13:26 [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Vladimir Davydov
                   ` (2 preceding siblings ...)
  2016-08-02 12:23 ` [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Michal Hocko
@ 2016-08-02 12:42 ` Michal Hocko
  2016-08-02 13:22   ` Vladimir Davydov
  3 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2016-08-02 12:42 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel


On Mon 01-08-16 16:26:24, Vladimir Davydov wrote:
[...]
> +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> +{
> +	while (!atomic_inc_not_zero(&memcg->id.ref))
> +		memcg = parent_mem_cgroup(memcg);
> +	return memcg;
> +}

Does this actually work properly? Say we have root -> A so parent is
NULL if root (use_hierarchy == false).
-- 
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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put
  2016-08-01 13:26 ` [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put Vladimir Davydov
@ 2016-08-02 12:45   ` Michal Hocko
  2016-08-02 15:51     ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2016-08-02 12:45 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel

On Mon 01-08-16 16:26:26, Vladimir Davydov wrote:
[...]
>  static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
>  {
> -	while (!atomic_inc_not_zero(&memcg->id.ref))
> +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> +		VM_BUG_ON(mem_cgroup_is_root(memcg));
>  		memcg = parent_mem_cgroup(memcg);
> +	}

why is this BUG_ON ok? What if the root.use_hierarchy=true?

>  	return memcg;
>  }
-- 
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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup
  2016-08-02 12:42 ` Michal Hocko
@ 2016-08-02 13:22   ` Vladimir Davydov
  0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Davydov @ 2016-08-02 13:22 UTC (permalink / raw)
  To: Michal Hocko; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel

On Tue, Aug 02, 2016 at 02:42:32PM +0200, Michal Hocko wrote:
> 
> On Mon 01-08-16 16:26:24, Vladimir Davydov wrote:
> [...]
> > +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> > +{
> > +	while (!atomic_inc_not_zero(&memcg->id.ref))
> > +		memcg = parent_mem_cgroup(memcg);
> > +	return memcg;
> > +}
> 
> Does this actually work properly? Say we have root -> A so parent is
> NULL if root (use_hierarchy == false).

Yeah, I completely forgot about the !use_hierarchy case. Thanks for
catching this. I'll fix and resend.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put
  2016-08-02 12:45   ` Michal Hocko
@ 2016-08-02 15:51     ` Michal Hocko
  0 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2016-08-02 15:51 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Andrew Morton, Johannes Weiner, linux-mm, linux-kernel

On Tue 02-08-16 14:45:03, Michal Hocko wrote:
> On Mon 01-08-16 16:26:26, Vladimir Davydov wrote:
> [...]
> >  static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> >  {
> > -	while (!atomic_inc_not_zero(&memcg->id.ref))
> > +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> > +		VM_BUG_ON(mem_cgroup_is_root(memcg));
> >  		memcg = parent_mem_cgroup(memcg);
> > +	}
> 
> why is this BUG_ON ok? What if the root.use_hierarchy=true?

Scratch that. I got it. root will never get its count down to 0. Sorry
about the noise.
-- 
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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-08-02 15:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-01 13:26 [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Vladimir Davydov
2016-08-01 13:26 ` [PATCH 2/3] mm: memcontrol: fix memcg id ref counter on swap charge move Vladimir Davydov
2016-08-02 12:34   ` Michal Hocko
2016-08-01 13:26 ` [PATCH 3/3] mm: memcontrol: add sanity checks for memcg->id.ref on get/put Vladimir Davydov
2016-08-02 12:45   ` Michal Hocko
2016-08-02 15:51     ` Michal Hocko
2016-08-02 12:23 ` [PATCH 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Michal Hocko
2016-08-02 12:42 ` Michal Hocko
2016-08-02 13:22   ` Vladimir Davydov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).