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

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

[Wei Yang]

No functional change, try to make it more readable.

v2: some adjustment as suggested by Johannes 

Wei Yang (3):
  mm/memcg: set memcg after css verified and got reference
  mm/memcg: set pos explicitly for reclaim and !reclaim
  mm/memcg: move generation assignment and comparison together

 mm/memcontrol.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

-- 
2.33.1

[Patch v2 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>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index dc193e83794d..eed9916cdce5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1057,15 +1057,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

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

[Roman Gushchin]

On Wed, Mar 30, 2022 at 11:47:17PM +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>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Nice cleanup!

> ---
>  mm/memcontrol.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index dc193e83794d..eed9916cdce5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1057,15 +1057,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 v2 2/3] mm/memcg: set pos explicitly for reclaim and !reclaim

[Wei Yang]

During mem_cgroup_iter, there are two ways to get iteration position:
reclaim vs non-reclaim mode.

Let's do it explicitly for reclaim vs non-reclaim mode.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

---
v2: split into two explicit part as suggested by Johannes
---
 mm/memcontrol.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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)
-- 
2.33.1

Re: [Patch v2 2/3] mm/memcg: set pos explicitly for reclaim and !reclaim

[Roman Gushchin]

On Wed, Mar 30, 2022 at 11:47:18PM +0000, Wei Yang wrote:
> During mem_cgroup_iter, there are two ways to get iteration position:
> reclaim vs non-reclaim mode.
> 
> Let's do it explicitly for reclaim vs non-reclaim mode.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

> 
> ---
> v2: split into two explicit part as suggested by Johannes
> ---
>  mm/memcontrol.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> 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)
> -- 
> 2.33.1
> 
> 

Re: [Patch v2 2/3] mm/memcg: set pos explicitly for reclaim and !reclaim

[Johannes Weiner]

On Wed, Mar 30, 2022 at 11:47:18PM +0000, Wei Yang wrote:
> During mem_cgroup_iter, there are two ways to get iteration position:
> reclaim vs non-reclaim mode.
> 
> Let's do it explicitly for reclaim vs non-reclaim mode.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

[Patch v2 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.

[hannes@cmpxchg.org: better comment to explain reclaim model]

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>

---
v2: a better comment from Johannes
---
 mm/memcontrol.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5d433b79ba47..2cd8bfdec379 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1013,7 +1013,13 @@ 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 start, join the current reclaim iteration cycle.
+		 * Exit when a concurrent walker completes it.
+		 */
+		if (!prev)
+			reclaim->generation = iter->generation;
+		else if (reclaim->generation != iter->generation)
 			goto out_unlock;
 
 		while (1) {
@@ -1075,8 +1081,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 v2 3/3] mm/memcg: move generation assignment and comparison together

[Roman Gushchin]

On Wed, Mar 30, 2022 at 11:47:19PM +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.
> 
> [hannes@cmpxchg.org: better comment to explain reclaim model]
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

> 
> ---
> v2: a better comment from Johannes
> ---
>  mm/memcontrol.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5d433b79ba47..2cd8bfdec379 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1013,7 +1013,13 @@ 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 start, join the current reclaim iteration cycle.
> +		 * Exit when a concurrent walker completes it.
> +		 */
> +		if (!prev)
> +			reclaim->generation = iter->generation;
> +		else if (reclaim->generation != iter->generation)
>  			goto out_unlock;
>  
>  		while (1) {
> @@ -1075,8 +1081,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
> 
>