* [PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes-druUgvl0LCNAfugRpC6u6w, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Wei Yang
No functional change, try to make it more readable.
Wei Yang (3):
mm/memcg: set memcg after css verified and got reference
mm/memcg: set pos to prev unconditionally
mm/memcg: move generation assignment and comparison together
mm/memcontrol.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] mm/memcg: set memcg after css verified and got reference
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes, mhocko, vdavydov.dev, akpm; +Cc: cgroups, linux-mm, Wei Yang
Instead of reset memcg when css is either not verified or not got
reference, we can set it after these process.
No functional change, just simplified the code a little.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
mm/memcontrol.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 36e9f38c919d..9464fe2aa329 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1040,15 +1040,10 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
* is provided by the caller, so we know it's alive
* and kicking, and don't take an extra reference.
*/
- memcg = mem_cgroup_from_css(css);
-
- if (css == &root->css)
- break;
-
- if (css_tryget(css))
+ if (css == &root->css || css_tryget(css)) {
+ memcg = mem_cgroup_from_css(css);
break;
-
- memcg = NULL;
+ }
}
if (reclaim) {
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 1/3] mm/memcg: set memcg after css verified and got reference
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes-druUgvl0LCNAfugRpC6u6w, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Wei Yang
Instead of reset memcg when css is either not verified or not got
reference, we can set it after these process.
No functional change, just simplified the code a little.
Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
mm/memcontrol.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 36e9f38c919d..9464fe2aa329 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1040,15 +1040,10 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
* is provided by the caller, so we know it's alive
* and kicking, and don't take an extra reference.
*/
- memcg = mem_cgroup_from_css(css);
-
- if (css == &root->css)
- break;
-
- if (css_tryget(css))
+ if (css == &root->css || css_tryget(css)) {
+ memcg = mem_cgroup_from_css(css);
break;
-
- memcg = NULL;
+ }
}
if (reclaim) {
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/3] mm/memcg: set memcg after css verified and got reference
@ 2022-03-29 18:44 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-29 18:44 UTC (permalink / raw)
To: Wei Yang; +Cc: mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Fri, Feb 25, 2022 at 12:34:35AM +0000, Wei Yang wrote:
> Instead of reset memcg when css is either not verified or not got
> reference, we can set it after these process.
>
> No functional change, just simplified the code a little.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
This looks better indeed.
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] mm/memcg: set memcg after css verified and got reference
@ 2022-03-29 18:44 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-29 18:44 UTC (permalink / raw)
To: Wei Yang
Cc: mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Fri, Feb 25, 2022 at 12:34:35AM +0000, Wei Yang wrote:
> Instead of reset memcg when css is either not verified or not got
> reference, we can set it after these process.
>
> No functional change, just simplified the code a little.
>
> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
This looks better indeed.
Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes, mhocko, vdavydov.dev, akpm; +Cc: cgroups, linux-mm, Wei Yang
Current code set pos to prev based on condition (prev && !reclaim),
while we can do this unconditionally.
Since:
* If !reclaim, pos is the same as prev no matter it is NULL or not.
* If reclaim, pos would be set properly from iter->position.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
mm/memcontrol.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9464fe2aa329..03399146168f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
struct mem_cgroup_reclaim_iter *iter;
struct cgroup_subsys_state *css = NULL;
struct mem_cgroup *memcg = NULL;
- struct mem_cgroup *pos = NULL;
+ struct mem_cgroup *pos = prev;
if (mem_cgroup_disabled())
return NULL;
@@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!root)
root = root_mem_cgroup;
- if (prev && !reclaim)
- pos = prev;
-
rcu_read_lock();
if (reclaim) {
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes-druUgvl0LCNAfugRpC6u6w, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Wei Yang
Current code set pos to prev based on condition (prev && !reclaim),
while we can do this unconditionally.
Since:
* If !reclaim, pos is the same as prev no matter it is NULL or not.
* If reclaim, pos would be set properly from iter->position.
Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
mm/memcontrol.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9464fe2aa329..03399146168f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
struct mem_cgroup_reclaim_iter *iter;
struct cgroup_subsys_state *css = NULL;
struct mem_cgroup *memcg = NULL;
- struct mem_cgroup *pos = NULL;
+ struct mem_cgroup *pos = prev;
if (mem_cgroup_disabled())
return NULL;
@@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!root)
root = root_mem_cgroup;
- if (prev && !reclaim)
- pos = prev;
-
rcu_read_lock();
if (reclaim) {
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-29 18:48 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-29 18:48 UTC (permalink / raw)
To: Wei Yang; +Cc: mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Fri, Feb 25, 2022 at 12:34:36AM +0000, Wei Yang wrote:
> Current code set pos to prev based on condition (prev && !reclaim),
> while we can do this unconditionally.
>
> Since:
>
> * If !reclaim, pos is the same as prev no matter it is NULL or not.
> * If reclaim, pos would be set properly from iter->position.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
> mm/memcontrol.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9464fe2aa329..03399146168f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> struct mem_cgroup_reclaim_iter *iter;
> struct cgroup_subsys_state *css = NULL;
> struct mem_cgroup *memcg = NULL;
> - struct mem_cgroup *pos = NULL;
> + struct mem_cgroup *pos = prev;
I don't like this so much. It suggests pos always starts with prev, no
matter what. But this isn't true for reclaim mode, which overrides the
initialized value again.
> if (mem_cgroup_disabled())
> return NULL;
> @@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> if (!root)
> root = root_mem_cgroup;
>
> - if (prev && !reclaim)
> - pos = prev;
How about making the reclaim vs non-reclaim mode explicit and do:
if (reclaim) {
...
pos = iter->position;
...
} else {
pos = prev;
}
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-29 18:48 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-29 18:48 UTC (permalink / raw)
To: Wei Yang
Cc: mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Fri, Feb 25, 2022 at 12:34:36AM +0000, Wei Yang wrote:
> Current code set pos to prev based on condition (prev && !reclaim),
> while we can do this unconditionally.
>
> Since:
>
> * If !reclaim, pos is the same as prev no matter it is NULL or not.
> * If reclaim, pos would be set properly from iter->position.
>
> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> mm/memcontrol.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9464fe2aa329..03399146168f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> struct mem_cgroup_reclaim_iter *iter;
> struct cgroup_subsys_state *css = NULL;
> struct mem_cgroup *memcg = NULL;
> - struct mem_cgroup *pos = NULL;
> + struct mem_cgroup *pos = prev;
I don't like this so much. It suggests pos always starts with prev, no
matter what. But this isn't true for reclaim mode, which overrides the
initialized value again.
> if (mem_cgroup_disabled())
> return NULL;
> @@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> if (!root)
> root = root_mem_cgroup;
>
> - if (prev && !reclaim)
> - pos = prev;
How about making the reclaim vs non-reclaim mode explicit and do:
if (reclaim) {
...
pos = iter->position;
...
} else {
pos = prev;
}
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-30 0:47 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 0:47 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Wei Yang, mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Tue, Mar 29, 2022 at 02:48:05PM -0400, Johannes Weiner wrote:
>On Fri, Feb 25, 2022 at 12:34:36AM +0000, Wei Yang wrote:
>> Current code set pos to prev based on condition (prev && !reclaim),
>> while we can do this unconditionally.
>>
>> Since:
>>
>> * If !reclaim, pos is the same as prev no matter it is NULL or not.
>> * If reclaim, pos would be set properly from iter->position.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>> mm/memcontrol.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 9464fe2aa329..03399146168f 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> struct mem_cgroup_reclaim_iter *iter;
>> struct cgroup_subsys_state *css = NULL;
>> struct mem_cgroup *memcg = NULL;
>> - struct mem_cgroup *pos = NULL;
>> + struct mem_cgroup *pos = prev;
>
>I don't like this so much. It suggests pos always starts with prev, no
>matter what. But this isn't true for reclaim mode, which overrides the
>initialized value again.
>
>> if (mem_cgroup_disabled())
>> return NULL;
>> @@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> if (!root)
>> root = root_mem_cgroup;
>>
>> - if (prev && !reclaim)
>> - pos = prev;
>
>How about making the reclaim vs non-reclaim mode explicit and do:
>
> if (reclaim) {
> ...
> pos = iter->position;
> ...
> } else {
> pos = prev;
> }
Something like this?
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index eed9916cdce5..5d433b79ba47 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!root)
root = root_mem_cgroup;
- if (prev && !reclaim)
- pos = prev;
-
rcu_read_lock();
if (reclaim) {
@@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
*/
(void)cmpxchg(&iter->position, pos, NULL);
}
+ } else if (prev) {
+ pos = prev;
}
if (pos)
--
Wei Yang
Help you, Help me
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-30 0:47 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 0:47 UTC (permalink / raw)
To: Johannes Weiner
Cc: Wei Yang, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Tue, Mar 29, 2022 at 02:48:05PM -0400, Johannes Weiner wrote:
>On Fri, Feb 25, 2022 at 12:34:36AM +0000, Wei Yang wrote:
>> Current code set pos to prev based on condition (prev && !reclaim),
>> while we can do this unconditionally.
>>
>> Since:
>>
>> * If !reclaim, pos is the same as prev no matter it is NULL or not.
>> * If reclaim, pos would be set properly from iter->position.
>>
>> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> mm/memcontrol.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 9464fe2aa329..03399146168f 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -980,7 +980,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> struct mem_cgroup_reclaim_iter *iter;
>> struct cgroup_subsys_state *css = NULL;
>> struct mem_cgroup *memcg = NULL;
>> - struct mem_cgroup *pos = NULL;
>> + struct mem_cgroup *pos = prev;
>
>I don't like this so much. It suggests pos always starts with prev, no
>matter what. But this isn't true for reclaim mode, which overrides the
>initialized value again.
>
>> if (mem_cgroup_disabled())
>> return NULL;
>> @@ -988,9 +988,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> if (!root)
>> root = root_mem_cgroup;
>>
>> - if (prev && !reclaim)
>> - pos = prev;
>
>How about making the reclaim vs non-reclaim mode explicit and do:
>
> if (reclaim) {
> ...
> pos = iter->position;
> ...
> } else {
> pos = prev;
> }
Something like this?
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index eed9916cdce5..5d433b79ba47 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!root)
root = root_mem_cgroup;
- if (prev && !reclaim)
- pos = prev;
-
rcu_read_lock();
if (reclaim) {
@@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
*/
(void)cmpxchg(&iter->position, pos, NULL);
}
+ } else if (prev) {
+ pos = prev;
}
if (pos)
--
Wei Yang
Help you, Help me
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
2022-03-30 0:47 ` Wei Yang
@ 2022-03-30 12:08 ` Johannes Weiner
-1 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-30 12:08 UTC (permalink / raw)
To: Wei Yang; +Cc: mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Wed, Mar 30, 2022 at 12:47:50AM +0000, Wei Yang wrote:
> Something like this?
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index eed9916cdce5..5d433b79ba47 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> if (!root)
> root = root_mem_cgroup;
>
> - if (prev && !reclaim)
> - pos = prev;
> -
> rcu_read_lock();
>
> if (reclaim) {
> @@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> */
> (void)cmpxchg(&iter->position, pos, NULL);
> }
> + } else if (prev) {
> + pos = prev;
> }
>
> if (pos)
Yep!
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-30 12:08 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-30 12:08 UTC (permalink / raw)
To: Wei Yang
Cc: mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Wed, Mar 30, 2022 at 12:47:50AM +0000, Wei Yang wrote:
> Something like this?
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index eed9916cdce5..5d433b79ba47 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> if (!root)
> root = root_mem_cgroup;
>
> - if (prev && !reclaim)
> - pos = prev;
> -
> rcu_read_lock();
>
> if (reclaim) {
> @@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> */
> (void)cmpxchg(&iter->position, pos, NULL);
> }
> + } else if (prev) {
> + pos = prev;
> }
>
> if (pos)
Yep!
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-30 14:22 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 14:22 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Wei Yang, mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Wed, Mar 30, 2022 at 08:08:49AM -0400, Johannes Weiner wrote:
>On Wed, Mar 30, 2022 at 12:47:50AM +0000, Wei Yang wrote:
>> Something like this?
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index eed9916cdce5..5d433b79ba47 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> if (!root)
>> root = root_mem_cgroup;
>>
>> - if (prev && !reclaim)
>> - pos = prev;
>> -
>> rcu_read_lock();
>>
>> if (reclaim) {
>> @@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> */
>> (void)cmpxchg(&iter->position, pos, NULL);
>> }
>> + } else if (prev) {
>> + pos = prev;
>> }
>>
>> if (pos)
>
>Yep!
Sure, I would prepare a v2.
BTW, do you have some comment on patch 3?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally
@ 2022-03-30 14:22 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 14:22 UTC (permalink / raw)
To: Johannes Weiner
Cc: Wei Yang, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Wed, Mar 30, 2022 at 08:08:49AM -0400, Johannes Weiner wrote:
>On Wed, Mar 30, 2022 at 12:47:50AM +0000, Wei Yang wrote:
>> Something like this?
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index eed9916cdce5..5d433b79ba47 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1005,9 +1005,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> if (!root)
>> root = root_mem_cgroup;
>>
>> - if (prev && !reclaim)
>> - pos = prev;
>> -
>> rcu_read_lock();
>>
>> if (reclaim) {
>> @@ -1033,6 +1030,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> */
>> (void)cmpxchg(&iter->position, pos, NULL);
>> }
>> + } else if (prev) {
>> + pos = prev;
>> }
>>
>> if (pos)
>
>Yep!
Sure, I would prepare a v2.
BTW, do you have some comment on patch 3?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes, mhocko, vdavydov.dev, akpm; +Cc: cgroups, linux-mm, Wei Yang
For each round-trip, we assign generation on first invocation and
compare it on subsequent invocations.
Let's move them together to make it more self-explaining. Also this
reduce a check on prev.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
mm/memcontrol.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 03399146168f..17da93c2f94e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
mz = root->nodeinfo[reclaim->pgdat->node_id];
iter = &mz->iter;
- if (prev && reclaim->generation != iter->generation)
+ /*
+ * On first invocation, assign iter->generation to
+ * reclaim->generation.
+ * On subsequent invocations, make sure no one else jump in.
+ */
+ if (!prev)
+ reclaim->generation = iter->generation;
+ else if (reclaim->generation != iter->generation)
goto out_unlock;
while (1) {
@@ -1056,8 +1063,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!memcg)
iter->generation++;
- else if (!prev)
- reclaim->generation = iter->generation;
}
out_unlock:
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-02-25 0:34 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-02-25 0:34 UTC (permalink / raw)
To: hannes-druUgvl0LCNAfugRpC6u6w, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Wei Yang
For each round-trip, we assign generation on first invocation and
compare it on subsequent invocations.
Let's move them together to make it more self-explaining. Also this
reduce a check on prev.
Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
mm/memcontrol.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 03399146168f..17da93c2f94e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
mz = root->nodeinfo[reclaim->pgdat->node_id];
iter = &mz->iter;
- if (prev && reclaim->generation != iter->generation)
+ /*
+ * On first invocation, assign iter->generation to
+ * reclaim->generation.
+ * On subsequent invocations, make sure no one else jump in.
+ */
+ if (!prev)
+ reclaim->generation = iter->generation;
+ else if (reclaim->generation != iter->generation)
goto out_unlock;
while (1) {
@@ -1056,8 +1063,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
if (!memcg)
iter->generation++;
- else if (!prev)
- reclaim->generation = iter->generation;
}
out_unlock:
--
2.33.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-03-30 15:57 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-30 15:57 UTC (permalink / raw)
To: Wei Yang; +Cc: mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote:
> For each round-trip, we assign generation on first invocation and
> compare it on subsequent invocations.
>
> Let's move them together to make it more self-explaining. Also this
> reduce a check on prev.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
This makes sense. The function is structured into 1) load state, 2)
advance, 3) save state. The load state is a better fit for
initializing reclaim->generation.
> @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> mz = root->nodeinfo[reclaim->pgdat->node_id];
> iter = &mz->iter;
>
> - if (prev && reclaim->generation != iter->generation)
> + /*
> + * On first invocation, assign iter->generation to
> + * reclaim->generation.
> + * On subsequent invocations, make sure no one else jump in.
> + */
> + if (!prev)
> + reclaim->generation = iter->generation;
> + else if (reclaim->generation != iter->generation)
> goto out_unlock;
The comment duplicates the code, it doesn't explain why we're doing
this. How about:
/*
* On start, join the current reclaim iteration cycle.
* Exit when a concurrent walker completes it.
*/
With that, please feel free to add:
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-03-30 15:57 ` Johannes Weiner
0 siblings, 0 replies; 24+ messages in thread
From: Johannes Weiner @ 2022-03-30 15:57 UTC (permalink / raw)
To: Wei Yang
Cc: mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote:
> For each round-trip, we assign generation on first invocation and
> compare it on subsequent invocations.
>
> Let's move them together to make it more self-explaining. Also this
> reduce a check on prev.
>
> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
This makes sense. The function is structured into 1) load state, 2)
advance, 3) save state. The load state is a better fit for
initializing reclaim->generation.
> @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> mz = root->nodeinfo[reclaim->pgdat->node_id];
> iter = &mz->iter;
>
> - if (prev && reclaim->generation != iter->generation)
> + /*
> + * On first invocation, assign iter->generation to
> + * reclaim->generation.
> + * On subsequent invocations, make sure no one else jump in.
> + */
> + if (!prev)
> + reclaim->generation = iter->generation;
> + else if (reclaim->generation != iter->generation)
> goto out_unlock;
The comment duplicates the code, it doesn't explain why we're doing
this. How about:
/*
* On start, join the current reclaim iteration cycle.
* Exit when a concurrent walker completes it.
*/
With that, please feel free to add:
Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-03-30 23:04 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 23:04 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Wei Yang, mhocko, vdavydov.dev, akpm, cgroups, linux-mm
On Wed, Mar 30, 2022 at 11:57:07AM -0400, Johannes Weiner wrote:
>On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote:
>> For each round-trip, we assign generation on first invocation and
>> compare it on subsequent invocations.
>>
>> Let's move them together to make it more self-explaining. Also this
>> reduce a check on prev.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>This makes sense. The function is structured into 1) load state, 2)
>advance, 3) save state. The load state is a better fit for
>initializing reclaim->generation.
>
>> @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> mz = root->nodeinfo[reclaim->pgdat->node_id];
>> iter = &mz->iter;
>>
>> - if (prev && reclaim->generation != iter->generation)
>> + /*
>> + * On first invocation, assign iter->generation to
>> + * reclaim->generation.
>> + * On subsequent invocations, make sure no one else jump in.
>> + */
>> + if (!prev)
>> + reclaim->generation = iter->generation;
>> + else if (reclaim->generation != iter->generation)
>> goto out_unlock;
>
>The comment duplicates the code, it doesn't explain why we're doing
>this. How about:
>
> /*
> * On start, join the current reclaim iteration cycle.
> * Exit when a concurrent walker completes it.
> */
This one looks better and explain the reclaim model.
Thanks
>
>With that, please feel free to add:
>
>Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together
@ 2022-03-30 23:04 ` Wei Yang
0 siblings, 0 replies; 24+ messages in thread
From: Wei Yang @ 2022-03-30 23:04 UTC (permalink / raw)
To: Johannes Weiner
Cc: Wei Yang, mhocko-DgEjT+Ai2ygdnm+yROfE0A,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Wed, Mar 30, 2022 at 11:57:07AM -0400, Johannes Weiner wrote:
>On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote:
>> For each round-trip, we assign generation on first invocation and
>> compare it on subsequent invocations.
>>
>> Let's move them together to make it more self-explaining. Also this
>> reduce a check on prev.
>>
>> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>This makes sense. The function is structured into 1) load state, 2)
>advance, 3) save state. The load state is a better fit for
>initializing reclaim->generation.
>
>> @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>> mz = root->nodeinfo[reclaim->pgdat->node_id];
>> iter = &mz->iter;
>>
>> - if (prev && reclaim->generation != iter->generation)
>> + /*
>> + * On first invocation, assign iter->generation to
>> + * reclaim->generation.
>> + * On subsequent invocations, make sure no one else jump in.
>> + */
>> + if (!prev)
>> + reclaim->generation = iter->generation;
>> + else if (reclaim->generation != iter->generation)
>> goto out_unlock;
>
>The comment duplicates the code, it doesn't explain why we're doing
>this. How about:
>
> /*
> * On start, join the current reclaim iteration cycle.
> * Exit when a concurrent walker completes it.
> */
This one looks better and explain the reclaim model.
Thanks
>
>With that, please feel free to add:
>
>Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()
@ 2022-02-25 8:13 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2022-02-25 8:13 UTC (permalink / raw)
To: Wei Yang; +Cc: hannes, vdavydov.dev, akpm, cgroups, linux-mm
On Fri 25-02-22 00:34:34, Wei Yang wrote:
> No functional change, try to make it more readable.
>
> Wei Yang (3):
> mm/memcg: set memcg after css verified and got reference
> mm/memcg: set pos to prev unconditionally
> mm/memcg: move generation assignment and comparison together
>
> mm/memcontrol.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
I am sorry but I do not really see these changes to be simplifying
the iterator code enough to be worth touching the code. The iterator
code is really subtle and we have experienced some subtle bugs there.
I would be really reluctant to touch it unless the result is a clear
simplification or a bug fix. Please keep in mind that the review
overhead is far from negligible here.
Unless Johannes sees that as a clear improvement then I would suggest
dropping these patches from the akpm's tree.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()
@ 2022-02-25 8:13 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2022-02-25 8:13 UTC (permalink / raw)
To: Wei Yang
Cc: hannes-druUgvl0LCNAfugRpC6u6w,
vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
On Fri 25-02-22 00:34:34, Wei Yang wrote:
> No functional change, try to make it more readable.
>
> Wei Yang (3):
> mm/memcg: set memcg after css verified and got reference
> mm/memcg: set pos to prev unconditionally
> mm/memcg: move generation assignment and comparison together
>
> mm/memcontrol.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
I am sorry but I do not really see these changes to be simplifying
the iterator code enough to be worth touching the code. The iterator
code is really subtle and we have experienced some subtle bugs there.
I would be really reluctant to touch it unless the result is a clear
simplification or a bug fix. Please keep in mind that the review
overhead is far from negligible here.
Unless Johannes sees that as a clear improvement then I would suggest
dropping these patches from the akpm's tree.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 24+ messages in thread