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