| Message ID | 20220225003437.12620-3-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcg: some cleanup for mem_cgroup_iter() | expand |
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; }
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)
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!
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?
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) {
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(-)