Re: [patch] Revert "memcg: add memory.vmscan_stat"

[KAMEZAWA Hiroyuki]

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/