linux-kernel - Re: [patch] Revert "memcg: add memory.vmscan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20110830193406.361d758a.kamezawa.hiroyu@jp.fujitsu.com>
Date:	Tue, 30 Aug 2011 19:34:06 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	Johannes Weiner <jweiner@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	Balbir Singh <bsingharora@...il.com>,
	Andrew Brestic <abrestic@...gle.com>,
	Ying Han <yinghan@...gle.com>, Michal Hocko <mhocko@...e.cz>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [patch] Revert "memcg: add memory.vmscan_stat"

On Tue, 30 Aug 2011 12:17:26 +0200
Johannes Weiner <jweiner@...hat.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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/