On Tue, 30 Aug 2011 12:17:26 +0200
Johannes Weiner [off-list ref] wrote:

On Tue, Aug 30, 2011 at 05:56:09PM +0900, KAMEZAWA Hiroyuki wrote:

quoted

quoted

quoted

quoted

I don't get why this has to be done completely different from the way
we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem
pointer.

I'm sorry I miss something. Do you say to add a function like

mem_cgroup_record_reclaim_stat(memcg, root_mem, anon_scan, anon_free, anon_rotate,
                               file_scan, file_free, elapsed_ns)

?

Exactly, though passing it a stat item index and a delta would
probably be closer to our other statistics accounting, i.e.:

	mem_cgroup_record_reclaim_stat(sc->mem_cgroup, sc->root_mem_cgroup,
				       MEM_CGROUP_SCAN_ANON, *nr_anon);

where sc->mem_cgroup is `victim' and sc->root_mem_cgroup is `root_mem'
from hierarchical_reclaim.  ->root_mem_cgroup might be confusing,
though.  I named it ->target_mem_cgroup in my patch set but I don't
feel too strongly about that.

Even better would be to reuse enum vm_event_item and at one point
merge all the accounting stuff into a single function and have one
single set of events that makes sense on a global level as well as on
a per-memcg level.

There is deviation and implementing similar things twice with slight
variations and I don't see any justification for all that extra code
that needs maintaining.  Or counters that have similar names globally
and on a per-memcg level but with different meanings, like the rotated
counter.  Globally, a rotated page (PGROTATED) is one that is moved
back to the inactive list after writeback finishes.  Per-memcg, the
rotated counter is our internal heuristics value to balance pressure
between LRUs and means either rotated on the inactive list, activated,
not activated but countes as activated because of VM_EXEC etc.

I am still for reverting this patch before the release until we have
this all sorted out.  I feel rather strongly that these statistics are
in no way ready to make them part of the ABI and export them to
userspace as they are now.

How about fixing interface first ? 1st version of this patch was 
in April and no big change since then.
I don't want to be starved more.

Thanks,
-Kame



--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>