Re: [PATCH 2/3] mm/memcg: set pos to prev unconditionally

[Wei Yang <richard.weiyang@gmail.com>]

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