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

KAMEZAWA Hiroyuki
2011-08-30T10:41:39Z

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.

Thanks,
-Kame