Re: [PATCH/RFC 5/14] Reclaim Scalability: Use an indexed array for LRU variables

[Balbir Singh <balbir@linux.vnet.ibm.com>]

Lee Schermerhorn wrote:
> On Tue, 2007-09-18 at 00:28 +0530, Balbir Singh wrote:
>> Lee Schermerhorn wrote:
>>> [PATCH/RFC] 05/15  Reclaim Scalability:   Use an indexed array for LRU variables
>>>
>>> From clameter@sgi.com Wed Aug 29 11:39:51 2007
>>>
>>> Currently we are defining explicit variables for the inactive
>>> and active list. An indexed array can be more generic and avoid
>>> repeating similar code in several places in the reclaim code.
>>>
>>> We are saving a few bytes in terms of code size:
>>>
>>> Before:
>>>
>>>    text    data     bss     dec     hex filename
>>> 4097753  573120 4092484 8763357  85b7dd vmlinux
>>>
>>> After:
>>>
>>>    text    data     bss     dec     hex filename
>>> 4097729  573120 4092484 8763333  85b7c5 vmlinux
>>>
>>> Having an easy way to add new lru lists may ease future work on
>>> the reclaim code.
>>>
>>> [CL's signoff added by lts based on mail from CL]
>>> Signed-off-by:  Christoph Lameter <clameter@sgi.com>
>>>
>>>  include/linux/mm_inline.h |   33 ++++++++---
>>>  include/linux/mmzone.h    |   17 +++--
>>>  mm/page_alloc.c           |    9 +--
>>>  mm/swap.c                 |    2 
>>>  mm/vmscan.c               |  132 ++++++++++++++++++++++------------------------
>>>  mm/vmstat.c               |    3 -
>>>  6 files changed, 107 insertions(+), 89 deletions(-)
>>>
>>> Index: Linux/include/linux/mmzone.h
>>> ===================================================================
>>> --- Linux.orig/include/linux/mmzone.h	2007-09-10 12:21:31.000000000 -0400
>>> +++ Linux/include/linux/mmzone.h	2007-09-10 12:22:33.000000000 -0400
>>> @@ -81,8 +81,8 @@ struct zone_padding {
>>>  enum zone_stat_item {
>>>  	/* First 128 byte cacheline (assuming 64 bit words) */
>>>  	NR_FREE_PAGES,
>>> -	NR_INACTIVE,
>>> -	NR_ACTIVE,
>>> +	NR_INACTIVE,	/* must match order of LRU_[IN]ACTIVE */
>>> +	NR_ACTIVE,	/*  "     "     "   "       "         */
>>>  	NR_ANON_PAGES,	/* Mapped anonymous pages */
>>>  	NR_FILE_MAPPED,	/* pagecache pages mapped into pagetables.
>>>  			   only modified from process context */
>>> @@ -106,6 +106,13 @@ enum zone_stat_item {
>>>  #endif
>>>  	NR_VM_ZONE_STAT_ITEMS };
>>>
>>> +enum lru_list {
>>> +	LRU_INACTIVE,	/* must match order of NR_[IN]ACTIVE */
>>> +	LRU_ACTIVE,	/*  "     "     "   "       "        */
>>> +	NR_LRU_LISTS };
>>> +
>>> +#define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
>>> +
>>>  struct per_cpu_pages {
>>>  	int count;		/* number of pages in the list */
>>>  	int high;		/* high watermark, emptying needed */
>>> @@ -259,10 +266,8 @@ struct zone {
>>>
>>>  	/* Fields commonly accessed by the page reclaim scanner */
>>>  	spinlock_t		lru_lock;	
>>> -	struct list_head	active_list;
>>> -	struct list_head	inactive_list;
>>> -	unsigned long		nr_scan_active;
>>> -	unsigned long		nr_scan_inactive;
>>> +	struct list_head	list[NR_LRU_LISTS];
>>> +	unsigned long		nr_scan[NR_LRU_LISTS];
>> I wonder if it makes sense to have an array of the form
>>
>> struct reclaim_lists {
>> 	struct list_head list[NR_LRU_LISTS];
>> 	unsigned long nr_scan[NR_LRU_LISTS];
>> 	reclaim_function_t list_reclaim_function[NR_LRU_LISTS];
>> }
>>
>> where reclaim_function is an array of reclaim functions for each list
>> (in our case shrink_active_list/shrink_inactive_list).
> 
> Are you thinking that memory controller would use the reclaim functions
> switch--e.g., because of it's private lru lists?  And what sort of
> reclaim functions do you have in mind?   Would it add additional
> indirection in the fault path where we add pages to the LRU and move
> them between LRU lists in the case of page activiation?  That could be a
> concern.  In any case, maybe should be named something like 'lru_lists'
> and 'lru_list_functions'?
> 

Well we already have our own isolate function and we re-use
shrink_(in)active_list() functions. The idea behind associating
a function was that shrink_list() would look cleaner. In addition,
the reclaim function for locked page lists would be NULL. I prefer
lru_list_functions.

>>
>>>  static inline void
>>>  del_page_from_lru(struct zone *zone, struct page *page)
>>>  {
>>> +	enum lru_list l = LRU_INACTIVE;
>>> +
>>>  	list_del(&page->lru);
>>>  	if (PageActive(page)) {
>>>  		__ClearPageActive(page);
>>>  		__dec_zone_state(zone, NR_ACTIVE);
>>> -	} else {
>>> -		__dec_zone_state(zone, NR_INACTIVE);
>>> +		l = LRU_ACTIVE;
>>>  	}
>>> +	__dec_zone_state(zone, NR_INACTIVE + l);
>> This is unconditional, does not seem right.
> 
> It's not the unconditional one that wrong.  As Mel pointed out earlier,
> I forgot to remove the explicit decrement of NR_ACTIVE.  Turns out that
> I unknowingly fixed this in the subsequent noreclaim infrastructure
> patch, but I need to fix it in this patch so that it stands alone.  Next
> respin.
> 

Yes, my bad. It's the NR_ACTIVE that is.

> Lee
> 


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>