Messages in this thread | | | Date | Thu, 20 Aug 2009 09:34:18 +0800 | From | Wu Fengguang <> | Subject | Re: [RFC] memcg: move definitions to .h and inline some functions |
| |
On Wed, Aug 19, 2009 at 10:27:05PM +0800, Balbir Singh wrote: > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-08-19 23:18:01]: > > > Wu Fengguang ?$B$5$s$O=q$-$^$7$?!' > > > On Tue, Aug 18, 2009 at 11:57:52PM +0800, KOSAKI Motohiro wrote: > > >> > > >> > > This one of the reasons why we unconditionally deactivate > > >> > > the active anon pages, and do background scanning of the > > >> > > active anon list when reclaiming page cache pages. > > >> > > > > >> > > We want to always move some pages to the inactive anon > > >> > > list, so it does not get too small. > > >> > > > >> > Right, the current code tries to pull inactive list out of > > >> > smallish-size state as long as there are vmscan activities. > > >> > > > >> > However there is a possible (and tricky) hole: mem cgroups > > >> > don't do batched vmscan. shrink_zone() may call shrink_list() > > >> > with nr_to_scan=1, in which case shrink_list() _still_ calls > > >> > isolate_pages() with the much larger SWAP_CLUSTER_MAX. > > >> > > > >> > It effectively scales up the inactive list scan rate by 10 times when > > >> > it is still small, and may thus prevent it from growing up for ever. > > >> > > > >> > In that case, LRU becomes FIFO. > > >> > > > >> > Jeff, can you confirm if the mem cgroup's inactive list is small? > > >> > If so, this patch should help. > > >> > > >> This patch does right thing. > > >> However, I would explain why I and memcg folks didn't do that in past > > >> days. > > >> > > >> Strangely, some memcg struct declaration is hide in *.c. Thus we can't > > >> make inline function and we hesitated to introduce many function calling > > >> overhead. > > >> > > >> So, Can we move some memcg structure declaration to *.h and make > > >> mem_cgroup_get_saved_scan() inlined function? > > > > > > OK here it is. I have to move big chunks to make it compile, and it > > > does reduced a dozen lines of code :) > > > > > > Is this big copy&paste acceptable? (memcg developers CCed). > > > > > > Thanks, > > > Fengguang > > > > I don't like this. plz add hooks to necessary places, at this stage. > > This will be too big for inlined function, anyway. > > plz move this after you find overhead is too big.
It shall not be a performance regression, since the text size is slightly smaller with the patch:
text data bss dec hex filename before 8732148 2771858 11048432 22552438 1581f76 vmlinux after 8731972 2771858 11048432 22552262 1581ec6 vmlinux
> Me too.. I want to abstract the implementation within memcontrol.c to > be honest (I am concerned that someone might include memcontrol.h and > access its structure members, which scares me). Hiding it within > memcontrol.c provides the right level of abstraction.
Yeah quite reasonable.
> Could you please explain your motivation for this change? I got cc'ed > on to a few emails, is this for the patch that export nr_save_scanned > approach?
Yes, KOSAKI proposed to inline the mem_cgroup_get_saved_scan() function introduced in that patch, which requires moving the structs into .h
I'll submit the original (un-inlined) patch.
Thanks, Fengguang
| |