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

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]

Date:	Fri, 6 Jun 2008 18:04:26 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Rik van Riel <riel@...hat.com>
Cc:	linux-kernel@...r.kernel.org, lee.schermerhorn@...com,
	kosaki.motohiro@...fujitsu.com, clameter@....com
Subject: Re: [PATCH -mm 02/25] Use an indexed array for LRU variables

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

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

>
> ...
>

--
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/