LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@osdl.org>
To: Andy Whitcroft <apw@shadowen.org>
Cc: linux-mm@kvack.org, Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Mel Gorman <mel@csn.ul.ie>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/4] Lumpy Reclaim V3
Date: Mon, 11 Dec 2006 15:29:07 -0800	[thread overview]
Message-ID: <20061211152907.f44cdd94.akpm@osdl.org> (raw)
In-Reply-To: <exportbomb.1165424343@pinky>

On Wed, 6 Dec 2006 16:59:04 +0000
Andy Whitcroft <apw@shadowen.org> wrote:

> This is a repost of the lumpy reclaim patch set.  This is
> basically unchanged from the last post, other than being rebased
> to 2.6.19-rc2-mm2.

The patch sequencing appeared to be designed to make the code hard to
review, so I clumped them all into a single diff:

>  
>  /*
> + * Attempt to remove the specified page from its LRU.  Only take this
> + * page if it is of the appropriate PageActive status.  Pages which
> + * are being freed elsewhere are also ignored.
> + *
> + * @page:	page to consider
> + * @active:	active/inactive flag only take pages of this type

I dunno who started adding these @'s into non-kernel-doc comments.  I'll
un-add them.

> + * returns 0 on success, -ve errno on failure.
> + */
> +int __isolate_lru_page(struct page *page, int active)
> +{
> +	int ret = -EINVAL;
> +
> +	if (PageLRU(page) && (PageActive(page) == active)) {

We hope that all architectures remember that test_bit returns 0 or
1.  We got that wrong a few years back.  What we do now is rather
un-C-like.  And potentially inefficient.  Hopefully the compiler usually
sorts it out though.


> +		ret = -EBUSY;
> +		if (likely(get_page_unless_zero(page))) {
> +			/*
> +			 * Be careful not to clear PageLRU until after we're
> +			 * sure the page is not being freed elsewhere -- the
> +			 * page release code relies on it.
> +			 */
> +			ClearPageLRU(page);
> +			ret = 0;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/*
>   * zone->lru_lock is heavily contended.  Some of the functions that
>   * shrink the lists perform better by taking out a batch of pages
>   * and working on them outside the LRU lock.
> @@ -621,33 +653,71 @@ keep:
>   */
>  static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
>  		struct list_head *src, struct list_head *dst,
> -		unsigned long *scanned)
> +		unsigned long *scanned, int order)
>  {
>  	unsigned long nr_taken = 0;
> -	struct page *page;
> -	unsigned long scan;
> +	struct page *page, *tmp;

"tmp" isn't a very good identifier.

> +	unsigned long scan, pfn, end_pfn, page_pfn;

One declaration per line is preferred.  This gives you room for a brief
comment, where appropriate.


> +		/*
> +		 * Attempt to take all pages in the order aligned region
> +		 * surrounding the tag page.  Only take those pages of
> +		 * the same active state as that tag page.
> +		 */
> +		zone_id = page_zone_id(page);
> +		page_pfn = __page_to_pfn(page);
> +		pfn = page_pfn & ~((1 << order) - 1);

Is this always true?  It assumes that the absolute value of the starting
pfn of each zone is a multiple of MAX_ORDER (doesn't it?) I don't see any
reason per-se why that has to be true (although it might be).

hm, I guess it has to be true, else hugetlb pages wouldn't work too well.

> +		end_pfn = pfn + (1 << order);
> +		for (; pfn < end_pfn; pfn++) {
> +			if (unlikely(pfn == page_pfn))
> +				continue;
> +			if (unlikely(!pfn_valid(pfn)))
> +				break;
> +
> +			tmp = __pfn_to_page(pfn);
> +			if (unlikely(page_zone_id(tmp) != zone_id))
> +				continue;
> +			scan++;
> +			switch (__isolate_lru_page(tmp, active)) {
> +			case 0:
> +				list_move(&tmp->lru, dst);
> +				nr_taken++;
> +				break;
> +
> +			case -EBUSY:
> +				/* else it is being freed elsewhere */
> +				list_move(&tmp->lru, src);
> +			default:
> +				break;
> +			}
> +		}

I think each of those

			if (expr)
				continue;

statements would benefit from a nice comment explaining why.


This physical-scan part of the function will skip pages which happen to be
on *src.  I guess that won't matter much, once the sytem has been up for a
while and the LRU is nicely scrambled.


If this function is passed a list of 32 pages, and order=4, I think it will
go and give us as many as 512 pages on *dst?  A check of nr_taken might be
needed.


The patch is pretty simple, isn't it?

I guess a shortcoming is that it doesn't address the situation where
GFP_ATOMIC network rx is trying to allocate order-2 pages for large skbs,
but kswapd doesn't know that.  AFACIT nobody will actually run the nice new
code in this quite common scenario.


  parent reply	other threads:[~2006-12-11 23:29 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-12-06 16:59 Andy Whitcroft
2006-12-06 16:59 ` [PATCH 1/4] lumpy reclaim v2 Andy Whitcroft
2006-12-15  4:57   ` Andrew Morton
2007-01-26 11:00     ` Andrew Morton
2006-12-06 17:00 ` [PATCH 2/4] lumpy cleanup a missplaced comment and simplify some code Andy Whitcroft
2006-12-06 17:00 ` [PATCH 3/4] lumpy ensure we respect zone boundaries Andy Whitcroft
2006-12-06 17:01 ` [PATCH 4/4] lumpy take the other active inactive pages in the area Andy Whitcroft
2006-12-11 23:29 ` Andrew Morton [this message]
2007-01-29 12:24   ` [PATCH 0/4] Lumpy Reclaim V3 Andy Whitcroft
2006-12-12 11:13 ` Andrew Morton
2006-12-12 13:14   ` Andy Whitcroft
2007-01-29 12:25   ` Andy Whitcroft
  -- strict thread matches above, loose matches on Subject: below --
2006-11-23 16:48 Andy Whitcroft
2006-11-23 19:02 ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20061211152907.f44cdd94.akpm@osdl.org \
    --to=akpm@osdl.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=apw@shadowen.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mel@csn.ul.ie \
    --subject='Re: [PATCH 0/4] Lumpy Reclaim V3' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).