[PATCH] filemap.c fixes

[PATCH] filemap.c fixes

[Rik van Riel]

Hi Linus,

here are a filemap.c fix and a slight addition:

1) __find_page_nolock should only set the referenced bit
   on an active page, otherwise a number of subsequent
   reads from the same page within one page scan interval
   can SEVERELY mess up page aging to the disadvantage of
   the other pages in the system ...
   just setting the referenced bit on the page makes the
   aging a lot fairer

2) in drop_behind() we first increase the page age and
   will then proceed to deactivate the page again; better
   have a simpler help function for this ... note that this
   help function could also be used for eg. ->writepage()
   write clustering code

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

http://www.surriel.com/		http://distro.conectiva.com/

Send all your spam to aardvark@nl.linux.org (spam digging piggy)



--- linux-2.4.5-pre1/mm/filemap.c.orig	Mon May 14 00:55:53 2001
+++ linux-2.4.5-pre1/mm/filemap.c	Mon May 14 00:56:03 2001
@@ -299,13 +299,14 @@
 			break;
 	}
 	/*
-	 * Touching the page may move it to the active list.
-	 * If we end up with too few inactive pages, we wake
-	 * up kswapd.
+	 * Mark the page referenced, moving inactive pages to the
+	 * active list.
 	 */
-	age_page_up(page);
-	if (inactive_shortage() > inactive_target / 2 && free_shortage())
-			wakeup_kswapd();
+	if (!PageActive(page))
+		activate_page(page);
+	else
+		SetPageReferenced(page);
+
 not_found:
 	return page;
 }
@@ -783,8 +784,7 @@
 	 */
 	spin_lock(&pagecache_lock);
 	while (--index >= start) {
-		hash = page_hash(mapping, index);
-		page = __find_page_nolock(mapping, index, *hash);
+		page = __find_page_simple(mapping, index);
 		if (!page)
 			break;
 		deactivate_page(page);

Re: [PATCH] filemap.c fixes

[Daniel Phillips]

On Monday 14 May 2001 06:00, Rik van Riel wrote:

> +	if (!PageActive(page))
> +		activate_page(page);
> +	else
> +		SetPageReferenced(page);
> +

How about:

> +	if (PageActive(page))
> +		SetPageReferenced(page);
> +	else
> +		activate_page(page);

--
Daniel

Re: [PATCH] filemap.c fixes

[Rik van Riel]

On Mon, 14 May 2001, Daniel Phillips wrote:
> On Monday 14 May 2001 06:00, Rik van Riel wrote:
> 
> > +	if (!PageActive(page))
> > +		activate_page(page);
> > +	else
> > +		SetPageReferenced(page);
> > +
> 
> How about:
> 
> > +	if (PageActive(page))
> > +		SetPageReferenced(page);
> > +	else
> > +		activate_page(page);

Fine with me ...

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

http://www.surriel.com/		http://distro.conectiva.com/

Send all your spam to aardvark@nl.linux.org (spam digging piggy)

Re: [PATCH] filemap.c fixes

[Linus Torvalds]

On Mon, 14 May 2001, Rik van Riel wrote:

> On Mon, 14 May 2001, Daniel Phillips wrote:
> > 
> > How about:
> > 
> > > +	if (PageActive(page))
> > > +		SetPageReferenced(page);
> > > +	else
> > > +		activate_page(page);
> 
> Fine with me ...

Now, please explain to me why it's not just a simple

	SetPageReferenced(page);

and then just moving it lazily from one queue to another..

Advantage: fast and robust. Very simple. 

Disadvantage: lazy queue movement. But we're already doing that for other
things (ie page_launder() already has the logic to move pages with counts
and references to the active list). So this is nothing new.

The advantage of doing the work lazily is not just simplicity: it's
actually much _faster_ to delay the work until later, because in many
cases the work never needs to be done at all (ie we might not be low on
memory, or the page ends up being moved for other reasons anyway).

Comments?

		Linus

Re: [PATCH] filemap.c fixes

[Rik van Riel]

On Mon, 14 May 2001, Linus Torvalds wrote:

> > > > +	if (PageActive(page))
> > > > +		SetPageReferenced(page);
> > > > +	else
> > > > +		activate_page(page);

> Now, please explain to me why it's not just a simple
> 
> 	SetPageReferenced(page);
> 
> and then just moving it lazily from one queue to another..

This might get us into problems when we think we have enough
inactive pages to take the next load spike, but we don't.

On the other hand, that's a theoretical situation and I don't
think we'll hit that in practice ... at least, not in workloads
we'd be able to deal nicely with in any way.

Just going with the simple version should work.

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

http://www.surriel.com/		http://distro.conectiva.com/

Send all your spam to aardvark@nl.linux.org (spam digging piggy)

Re: [PATCH] filemap.c fixes

[Linus Torvalds]

On Tue, 15 May 2001, Rik van Riel wrote:
> 
> > Now, please explain to me why it's not just a simple
> > 
> > 	SetPageReferenced(page);
> > 
> > and then just moving it lazily from one queue to another..
> ... 
> Just going with the simple version should work.

Can you and Marcelo fight out the changes you've posted and re-do them
against pre2? I've applied some of the most obvious ones, but some had
a thread of "we should really remove xxx/3" etc that didn't seem to get
finished..

Thanks,

		Linus

Re: [PATCH] filemap.c fixes

[Rik van Riel]

On Tue, 15 May 2001, Linus Torvalds wrote:

> Can you and Marcelo fight out the changes you've posted and re-do them
> against pre2?

OK. Though I don't know when Marcelo and I will be on the same
timezone again (for some reasons his days don't seem to take
24 hours ;)).

> I've applied some of the most obvious ones, but some had a thread of
> "we should really remove xxx/3" etc that didn't seem to get finished..

It should be ok to apply that patch anyway. When we _do_ figure
out which of the two options we should take we'll send you an
incremental patch. Until then we can just leave the "xxx/3" thing
alone.

(in fact, I don't think it'll make much of a difference at all)

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

http://www.surriel.com/		http://distro.conectiva.com/

Send all your spam to aardvark@nl.linux.org (spam digging piggy)