On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <jweiner@xxxxxxxxxx> wrote: > On Tue, Aug 30, 2011 at 05:56:09PM +0900, KAMEZAWA Hiroyuki wrote: > > > > > 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@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>