[patch] mm: move pagevec stripping to save unlock-relock

[patch] mm: move pagevec stripping to save unlock-relock

[Johannes Weiner]

In shrink_active_list() after the deactivation loop, we strip buffer
heads from the potentially remaining pages in the pagevec.

Currently, this drops the zone's lru lock for stripping, only to
reacquire it again afterwards to update statistics.

It is not necessary to strip the pages before updating the stats, so
move the whole thing out of the protected region and save the extra
locking.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/vmscan.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1298,14 +1298,11 @@ static void shrink_active_list(unsigned 
 	}
 	__mod_zone_page_state(zone, NR_LRU_BASE + lru, pgmoved);
 	pgdeactivate += pgmoved;
-	if (buffer_heads_over_limit) {
-		spin_unlock_irq(&zone->lru_lock);
-		pagevec_strip(&pvec);
-		spin_lock_irq(&zone->lru_lock);
-	}
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
 	__count_vm_events(PGDEACTIVATE, pgdeactivate);
 	spin_unlock_irq(&zone->lru_lock);
+	if (buffer_heads_over_limit)
+		pagevec_strip(&pvec);
 	if (vm_swap_full())
 		pagevec_swap_free(&pvec);
 

Re: [patch] mm: move pagevec stripping to save unlock-relock

[MinChan Kim]

Nice catch!

Reviewed-by: MinChan Kim <minchan.kim@gmail.com>

On Wed, Feb 25, 2009 at 11:38 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> In shrink_active_list() after the deactivation loop, we strip buffer
> heads from the potentially remaining pages in the pagevec.
>
> Currently, this drops the zone's lru lock for stripping, only to
> reacquire it again afterwards to update statistics.
>
> It is not necessary to strip the pages before updating the stats, so
> move the whole thing out of the protected region and save the extra
> locking.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/vmscan.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1298,14 +1298,11 @@ static void shrink_active_list(unsigned
>        }
>        __mod_zone_page_state(zone, NR_LRU_BASE + lru, pgmoved);
>        pgdeactivate += pgmoved;
> -       if (buffer_heads_over_limit) {
> -               spin_unlock_irq(&zone->lru_lock);
> -               pagevec_strip(&pvec);
> -               spin_lock_irq(&zone->lru_lock);
> -       }
>        __count_zone_vm_events(PGREFILL, zone, pgscanned);
>        __count_vm_events(PGDEACTIVATE, pgdeactivate);
>        spin_unlock_irq(&zone->lru_lock);
> +       if (buffer_heads_over_limit)
> +               pagevec_strip(&pvec);
>        if (vm_swap_full())
>                pagevec_swap_free(&pvec);
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>



-- 
Kinds regards,
MinChan Kim

[patch] mm: don't free swap slots on page deactivation

[Johannes Weiner]

The pagevec_swap_free() at the end of shrink_active_list() was
introduced in 68a22394 "vmscan: free swap space on swap-in/activation"
when shrink_active_list() was still rotating referenced active pages.

In 7e9cd48 "vmscan: fix pagecache reclaim referenced bit check" this
was changed, the rotating removed but the pagevec_swap_free() after
the rotation loop was forgotten, applying now to the pagevec of the
deactivation loop instead.

Now swap space is freed for deactivated pages.  And only for those
that happen to be on the pagevec after the deactivation loop.

Complete 7e9cd48 and remove the rest of the swap freeing.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/vmscan.c |    3 ---
 1 file changed, 3 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1303,9 +1303,6 @@ static void shrink_active_list(unsigned 
 	spin_unlock_irq(&zone->lru_lock);
 	if (buffer_heads_over_limit)
 		pagevec_strip(&pvec);
-	if (vm_swap_full())
-		pagevec_swap_free(&pvec);
-
 	pagevec_release(&pvec);
 }
 

Re: [patch] mm: don't free swap slots on page deactivation

[Hugh Dickins]

On Wed, 25 Feb 2009, Johannes Weiner wrote:

> The pagevec_swap_free() at the end of shrink_active_list() was
> introduced in 68a22394 "vmscan: free swap space on swap-in/activation"
> when shrink_active_list() was still rotating referenced active pages.
> 
> In 7e9cd48 "vmscan: fix pagecache reclaim referenced bit check" this
> was changed, the rotating removed but the pagevec_swap_free() after
> the rotation loop was forgotten, applying now to the pagevec of the
> deactivation loop instead.
> 
> Now swap space is freed for deactivated pages.  And only for those
> that happen to be on the pagevec after the deactivation loop.
> 
> Complete 7e9cd48 and remove the rest of the swap freeing.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Rik van Riel <riel@redhat.com>

Nice observation.  I was going to object that the original code was
indifferent to whether it was freeing swap from active or inactive,
they all got lumped into the same pvec.  But that was just an oversight
in the original code: you're right that if it was our intention to free
swap from inactive pages here (when vm_swap_full), then we'd be freeing
it from them in the loop above (where the buffer_heads_over_limit
pagevec_strip is done).

Once upon a time (early 2007), testing an earlier incarnation of that
code, I did find almost nothing being freed by that pagevec_swap_free
anyway: other vm_swap_full frees were being effective, effective
enough to render this one rather pointless, even when it was operating
as intended.  But I never got around to checking on that in 2008's
splitLRU patches, and a lot changed in between: I may be misleading.

If Rik agrees (I think these do need his Ack), note that there are
no other users of pagevec_swap_free, so you'd do well to remove it
from mm/swap.c and include/linux/pagevec.h - I can well imagine us
wanting to bring it back some time, but can easily look it up when
and if we do need it again in the future.

Hugh

> ---
>  mm/vmscan.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1303,9 +1303,6 @@ static void shrink_active_list(unsigned 
>  	spin_unlock_irq(&zone->lru_lock);
>  	if (buffer_heads_over_limit)
>  		pagevec_strip(&pvec);
> -	if (vm_swap_full())
> -		pagevec_swap_free(&pvec);
> -
>  	pagevec_release(&pvec);
>  }
>  

Re: [patch] mm: don't free swap slots on page deactivation

[Rik van Riel]

Johannes Weiner wrote:
> The pagevec_swap_free() at the end of shrink_active_list() was
> introduced in 68a22394 "vmscan: free swap space on swap-in/activation"
> when shrink_active_list() was still rotating referenced active pages.
> 
> In 7e9cd48 "vmscan: fix pagecache reclaim referenced bit check" this
> was changed, the rotating removed but the pagevec_swap_free() after
> the rotation loop was forgotten, applying now to the pagevec of the
> deactivation loop instead.
> 
> Now swap space is freed for deactivated pages.  And only for those
> that happen to be on the pagevec after the deactivation loop.
> 
> Complete 7e9cd48 and remove the rest of the swap freeing.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Rik van Riel <riel@redhat.com>

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

-- 
All rights reversed.

[PATCH for mmotm] remove pagevec_swap_free()

[KOSAKI Motohiro]

> On Wed, 25 Feb 2009, Johannes Weiner wrote:
> 
> > The pagevec_swap_free() at the end of shrink_active_list() was
> > introduced in 68a22394 "vmscan: free swap space on swap-in/activation"
> > when shrink_active_list() was still rotating referenced active pages.
> > 
> > In 7e9cd48 "vmscan: fix pagecache reclaim referenced bit check" this
> > was changed, the rotating removed but the pagevec_swap_free() after
> > the rotation loop was forgotten, applying now to the pagevec of the
> > deactivation loop instead.
> > 
> > Now swap space is freed for deactivated pages.  And only for those
> > that happen to be on the pagevec after the deactivation loop.
> > 
> > Complete 7e9cd48 and remove the rest of the swap freeing.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Rik van Riel <riel@redhat.com>
> 
> Nice observation.  I was going to object that the original code was
> indifferent to whether it was freeing swap from active or inactive,
> they all got lumped into the same pvec.  But that was just an oversight
> in the original code: you're right that if it was our intention to free
> swap from inactive pages here (when vm_swap_full), then we'd be freeing
> it from them in the loop above (where the buffer_heads_over_limit
> pagevec_strip is done).
> 
> Once upon a time (early 2007), testing an earlier incarnation of that
> code, I did find almost nothing being freed by that pagevec_swap_free
> anyway: other vm_swap_full frees were being effective, effective
> enough to render this one rather pointless, even when it was operating
> as intended.  But I never got around to checking on that in 2008's
> splitLRU patches, and a lot changed in between: I may be misleading.
>
> If Rik agrees (I think these do need his Ack), note that there are
> no other users of pagevec_swap_free, so you'd do well to remove it
> from mm/swap.c and include/linux/pagevec.h - I can well imagine us
> wanting to bring it back some time, but can easily look it up when
> and if we do need it again in the future.

Yup, removing is better.


==========
Subject: [PATCH] remove pagevec_swap_free()

pagevec_swap_free() is unused. 
then it can be removed.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Hugh Dickins <hugh@veritas.com>
---
 include/linux/pagevec.h |    1 -
 mm/swap.c               |   23 -----------------------
 2 files changed, 0 insertions(+), 24 deletions(-)

diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
index 7b2886f..bab82f4 100644
--- a/include/linux/pagevec.h
+++ b/include/linux/pagevec.h
@@ -24,7 +24,6 @@ void __pagevec_release(struct pagevec *pvec);
 void __pagevec_free(struct pagevec *pvec);
 void ____pagevec_lru_add(struct pagevec *pvec, enum lru_list lru);
 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,
diff --git a/mm/swap.c b/mm/swap.c
index eee08df..d33e499 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -471,29 +471,6 @@ void pagevec_strip(struct pagevec *pvec)
 }
 
 /**
- * pagevec_swap_free - try to free swap space from the pages in a pagevec
- * @pvec: pagevec with swapcache pages to free the swap space of
- *
- * The caller needs to hold an extra reference to each page and
- * not hold the page lock on the pages.  This function uses a
- * trylock on the page lock so it may not always free the swap
- * space associated with a page.
- */
-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) && trylock_page(page)) {
-			try_to_free_swap(page);
-			unlock_page(page);
-		}
-	}
-}
-
-/**
  * pagevec_lookup - gang pagecache lookup
  * @pvec:	Where the resulting pages are placed
  * @mapping:	The address_space to search
-- 
1.6.0.6

Re: [PATCH for mmotm] remove pagevec_swap_free()

[Hugh Dickins]

On Sun, 1 Mar 2009, KOSAKI Motohiro wrote:
> Subject: [PATCH] remove pagevec_swap_free()
> 
> pagevec_swap_free() is unused. 
> then it can be removed.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Following on from your patch to remove its use, yes.

Acked-by: Hugh Dickins <hugh@veritas.com>

Thanks,
Hugh