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

[Johannes Weiner <jweiner@redhat.com>]

On Tue, Aug 30, 2011 at 07:34:06PM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 30 Aug 2011 12:17:26 +0200
> Johannes Weiner <jweiner@redhat.com> 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.

Back then I mentioned all my concerns and alternate suggestions.
Different from you, I explained and provided a reason for every single
counter I wanted to add, suggested a basic pattern for how to
interpret them to gain insight into memcg configurations and their
behaviour.  No reaction.  If you want to make progress, than don't
ignore concerns and arguments.  If my arguments are crap, then tell me
why and we can move on.

What we have now is not ready.  It wasn't discussed properly, which
certainly wasn't for the lack of interest in this change.  I just got
tired of raising the same points over and over again without answer.

The amount of time a change has been around is not an argument for it
to get merged.  On the other hand, the fact that it hasn't changed
since April *even though* the implementation was opposed back then
doesn't really speak for your way of getting this upstream, does it?