[PATCH] free swap space when (re)activating page

[PATCH] free swap space when (re)activating page

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

The attached patch does what I described in the other thread, it
makes the pageout code free swap space when swap is getting full,
by taking away the swap space from pages that get moved onto or
back onto the active list.

In some tests on a system with 2GB RAM and 1GB swap, it kept the
free swap at 500MB for a 2.3GB qsbench, while without the patch
over 950MB of swap was in use all of the time.

This should give kswapd more flexibility in what to swap out.

What do you think?

Signed-off-by: Rik van Riel <riel@redhat.com>

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

[-- Attachment #2: linux-2.6-swapfree.patch --]
[-- Type: text/x-patch, Size: 1988 bytes --]

--- linux-2.6.20.x86_64/mm/vmscan.c.swapfull	2007-02-16 06:47:02.000000000 -0500
+++ linux-2.6.20.x86_64/mm/vmscan.c	2007-02-16 07:03:30.000000000 -0500
@@ -587,6 +587,9 @@ free_it:
 		continue;
 
 activate_locked:
+		/* Not a candidate for swapping, so reclaim swap space. */
+		if (PageSwapCache(page) && vm_swap_full())
+			remove_exclusive_swap_page(page);
 		SetPageActive(page);
 		pgactivate++;
 keep_locked:
@@ -875,6 +878,11 @@ force_reclaim_mapped:
 		pagevec_strip(&pvec);
 		spin_lock_irq(&zone->lru_lock);
 	}
+	if (vm_swap_full()) {
+		spin_unlock_irq(&zone->lru_lock);
+		pagevec_swap_free(&pvec);
+		spin_lock_irq(&zone->lru_lock);
+	}
 
 	pgmoved = 0;
 	while (!list_empty(&l_active)) {
--- linux-2.6.20.x86_64/mm/swap.c.swapfull	2007-02-16 07:09:38.000000000 -0500
+++ linux-2.6.20.x86_64/mm/swap.c	2007-02-16 07:05:00.000000000 -0500
@@ -420,6 +420,24 @@ void pagevec_strip(struct pagevec *pvec)
 	}
 }
 
+/*
+ * Try to free swap space from the pages in a pagevec
+ */
+void pagevec_swap_free(struct pagevec *pvec)
+{
+	int i;
+
+	for (i = 0; i < pagevec_count(pvec); i++) {
+		struct page *page = pvec->pages[i];
+
+		if (PageSwapCache(page) && !TestSetPageLocked(page)) {
+			if (PageSwapCache(page))
+				remove_exclusive_swap_page(page);
+			unlock_page(page);
+		}
+	}
+}
+
 /**
  * pagevec_lookup - gang pagecache lookup
  * @pvec:	Where the resulting pages are placed
--- linux-2.6.20.x86_64/include/linux/pagevec.h.swapfull	2007-02-16 07:06:29.000000000 -0500
+++ linux-2.6.20.x86_64/include/linux/pagevec.h	2007-02-16 07:06:41.000000000 -0500
@@ -26,6 +26,7 @@ void __pagevec_free(struct pagevec *pvec
 void __pagevec_lru_add(struct pagevec *pvec);
 void __pagevec_lru_add_active(struct pagevec *pvec);
 void pagevec_strip(struct pagevec *pvec);
+void pagevec_swap_free(struct pagevec *pvec);
 unsigned pagevec_lookup(struct pagevec *pvec, struct address_space *mapping,
 		pgoff_t start, unsigned nr_pages);
 unsigned pagevec_lookup_tag(struct pagevec *pvec,

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Fri, 16 Feb 2007, Rik van Riel wrote:

> What do you think?

Looks good apart from one passage (which just vanished when I tried to 
reply, please post patches as inline text).

It was the portion that modifies shrink_active_list. Why operate
on the pagevec there? The pagevec only contains the leftovers to be 
released from scanning over the temporary inactive list.

Re: [PATCH] free swap space when (re)activating page

[Rik van Riel]

Christoph Lameter wrote:
> On Fri, 16 Feb 2007, Rik van Riel wrote:
> 
>> What do you think?
> 
> Looks good apart from one passage (which just vanished when I tried to 
> reply, please post patches as inline text).
> 
> It was the portion that modifies shrink_active_list. Why operate
> on the pagevec there? The pagevec only contains the leftovers to be 
> released from scanning over the temporary inactive list.

Why?  Because the pages that were not referenced will be
going onto the inactive list and are now a candidate for
swapping out.  I don't see why we would want to reclaim
the swap space for pages that area about to be swapped
out again.

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Tue, 20 Feb 2007, Rik van Riel wrote:

> > It was the portion that modifies shrink_active_list. Why operate
> > on the pagevec there? The pagevec only contains the leftovers to be released
> > from scanning over the temporary inactive list.
> 
> Why?  Because the pages that were not referenced will be
> going onto the inactive list and are now a candidate for
> swapping out.  I don't see why we would want to reclaim
> the swap space for pages that area about to be swapped
> out again.

Sounds sane. Then drop that piece. Again, you were only operating on the 
pages left over in the pagevec after the move of the pages to the 
inactive list. If you really wanted to do something there then the 
processing should have covered all pages that go to the inactive list.

Re: [PATCH] free swap space when (re)activating page

[Rik van Riel]

Christoph Lameter wrote:
> On Tue, 20 Feb 2007, Rik van Riel wrote:
> 
>>> It was the portion that modifies shrink_active_list. Why operate
>>> on the pagevec there? The pagevec only contains the leftovers to be released
>>> from scanning over the temporary inactive list.
>> Why?  Because the pages that were not referenced will be
>> going onto the inactive list and are now a candidate for
>> swapping out.  I don't see why we would want to reclaim
>> the swap space for pages that area about to be swapped
>> out again.
> 
> Sounds sane. Then drop that piece. Again, you were only operating on the 
> pages left over in the pagevec after the move of the pages to the 
> inactive list. If you really wanted to do something there then the 
> processing should have covered all pages that go to the inactive list.

Nono, I try to remove the swap space occupied by pages that
go back onto the active list.  Regardless of whether they
were already there, or whether they started out on the
inactive list.

Stripping the swap space of the pages that are going to
the inactive list makes less sense IMHO, because those
pages are candidates for swapping out - meaning those
should keep the space.

-- 
All Rights Reversed

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Tue, 20 Feb 2007, Rik van Riel wrote:

> Nono, I try to remove the swap space occupied by pages that
> go back onto the active list.  Regardless of whether they
> were already there, or whether they started out on the
> inactive list.

Ok then do it for all pages that go back not just for those leftover from 
the moving of pages to the inactive list (why would you move those???)

> Stripping the swap space of the pages that are going to
> the inactive list makes less sense IMHO, because those
> pages are candidates for swapping out - meaning those
> should keep the space.

Just trying to figure out what your patch does there and it does not make 
much sense to me so far.

Maybe the hunk does apply in a different location than I thought. If you 
do that in the loop over the pages on active list then it would make 
sense. But in that case you need another piece of it doing the same to the 
pages that are released at the end of shrink_active_list().

Re: [PATCH] free swap space when (re)activating page

[Rik van Riel]

Christoph Lameter wrote:
> On Tue, 20 Feb 2007, Rik van Riel wrote:
> 
>> Nono, I try to remove the swap space occupied by pages that
>> go back onto the active list.  Regardless of whether they
>> were already there, or whether they started out on the
>> inactive list.
> 
> Ok then do it for all pages that go back not just for those leftover from 
> the moving of pages to the inactive list (why would you move those???)

I do.  The only pages that are exempt are the pages that move
from the active list to the inactive list, because those will
probably be evicted soon enough.

> Maybe the hunk does apply in a different location than I thought.

I suspect that's the case ...

> If you 
> do that in the loop over the pages on active list then it would make 
> sense. But in that case you need another piece of it doing the same to the 
> pages that are released at the end of shrink_active_list().

... because I think this is what my patch does :)

-- 
All Rights Reversed

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Tue, 20 Feb 2007, Rik van Riel wrote:

> > Maybe the hunk does apply in a different location than I thought.
> 
> I suspect that's the case ...

No that is not the case:

@@ -875,6 +878,11 @@ force_reclaim_mapped:
 		pagevec_strip(&pvec);
 		spin_lock_irq(&zone->lru_lock);
 	}
+	if (vm_swap_full()) {
+		spin_unlock_irq(&zone->lru_lock);
+		pagevec_swap_free(&pvec);
+		spin_lock_irq(&zone->lru_lock);
+	}
 
 	pgmoved = 0;
 	while (!list_empty(&l_active)) {

So you do the swap free on the pages leftover from moving to the inactive 
list and not to the pages that will be moved to the active list. It does 
not do what you wanted to be done.

You need to

1. Do the pagevec_swap_free in the loop over the active pages

2. At end of the loop over the active pages do something like the above.

Re: [PATCH] free swap space when (re)activating page

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

Rik van Riel wrote:

> ... because I think this is what my patch does :)

Never mind, I see it now.

The attached patch should be correct.

Btw, why do we not call pagevec_strip on the pages on l_active?
I assume we want to reclaim their buffer heads, too...

-- 
All Rights Reversed

[-- Attachment #2: linux-2.6-swapfree.patch --]
[-- Type: text/x-patch, Size: 2259 bytes --]

--- linux-2.6.20.noarch/mm/vmscan.c.swapfree	2007-02-20 06:44:13.000000000 -0500
+++ linux-2.6.20.noarch/mm/vmscan.c	2007-02-20 06:54:10.000000000 -0500
@@ -587,6 +587,9 @@ free_it:
 		continue;
 
 activate_locked:
+		/* Not a candidate for swapping, so reclaim swap space. */
+		if (PageSwapCache(page) && vm_swap_full())
+			remove_exclusive_swap_page(page);
 		SetPageActive(page);
 		pgactivate++;
 keep_locked:
@@ -889,6 +892,8 @@ force_reclaim_mapped:
 			__mod_zone_page_state(zone, NR_ACTIVE, pgmoved);
 			pgmoved = 0;
 			spin_unlock_irq(&zone->lru_lock);
+			if (vm_swap_full())
+				pagevec_swap_free(&pvec);
 			__pagevec_release(&pvec);
 			spin_lock_irq(&zone->lru_lock);
 		}
@@ -899,6 +904,8 @@ force_reclaim_mapped:
 	__count_vm_events(PGDEACTIVATE, pgdeactivate);
 	spin_unlock_irq(&zone->lru_lock);
 
+	if (vm_swap_full())
+		pagevec_swap_free(&pvec);
 	pagevec_release(&pvec);
 }
 
--- linux-2.6.20.noarch/mm/swap.c.swapfree	2007-02-04 13:44:54.000000000 -0500
+++ linux-2.6.20.noarch/mm/swap.c	2007-02-20 06:44:17.000000000 -0500
@@ -420,6 +420,26 @@ void pagevec_strip(struct pagevec *pvec)
 	}
 }
 
+/*
+ * Try to free swap space from the pages in a pagevec
+ */
+void pagevec_swap_free(struct pagevec *pvec)
+{
+	int i;
+
+	for (i = 0; i < pagevec_count(pvec); i++) {
+		struct page *page = pvec->pages[i];
+
+		if (PageSwapCache(page) && !TestSetPageLocked(page)) {
+			if (PageSwapCache(page))
+				remove_exclusive_swap_page(page);
+			unlock_page(page);
+			if (printk_ratelimit())
+				printk("kswapd freed a swap space\n");
+		}
+	}
+}
+
 /**
  * pagevec_lookup - gang pagecache lookup
  * @pvec:	Where the resulting pages are placed
--- linux-2.6.20.noarch/include/linux/pagevec.h.swapfree	2007-02-04 13:44:54.000000000 -0500
+++ linux-2.6.20.noarch/include/linux/pagevec.h	2007-02-20 06:44:17.000000000 -0500
@@ -26,6 +26,7 @@ void __pagevec_free(struct pagevec *pvec
 void __pagevec_lru_add(struct pagevec *pvec);
 void __pagevec_lru_add_active(struct pagevec *pvec);
 void pagevec_strip(struct pagevec *pvec);
+void pagevec_swap_free(struct pagevec *pvec);
 unsigned pagevec_lookup(struct pagevec *pvec, struct address_space *mapping,
 		pgoff_t start, unsigned nr_pages);
 unsigned pagevec_lookup_tag(struct pagevec *pvec,

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Tue, 20 Feb 2007, Rik van Riel wrote:

> The attached patch should be correct.

Oh. It vanished again when I replied to your mail.
 
> Btw, why do we not call pagevec_strip on the pages on l_active?
> I assume we want to reclaim their buffer heads, too...

Yes we want to reduce buffer heads if we are over the limit.

Re: [PATCH] free swap space when (re)activating page

[Christoph Lameter]

On Tue, 20 Feb 2007, Rik van Riel wrote:

 > Btw, why do we not call pagevec_strip on the pages on l_active?
> I assume we want to reclaim their buffer heads, too...

But those buffer heads may be used soon. So its better to leave them 
alone.

Re: [PATCH] free swap space when (re)activating page

[William Lee Irwin III]

On Fri, Feb 16, 2007 at 05:46:29PM -0500, Rik van Riel wrote:
> The attached patch does what I described in the other thread, it
> makes the pageout code free swap space when swap is getting full,
> by taking away the swap space from pages that get moved onto or
> back onto the active list.
> In some tests on a system with 2GB RAM and 1GB swap, it kept the
> free swap at 500MB for a 2.3GB qsbench, while without the patch
> over 950MB of swap was in use all of the time.
> This should give kswapd more flexibility in what to swap out.
> What do you think?
> Signed-off-by: Rik van Riel <riel@redhat.com>

I would call this a bugfix, not an optimization.


-- wli

Re: [PATCH] free swap space when (re)activating page

[Al Boldi]

Rik van Riel wrote:
> Rik van Riel wrote:
> > ... because I think this is what my patch does :)
>
> Never mind, I see it now.
>
> The attached patch should be correct.

Your patch seems to improve the situation a little bit, but the numbers still 
look weird, especially for swap-in, which gets progressively slower.

RAM 512mb , SWAP 1G
#mount -t tmpfs -o size=1G none /dev/shm
#time cat /dev/full > /dev/shm/x.dmp
15sec
#time cat /dev/shm/x.dmp > /dev/null
58sec
#time cat /dev/shm/x.dmp > /dev/null
72sec
#time cat /dev/shm/x.dmp > /dev/null
85sec
#time cat /dev/shm/x.dmp > /dev/null
93sec
#time cat /dev/shm/x.dmp > /dev/null
99sec


Thanks!

--
Al

Re: [PATCH] free swap space when (re)activating page

[Bodo Eggert]

Rik van Riel <riel@redhat.com> wrote:

+++ linux-2.6.20.noarch/mm/swap.c        2007-02-20 06:44:17.000000000 -0500
@@ -420,6 +420,26 @@ void pagevec_strip(struct pagevec *pvec)

> +                        if (printk_ratelimit())
> +                                printk("kswapd freed a swap space\n");
> 

NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO!!!!!!!!!!!

1) This message is a debug message! You forgot to set the printk level.

2) The message text is bad, the average log reader only knows swap files
   and pages in swapfiles. The first reaction will be like "WTF happened
   to my swap???". Thousands of admins will cry out in anguish trying to
   get the meaning of this message, and again cry out in wrath after they
   found out.

3) What should I do if I see this message? It's neither good, nor bad for
   me, nor is it in any way informative even if it were changed to be
   meaningfull. It's utterly useless! Let it be!

-- 
We are all born ignorant, but one must work hard to remain stupid.
        -- Benjamin Franklin

Friß, Spammer: JEN-ersdiT@kowaZ.7eggert.dyndns.org

Re: [PATCH] free swap space when (re)activating page

[Rik van Riel]

Bodo Eggert wrote:
> Rik van Riel <riel@redhat.com> wrote:
> 
> +++ linux-2.6.20.noarch/mm/swap.c        2007-02-20 06:44:17.000000000 -0500
> @@ -420,6 +420,26 @@ void pagevec_strip(struct pagevec *pvec)
> 
>> +                        if (printk_ratelimit())
>> +                                printk("kswapd freed a swap space\n");
>>
> 
> NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO!!!!!!!!!!!
> 
> 1) This message is a debug message! You forgot to set the printk level.

Doh, I forgot to cut it out of the patch when I fixed it
according to Christoph's hint.

This chunk should be removed from the patch...

Andrew, I'll send you a new one tomorrow morning.

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.