Re: [PATCH -mm 02/25] Use an indexed array for LRU variables

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 06 Jun 2008 16:28:40 -0400
Rik van Riel <riel@redhat.com> wrote:

> From: Christoph Lameter <clameter@sgi.com>
> 
> 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.

I would have spat the dummy at pointless churn and code uglification
but I see that we end up with five LRU lsits so ho hum.

>
> ...
>
>  
>  	/* 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];

It'd be a little cache-friendlier to lay this out as

	struct {
		struct list_head list;
		unsigned long nr_scan;
	} lru_stuff[NR_LRU_LISTS];


>  	unsigned long		pages_scanned;	   /* since last reclaim */
>  	unsigned long		flags;		   /* zone flags, see below */
>  
> Index: linux-2.6.26-rc2-mm1/include/linux/mm_inline.h
> ===================================================================
> --- linux-2.6.26-rc2-mm1.orig/include/linux/mm_inline.h	2008-05-23 14:21:21.000000000 -0400
> +++ linux-2.6.26-rc2-mm1/include/linux/mm_inline.h	2008-05-23 14:21:33.000000000 -0400
> @@ -1,40 +1,51 @@
>  static inline void
> +add_page_to_lru_list(struct zone *zone, struct page *page, enum lru_list l)
> +{
> +	list_add(&page->lru, &zone->list[l]);
> +	__inc_zone_state(zone, NR_INACTIVE + l);

                               ^ that's a bug, isn't it?

oh, no it isn't.

Can we rename NR_INACTIVE?  Maybe VMSCAN_BASE or something?

> +}
> +
>
> ...
>
> @@ -945,10 +945,7 @@ static unsigned long shrink_inactive_lis
>  			VM_BUG_ON(PageLRU(page));
>  			SetPageLRU(page);
>  			list_del(&page->lru);
> -			if (PageActive(page))
> -				add_page_to_active_list(zone, page);
> -			else
> -				add_page_to_inactive_list(zone, page);
> +			add_page_to_lru_list(zone, page, PageActive(page));

urgh.  the third arg to add_page_to_lru_list() is an `enum lru_list'
and here we are secretly coercing PageActive()'s boolean return into a
just-happens-to-be-right `enum lru_list'.

That's pretty nasty?

>  			if (!pagevec_add(&pvec, page)) {
>  				spin_unlock_irq(&zone->lru_lock);
>  				__pagevec_release(&pvec);
>
> ...
>
> +static unsigned long shrink_list(enum lru_list l, unsigned long nr_to_scan,
> +	struct zone *zone, struct scan_control *sc, int priority)
> +{
> +	if (l == LRU_ACTIVE) {
> +		shrink_active_list(nr_to_scan, zone, sc, priority);
> +		return 0;
> +	}
> +	return shrink_inactive_list(nr_to_scan, zone, sc);
> +}

I guess a lot of this code gets changed a lot later on.

>
> ...
>