All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
@ 2018-10-09 17:46 ` Kirill Tkhai
  0 siblings, 0 replies; 6+ messages in thread
From: Kirill Tkhai @ 2018-10-09 17:46 UTC (permalink / raw)
  To: akpm, hannes, mhocko, vdavydov.dev, akpm, ktkhai, linux-kernel, linux-mm

This will allow to use generic refcount_t interfaces
to check counters overflow instead of currently existing
VM_BUG_ON(). The only difference after the patch is
VM_BUG_ON() may cause BUG(), while refcount_t fires
with WARN(). But this seems not to be significant here,
since such the problems are usually caught by syzbot
with panic-on-warn enabled.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 include/linux/memcontrol.h |    2 +-
 mm/memcontrol.c            |   10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 4399cc3f00e4..7ab2120155a4 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -78,7 +78,7 @@ struct mem_cgroup_reclaim_cookie {
 
 struct mem_cgroup_id {
 	int id;
-	atomic_t ref;
+	refcount_t ref;
 };
 
 /*
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7bebe2ddec05..aa728d5b3d72 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4299,14 +4299,12 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
 
 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);
+	refcount_add(n, &memcg->id.ref);
 }
 
 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)) {
+	if (refcount_sub_and_test(n, &memcg->id.ref)) {
 		mem_cgroup_id_remove(memcg);
 
 		/* Memcg ID pins CSS */
@@ -4523,7 +4521,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	}
 
 	/* Online state pins memcg ID, memcg ID pins CSS */
-	atomic_set(&memcg->id.ref, 1);
+	refcount_set(&memcg->id.ref, 1);
 	css_get(css);
 	return 0;
 }
@@ -6357,7 +6355,7 @@ subsys_initcall(mem_cgroup_init);
 #ifdef CONFIG_MEMCG_SWAP
 static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
 {
-	while (!atomic_inc_not_zero(&memcg->id.ref)) {
+	while (!refcount_inc_not_zero(&memcg->id.ref)) {
 		/*
 		 * The root cgroup cannot be destroyed, so it's refcount must
 		 * always be >= 1.


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
@ 2018-10-09 17:46 ` Kirill Tkhai
  0 siblings, 0 replies; 6+ messages in thread
From: Kirill Tkhai @ 2018-10-09 17:46 UTC (permalink / raw)
  To: akpm, hannes, mhocko, vdavydov.dev

This will allow to use generic refcount_t interfaces
to check counters overflow instead of currently existing
VM_BUG_ON(). The only difference after the patch is
VM_BUG_ON() may cause BUG(), while refcount_t fires
with WARN(). But this seems not to be significant here,
since such the problems are usually caught by syzbot
with panic-on-warn enabled.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 include/linux/memcontrol.h |    2 +-
 mm/memcontrol.c            |   10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 4399cc3f00e4..7ab2120155a4 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -78,7 +78,7 @@ struct mem_cgroup_reclaim_cookie {
 
 struct mem_cgroup_id {
 	int id;
-	atomic_t ref;
+	refcount_t ref;
 };
 
 /*
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7bebe2ddec05..aa728d5b3d72 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4299,14 +4299,12 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
 
 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);
+	refcount_add(n, &memcg->id.ref);
 }
 
 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)) {
+	if (refcount_sub_and_test(n, &memcg->id.ref)) {
 		mem_cgroup_id_remove(memcg);
 
 		/* Memcg ID pins CSS */
@@ -4523,7 +4521,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	}
 
 	/* Online state pins memcg ID, memcg ID pins CSS */
-	atomic_set(&memcg->id.ref, 1);
+	refcount_set(&memcg->id.ref, 1);
 	css_get(css);
 	return 0;
 }
@@ -6357,7 +6355,7 @@ subsys_initcall(mem_cgroup_init);
 #ifdef CONFIG_MEMCG_SWAP
 static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
 {
-	while (!atomic_inc_not_zero(&memcg->id.ref)) {
+	while (!refcount_inc_not_zero(&memcg->id.ref)) {
 		/*
 		 * The root cgroup cannot be destroyed, so it's refcount must
 		 * always be >= 1.

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
  2018-10-09 17:46 ` Kirill Tkhai
  (?)
@ 2018-10-16 12:05 ` Michal Hocko
  -1 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2018-10-16 12:05 UTC (permalink / raw)
  To: Kirill Tkhai; +Cc: akpm, hannes, vdavydov.dev, linux-kernel, linux-mm

On Tue 09-10-18 20:46:56, Kirill Tkhai wrote:
> This will allow to use generic refcount_t interfaces
> to check counters overflow instead of currently existing
> VM_BUG_ON(). The only difference after the patch is
> VM_BUG_ON() may cause BUG(), while refcount_t fires
> with WARN(). But this seems not to be significant here,
> since such the problems are usually caught by syzbot
> with panic-on-warn enabled.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

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

> ---
>  include/linux/memcontrol.h |    2 +-
>  mm/memcontrol.c            |   10 ++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 4399cc3f00e4..7ab2120155a4 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -78,7 +78,7 @@ struct mem_cgroup_reclaim_cookie {
>  
>  struct mem_cgroup_id {
>  	int id;
> -	atomic_t ref;
> +	refcount_t ref;
>  };
>  
>  /*
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7bebe2ddec05..aa728d5b3d72 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4299,14 +4299,12 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>  
>  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);
> +	refcount_add(n, &memcg->id.ref);
>  }
>  
>  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)) {
> +	if (refcount_sub_and_test(n, &memcg->id.ref)) {
>  		mem_cgroup_id_remove(memcg);
>  
>  		/* Memcg ID pins CSS */
> @@ -4523,7 +4521,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
>  	}
>  
>  	/* Online state pins memcg ID, memcg ID pins CSS */
> -	atomic_set(&memcg->id.ref, 1);
> +	refcount_set(&memcg->id.ref, 1);
>  	css_get(css);
>  	return 0;
>  }
> @@ -6357,7 +6355,7 @@ subsys_initcall(mem_cgroup_init);
>  #ifdef CONFIG_MEMCG_SWAP
>  static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
>  {
> -	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> +	while (!refcount_inc_not_zero(&memcg->id.ref)) {
>  		/*
>  		 * The root cgroup cannot be destroyed, so it's refcount must
>  		 * always be >= 1.

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
  2018-10-09 17:46 ` Kirill Tkhai
  (?)
  (?)
@ 2018-10-16 12:49 ` Andrea Parri
  2018-10-16 13:19   ` Kirill Tkhai
  -1 siblings, 1 reply; 6+ messages in thread
From: Andrea Parri @ 2018-10-16 12:49 UTC (permalink / raw)
  To: Kirill Tkhai; +Cc: akpm, hannes, mhocko, vdavydov.dev, linux-kernel, linux-mm

Hi Kirill,

On Tue, Oct 09, 2018 at 08:46:56PM +0300, Kirill Tkhai wrote:
> This will allow to use generic refcount_t interfaces
> to check counters overflow instead of currently existing
> VM_BUG_ON(). The only difference after the patch is
> VM_BUG_ON() may cause BUG(), while refcount_t fires
> with WARN().

refcount_{sub_and_test,inc_not_zero}() are documented to provide
"slightly" more relaxed ordering than their atomic_* counterpart,
c.f.,

  Documentation/core-api/refcount-vs-atomic.rst
  lib/refcount.c (inline comments)

IIUC, this semantic change won't cause problems here (but please
double-check? ;D ).

  Andrea


>But this seems not to be significant here,
> since such the problems are usually caught by syzbot
> with panic-on-warn enabled.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  include/linux/memcontrol.h |    2 +-
>  mm/memcontrol.c            |   10 ++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 4399cc3f00e4..7ab2120155a4 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -78,7 +78,7 @@ struct mem_cgroup_reclaim_cookie {
>  
>  struct mem_cgroup_id {
>  	int id;
> -	atomic_t ref;
> +	refcount_t ref;
>  };
>  
>  /*
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7bebe2ddec05..aa728d5b3d72 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4299,14 +4299,12 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>  
>  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);
> +	refcount_add(n, &memcg->id.ref);
>  }
>  
>  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)) {
> +	if (refcount_sub_and_test(n, &memcg->id.ref)) {
>  		mem_cgroup_id_remove(memcg);
>  
>  		/* Memcg ID pins CSS */
> @@ -4523,7 +4521,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
>  	}
>  
>  	/* Online state pins memcg ID, memcg ID pins CSS */
> -	atomic_set(&memcg->id.ref, 1);
> +	refcount_set(&memcg->id.ref, 1);
>  	css_get(css);
>  	return 0;
>  }
> @@ -6357,7 +6355,7 @@ subsys_initcall(mem_cgroup_init);
>  #ifdef CONFIG_MEMCG_SWAP
>  static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
>  {
> -	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> +	while (!refcount_inc_not_zero(&memcg->id.ref)) {
>  		/*
>  		 * The root cgroup cannot be destroyed, so it's refcount must
>  		 * always be >= 1.
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
  2018-10-16 12:49 ` Andrea Parri
@ 2018-10-16 13:19   ` Kirill Tkhai
  2018-10-18  9:12     ` Michal Hocko
  0 siblings, 1 reply; 6+ messages in thread
From: Kirill Tkhai @ 2018-10-16 13:19 UTC (permalink / raw)
  To: Andrea Parri; +Cc: akpm, hannes, mhocko, vdavydov.dev, linux-kernel, linux-mm

Hi, Andrea,

On 16.10.2018 15:49, Andrea Parri wrote:
> Hi Kirill,
> 
> On Tue, Oct 09, 2018 at 08:46:56PM +0300, Kirill Tkhai wrote:
>> This will allow to use generic refcount_t interfaces
>> to check counters overflow instead of currently existing
>> VM_BUG_ON(). The only difference after the patch is
>> VM_BUG_ON() may cause BUG(), while refcount_t fires
>> with WARN().
> 
> refcount_{sub_and_test,inc_not_zero}() are documented to provide
> "slightly" more relaxed ordering than their atomic_* counterpart,
> c.f.,
> 
>   Documentation/core-api/refcount-vs-atomic.rst
>   lib/refcount.c (inline comments)
> 
> IIUC, this semantic change won't cause problems here (but please
> double-check? ;D ).

I just don't see a place, where we may think about using a modification
of struct mem_cgroup::id::ref as a memory barrier to order something,
and all this looks safe for me.

Kirill
 
>> But this seems not to be significant here,
>> since such the problems are usually caught by syzbot
>> with panic-on-warn enabled.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  include/linux/memcontrol.h |    2 +-
>>  mm/memcontrol.c            |   10 ++++------
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 4399cc3f00e4..7ab2120155a4 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -78,7 +78,7 @@ struct mem_cgroup_reclaim_cookie {
>>  
>>  struct mem_cgroup_id {
>>  	int id;
>> -	atomic_t ref;
>> +	refcount_t ref;
>>  };
>>  
>>  /*
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 7bebe2ddec05..aa728d5b3d72 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4299,14 +4299,12 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>>  
>>  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);
>> +	refcount_add(n, &memcg->id.ref);
>>  }
>>  
>>  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)) {
>> +	if (refcount_sub_and_test(n, &memcg->id.ref)) {
>>  		mem_cgroup_id_remove(memcg);
>>  
>>  		/* Memcg ID pins CSS */
>> @@ -4523,7 +4521,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
>>  	}
>>  
>>  	/* Online state pins memcg ID, memcg ID pins CSS */
>> -	atomic_set(&memcg->id.ref, 1);
>> +	refcount_set(&memcg->id.ref, 1);
>>  	css_get(css);
>>  	return 0;
>>  }
>> @@ -6357,7 +6355,7 @@ subsys_initcall(mem_cgroup_init);
>>  #ifdef CONFIG_MEMCG_SWAP
>>  static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
>>  {
>> -	while (!atomic_inc_not_zero(&memcg->id.ref)) {
>> +	while (!refcount_inc_not_zero(&memcg->id.ref)) {
>>  		/*
>>  		 * The root cgroup cannot be destroyed, so it's refcount must
>>  		 * always be >= 1.
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type
  2018-10-16 13:19   ` Kirill Tkhai
@ 2018-10-18  9:12     ` Michal Hocko
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2018-10-18  9:12 UTC (permalink / raw)
  To: Kirill Tkhai
  Cc: Andrea Parri, akpm, hannes, vdavydov.dev, linux-kernel, linux-mm

On Tue 16-10-18 16:19:40, Kirill Tkhai wrote:
> Hi, Andrea,
> 
> On 16.10.2018 15:49, Andrea Parri wrote:
> > Hi Kirill,
> > 
> > On Tue, Oct 09, 2018 at 08:46:56PM +0300, Kirill Tkhai wrote:
> >> This will allow to use generic refcount_t interfaces
> >> to check counters overflow instead of currently existing
> >> VM_BUG_ON(). The only difference after the patch is
> >> VM_BUG_ON() may cause BUG(), while refcount_t fires
> >> with WARN().
> > 
> > refcount_{sub_and_test,inc_not_zero}() are documented to provide
> > "slightly" more relaxed ordering than their atomic_* counterpart,
> > c.f.,
> > 
> >   Documentation/core-api/refcount-vs-atomic.rst
> >   lib/refcount.c (inline comments)
> > 
> > IIUC, this semantic change won't cause problems here (but please
> > double-check? ;D ).
> 
> I just don't see a place, where we may think about using a modification
> of struct mem_cgroup::id::ref as a memory barrier to order something,
> and all this looks safe for me.

If there was any it would surely be unintentional. memcg->id.ref is a clear
reference counter pattern for the id lifetime.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-18  9:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 17:46 [PATCH] mm: Convert mem_cgroup_id::ref to refcount_t type Kirill Tkhai
2018-10-09 17:46 ` Kirill Tkhai
2018-10-16 12:05 ` Michal Hocko
2018-10-16 12:49 ` Andrea Parri
2018-10-16 13:19   ` Kirill Tkhai
2018-10-18  9:12     ` Michal Hocko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.