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 <[email protected]>
---
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.
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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.
>
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 <[email protected]>
>> ---
>> 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.
>>
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