[PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()

[PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()

[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

[PATCH 1/3] mm/memcg: set memcg after css verified and got reference

[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

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

[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

[PATCH 3/3] mm/memcg: move generation assignment and comparison together

[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

Re: [PATCH 0/3] mm/memcg: some cleanup for mem_cgroup_iter()

[Michal Hocko]

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

Re: [PATCH 1/3] mm/memcg: set memcg after css verified and got reference

[Johannes Weiner]

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>

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

[Johannes Weiner]

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;
	}

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

[Wei Yang]

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

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

[Johannes Weiner]

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!

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

[Wei Yang]

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

Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together

[Johannes Weiner]

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>

Re: [PATCH 3/3] mm/memcg: move generation assignment and comparison together

[Wei Yang]

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