[PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Jianyu Zhan]

Currently, the iteration job in mem_cgroup_iter() all delegates
to __mem_cgroup_iter_next(), which will skip dead node.

Thus, the outer while loop in mem_cgroup_iter() is meaningless.
It could be proven by this:

1. memcg != NULL
    we are done, no loop needed.
2. memcg == NULL
   2.1 prev != NULL, a round-trip is done, break out, no loop.
   2.2 prev == NULL, this is impossible, since prev==NULL means
       the initial interation, it will returns memcg==root.

So, this patches remove this meaningless while loop.

Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
---
 mm/memcontrol.c | 49 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 29501f0..e0ce15c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1212,6 +1212,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 {
 	struct mem_cgroup *memcg = NULL;
 	struct mem_cgroup *last_visited = NULL;
+	struct mem_cgroup_reclaim_iter *uninitialized_var(iter);
+	int uninitialized_var(seq);
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -1229,40 +1231,33 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 	}
 
 	rcu_read_lock();
-	while (!memcg) {
-		struct mem_cgroup_reclaim_iter *uninitialized_var(iter);
-		int uninitialized_var(seq);
-
-		if (reclaim) {
-			int nid = zone_to_nid(reclaim->zone);
-			int zid = zone_idx(reclaim->zone);
-			struct mem_cgroup_per_zone *mz;
-
-			mz = mem_cgroup_zoneinfo(root, nid, zid);
-			iter = &mz->reclaim_iter[reclaim->priority];
-			if (prev && reclaim->generation != iter->generation) {
-				iter->last_visited = NULL;
-				goto out_unlock;
-			}
+	if (reclaim) {
+		int nid = zone_to_nid(reclaim->zone);
+		int zid = zone_idx(reclaim->zone);
+		struct mem_cgroup_per_zone *mz;
 
-			last_visited = mem_cgroup_iter_load(iter, root, &seq);
+		mz = mem_cgroup_zoneinfo(root, nid, zid);
+		iter = &mz->reclaim_iter[reclaim->priority];
+		if (prev && reclaim->generation != iter->generation) {
+			iter->last_visited = NULL;
+			goto out_unlock;
 		}
 
-		memcg = __mem_cgroup_iter_next(root, last_visited);
+		last_visited = mem_cgroup_iter_load(iter, root, &seq);
+	}
 
-		if (reclaim) {
-			mem_cgroup_iter_update(iter, last_visited, memcg, root,
-					seq);
+	memcg = __mem_cgroup_iter_next(root, last_visited);
 
-			if (!memcg)
-				iter->generation++;
-			else if (!prev && memcg)
-				reclaim->generation = iter->generation;
-		}
+	if (reclaim) {
+		mem_cgroup_iter_update(iter, last_visited, memcg, root,
+				seq);
 
-		if (prev && !memcg)
-			goto out_unlock;
+		if (!memcg)
+			iter->generation++;
+		else if (!prev && memcg)
+			reclaim->generation = iter->generation;
 	}
+
 out_unlock:
 	rcu_read_unlock();
 out_css_put:
-- 
1.9.0.GIT

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Michal Hocko]

On Sat 19-04-14 06:58:55, Jianyu Zhan wrote:
> Currently, the iteration job in mem_cgroup_iter() all delegates
> to __mem_cgroup_iter_next(), which will skip dead node.
> 
> Thus, the outer while loop in mem_cgroup_iter() is meaningless.
> It could be proven by this:
> 
> 1. memcg != NULL
>     we are done, no loop needed.
> 2. memcg == NULL
>    2.1 prev != NULL, a round-trip is done, break out, no loop.
>    2.2 prev == NULL, this is impossible, since prev==NULL means
>        the initial interation, it will returns memcg==root.

What about
  3. last_visited == last_node in the tree

__mem_cgroup_iter_next returns NULL and the iterator would return
without visiting anything.

The patch is not correct, I am afraid.

> So, this patches remove this meaningless while loop.
> 
> Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
> ---
>  mm/memcontrol.c | 49 ++++++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 29501f0..e0ce15c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1212,6 +1212,8 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  {
>  	struct mem_cgroup *memcg = NULL;
>  	struct mem_cgroup *last_visited = NULL;
> +	struct mem_cgroup_reclaim_iter *uninitialized_var(iter);
> +	int uninitialized_var(seq);
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
> @@ -1229,40 +1231,33 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  	}
>  
>  	rcu_read_lock();
> -	while (!memcg) {
> -		struct mem_cgroup_reclaim_iter *uninitialized_var(iter);
> -		int uninitialized_var(seq);
> -
> -		if (reclaim) {
> -			int nid = zone_to_nid(reclaim->zone);
> -			int zid = zone_idx(reclaim->zone);
> -			struct mem_cgroup_per_zone *mz;
> -
> -			mz = mem_cgroup_zoneinfo(root, nid, zid);
> -			iter = &mz->reclaim_iter[reclaim->priority];
> -			if (prev && reclaim->generation != iter->generation) {
> -				iter->last_visited = NULL;
> -				goto out_unlock;
> -			}
> +	if (reclaim) {
> +		int nid = zone_to_nid(reclaim->zone);
> +		int zid = zone_idx(reclaim->zone);
> +		struct mem_cgroup_per_zone *mz;
>  
> -			last_visited = mem_cgroup_iter_load(iter, root, &seq);
> +		mz = mem_cgroup_zoneinfo(root, nid, zid);
> +		iter = &mz->reclaim_iter[reclaim->priority];
> +		if (prev && reclaim->generation != iter->generation) {
> +			iter->last_visited = NULL;
> +			goto out_unlock;
>  		}
>  
> -		memcg = __mem_cgroup_iter_next(root, last_visited);
> +		last_visited = mem_cgroup_iter_load(iter, root, &seq);
> +	}
>  
> -		if (reclaim) {
> -			mem_cgroup_iter_update(iter, last_visited, memcg, root,
> -					seq);
> +	memcg = __mem_cgroup_iter_next(root, last_visited);
>  
> -			if (!memcg)
> -				iter->generation++;
> -			else if (!prev && memcg)
> -				reclaim->generation = iter->generation;
> -		}
> +	if (reclaim) {
> +		mem_cgroup_iter_update(iter, last_visited, memcg, root,
> +				seq);
>  
> -		if (prev && !memcg)
> -			goto out_unlock;
> +		if (!memcg)
> +			iter->generation++;
> +		else if (!prev && memcg)
> +			reclaim->generation = iter->generation;
>  	}
> +
>  out_unlock:
>  	rcu_read_unlock();
>  out_css_put:
> -- 
> 1.9.0.GIT
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Jianyu Zhan]

On Tue, Apr 22, 2014 at 5:47 PM, Michal Hocko <mhocko@suse.cz> wrote:
> What about
>   3. last_visited == last_node in the tree
>
> __mem_cgroup_iter_next returns NULL and the iterator would return
> without visiting anything.

Hi,  Michal,

yep,  if 3 last_visited == last_node, then this means we have done a round-trip,
thus __mem_cgroup_iter_next returns NULL, in turn mem_cgroup_iter() return NULL.

This is what comments above mem_cgroup_iter() says:

>Returns references to children of the hierarchy below @root, or
>* @root itself, or %NULL after a full round-trip.

Actually, this condition could be reduced to conditon 2.1

Thanks,
Jianyu Zhan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Michal Hocko]

On Tue 22-04-14 18:17:09, Jianyu Zhan wrote:
> On Tue, Apr 22, 2014 at 5:47 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > What about
> >   3. last_visited == last_node in the tree
> >
> > __mem_cgroup_iter_next returns NULL and the iterator would return
> > without visiting anything.
> 
> Hi,  Michal,
> 
> yep,  if 3 last_visited == last_node, then this means we have done a round-trip,
> thus __mem_cgroup_iter_next returns NULL, in turn mem_cgroup_iter() return NULL.

Sorry, I should have been more specific that I was talking about
mem_cgroup_reclaim_cookie path where the iteration for this particular
zone and priority ended at the last node without finishing the full
roundtrip last time. This new iteration (prev==NULL) wants to continue
and it should start a new roundtrip.

Makes sense?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Jianyu Zhan]

On Tue, Apr 22, 2014 at 6:34 PM, Michal Hocko <mhocko@suse.cz> wrote:
> Sorry, I should have been more specific that I was talking about
> mem_cgroup_reclaim_cookie path where the iteration for this particular
> zone and priority ended at the last node without finishing the full
> roundtrip last time. This new iteration (prev==NULL) wants to continue
> and it should start a new roundtrip.
>
> Makes sense?

Hi, Michal,

Good catch, it makes sense !
This reminds me of my draft edition of this patch, I specifically handle
this case as:

if (reclaim) {
               if (!memcg ) {
                              iter->generation++;
                              if (!prev) {
                                    memcg = root;
                                    mem_cgroup_iter_update(iter, NULL,
memcg, root,  seq);
                                    goto out_unlock:
                              }
              }
              mem_cgroup_iter_update(iter, last_visited, memcg, root,
                                seq);
              if (!prev && memcg)
                        reclaim->generation = iter->generation;
}

This is literally manual unwinding the second while loop, and thus omit
the while loop,
to save a   mem_cgroup_iter_update() and a mem_cgroup_iter_update()

But it maybe a bit hard to read.

If it is OK, I could resend a new one.

Thanks,
Jianyu Zhan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Michal Hocko]

On Tue 22-04-14 18:58:11, Jianyu Zhan wrote:
[...]
> This reminds me of my draft edition of this patch, I specifically handle
> this case as:
> 
> if (reclaim) {
>                if (!memcg ) {
>                               iter->generation++;
>                               if (!prev) {
>                                     memcg = root;
>                                     mem_cgroup_iter_update(iter, NULL, memcg, root,  seq);
>                                     goto out_unlock:
>                               }
>               }
>               mem_cgroup_iter_update(iter, last_visited, memcg, root,
>                                 seq);
>               if (!prev && memcg)
>                         reclaim->generation = iter->generation;
> }
> 
> This is literally manual unwinding the second while loop, and thus omit
> the while loop,
> to save a   mem_cgroup_iter_update() and a mem_cgroup_iter_update()
> 
> But it maybe a bit hard to read.

Dunno, this particular case is more explicit but it is also uglier so I
do not think this is an overall improvement. I would rather keep the
current state unless the change either simplifies the generated code
or it is much better to read.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/memcontrol.c: remove meaningless while loop in mem_cgroup_iter()

[Jianyu Zhan]

On Tue, Apr 22, 2014 at 7:48 PM, Michal Hocko <mhocko@suse.cz> wrote:
> Dunno, this particular case is more explicit but it is also uglier so I
> do not think this is an overall improvement. I would rather keep the
> current state unless the change either simplifies the generated code
> or it is much better to read.

hmm, I agree.  I will give it more thinking.

Seem this has been merged into -mm.  Andrew, please drop it!

Thanks,
Jianyu Zhan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>