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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Tue, Apr 22, 2014 at 5:47 PM, Michal Hocko <[email protected]> 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
On Tue 22-04-14 18:17:09, Jianyu Zhan wrote:
> On Tue, Apr 22, 2014 at 5:47 PM, Michal Hocko <[email protected]> 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
On Tue, Apr 22, 2014 at 6:34 PM, Michal Hocko <[email protected]> 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
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
On Tue, Apr 22, 2014 at 7:48 PM, Michal Hocko <[email protected]> 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