On Wed, Dec 12, 2012 at 1:06 AM, Michal Hocko [off-list ref] wrote:

On Tue 11-12-12 14:36:10, Ying Han wrote:

quoted

On Tue, Dec 11, 2012 at 7:54 AM, Michal Hocko [off-list ref] wrote:

quoted

On Sun 09-12-12 11:39:50, Ying Han wrote:

quoted

On Mon, Nov 26, 2012 at 10:47 AM, Michal Hocko [off-list ref] wrote:

[...]

quoted

quoted

                if (reclaim) {
-                       iter->position = id;
+                       struct mem_cgroup *curr = memcg;
+
+                       if (last_visited)
+                               css_put(&last_visited->css);

                            ^^^^^^^^^^^
                            here

quoted

quoted

+
+                       if (css && !memcg)
+                               curr = mem_cgroup_from_css(css);
+
+                       /* make sure that the cached memcg is not removed */
+                       if (curr)
+                               css_get(&curr->css);
+                       iter->last_visited = curr;

Here we take extra refcnt for last_visited, and assume it is under
target reclaim which then calls mem_cgroup_iter_break() and we leaked
a refcnt of the target memcg css.

I think you are not right here. The extra reference is kept for
iter->last_visited and it will be dropped the next time somebody sees
the same zone-priority iter. See above.

Or have I missed your question?

Hmm, question remains.

My understanding of the mem_cgroup_iter() is that each call path
should close the loop itself, in the sense that no *leaked* css refcnt
after that loop finished. It is the case for all the caller today
where the loop terminates at memcg == NULL, where all the refcnt have
been dropped by then.

Now I am not sure I understand you. mem_cgroup_iter_break will always
drop the reference of the last returned memcg. So far so good.

Yes, and the patch doesn't change that.

But if

the last memcg got cached in per-zone-priority last_visited then we
_have_ to keep a reference to it regardless we broke out of the loop.
The last_visited thingy is shared between all parallel reclaimers so we
cannot just drop a reference to it.

Agree that the last_visited is shared between all the memcgs accessing
the per-zone-per-iterator.

Also agree that we don't want to drop reference of it if last_visited
is cached after the loop.

But If i look at the callers of mem_cgroup_iter(), they all look like
the following:

memcg = mem_cgroup_iter(root, NULL, &reclaim);
do {

    // do something

    memcg = mem_cgroup_iter(root, memcg, &reclaim);
} while (memcg);

So we get out of the loop when memcg returns as NULL, where the
last_visited is cached as NULL as well thus no css_get(). That is what
I meant by "each reclaim thread closes the loop". If that is true, the
current implementation of mem_cgroup_iter_break() changes that.

quoted

One exception is mem_cgroup_iter_break(), where the loop terminates
with *leaked* refcnt and that is what the iter_break() needs to clean
up. We can not rely on the next caller of the loop since it might
never happen.

Yes, this is true and I already have a half baked patch for that. I
haven't posted it yet but it basically checks all node-zone-prio
last_visited and removes itself from them on the way out in pre_destroy
callback (I just need to cleanup "find a new last_visited" part and will
post it).

Not sure whether that or just change the mem_cgroup_iter_break() by
dropping the refcnt of last_visited.

--Ying

quoted

It makes sense to drop the refcnt of last_visited, the same reason as
drop refcnt of prev. I don't see why it makes different.

Because then it might vanish when somebody else wants to access it. If
we just did mem_cgroup_get which would be enough to keep only memcg part
in memory then what can we do at the time we visit it? css_tryget would
tell us "no your buddy is gone", you do not have any links to the tree
so you would need to start from the beginning. That is what I have
implemented in the first version. Then I've realized that this could
make a bigger pressure on the groups created earlier which doesn't seem
to be right. With css pinning we are sure that there is a link to a next
node in the tree.

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>