lkml.org 
[lkml]   [2008]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH -mm 02/25] Use an indexed array for LRU variables
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.

>
> ...
>



\
 
 \ /
  Last update: 2008-06-07 03:07    [W:0.327 / U:5.176 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site