[PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
sc->isolate_pages() return value properly. Add similar proper return value check for
shrink_active_list() sc->isolate_pages().

Signed-off-by: Vincent Li <macli@brc.ubc.ca>
---
 mm/vmscan.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 460a6f7..2d1c846 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	if (scanning_global_lru(sc)) {
 		zone->pages_scanned += pgscanned;
 	}
-	reclaim_stat->recent_scanned[file] += nr_taken;
-
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
+
+	if (nr_taken == 0)
+		goto done;
+
+	reclaim_stat->recent_scanned[file] += nr_taken;
 	if (file)
 		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
 	else
@@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
 	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
 	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
+
+done:
 	spin_unlock_irq(&zone->lru_lock);
 }
 
-- 
1.6.0.4

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

On Mon, 31 Aug 2009 15:54:01 -0700
Vincent Li <macli@brc.ubc.ca> wrote:

> commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> sc->isolate_pages() return value properly. Add similar proper return value check for
> shrink_active_list() sc->isolate_pages().
> 
> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

You should have write down your patch's effect clearly
in changelog although it's easy. ;-)

-- 
Kind regards,
Minchan Kim

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Rik van Riel]

Vincent Li wrote:
> commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> sc->isolate_pages() return value properly. Add similar proper return value check for
> shrink_active_list() sc->isolate_pages().
> 
> Signed-off-by: Vincent Li <macli@brc.ubc.ca>

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

-- 
All rights reversed.

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Tue, 1 Sep 2009, Minchan Kim wrote:

> On Mon, 31 Aug 2009 15:54:01 -0700
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> > sc->isolate_pages() return value properly. Add similar proper return value check for
> > shrink_active_list() sc->isolate_pages().
> > 
> > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> 
> You should have write down your patch's effect clearly
> in changelog although it's easy. ;-)

Yes, I should have. This patch is inspired by Kosaki's patch, I 
thought mentioning that commit would make this patch as follow-up work and 
changelog clear enough. Would following changelog ok?

----
Add proper return value check for shrink_active_list() 
sc->isolate_pages(). 

When "nr_taken == 0"
	1: nr_scan related statistics should still be caculated.
	2: jump to the end of function and release zone->lru_lock.
----
Regards,

Vincent Li
Biomedical Research Center
University of British Columbia

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

On Mon, 31 Aug 2009 17:01:03 -0700 (PDT)
Vincent Li <macli@brc.ubc.ca> wrote:

> On Tue, 1 Sep 2009, Minchan Kim wrote:
> 
> > On Mon, 31 Aug 2009 15:54:01 -0700
> > Vincent Li <macli@brc.ubc.ca> wrote:
> > 
> > > commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> > > sc->isolate_pages() return value properly. Add similar proper return value check for
> > > shrink_active_list() sc->isolate_pages().
> > > 
> > > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > 
> > You should have write down your patch's effect clearly
> > in changelog although it's easy. ;-)
> 
> Yes, I should have. This patch is inspired by Kosaki's patch, I 
> thought mentioning that commit would make this patch as follow-up work and 
> changelog clear enough. Would following changelog ok?
> 
> ----
> Add proper return value check for shrink_active_list() 
> sc->isolate_pages(). 
> 
> When "nr_taken == 0"
> 	1: nr_scan related statistics should still be caculated.
> 	2: jump to the end of function and release zone->lru_lock.

It looks good than old. 
In fact, What I wanted is your patch impact.

For example, 
----
If we can't isolate pages from LRU list, 
we don't have to account page movement, either.
Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.

This patch removes unnecessary overhead of page accouting 
and locking in shrink_active_list as follow-up work of commit 5343daceec.
---



> ----
> Regards,
> 
> Vincent Li
> Biomedical Research Center
> University of British Columbia


-- 
Kind regards,
Minchan Kim

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[KOSAKI Motohiro]

> On Mon, 31 Aug 2009 17:01:03 -0700 (PDT)
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > On Tue, 1 Sep 2009, Minchan Kim wrote:
> > 
> > > On Mon, 31 Aug 2009 15:54:01 -0700
> > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > 
> > > > commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> > > > sc->isolate_pages() return value properly. Add similar proper return value check for
> > > > shrink_active_list() sc->isolate_pages().
> > > > 
> > > > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > > 
> > > You should have write down your patch's effect clearly
> > > in changelog although it's easy. ;-)
> > 
> > Yes, I should have. This patch is inspired by Kosaki's patch, I 
> > thought mentioning that commit would make this patch as follow-up work and 
> > changelog clear enough. Would following changelog ok?
> > 
> > ----
> > Add proper return value check for shrink_active_list() 
> > sc->isolate_pages(). 
> > 
> > When "nr_taken == 0"
> > 	1: nr_scan related statistics should still be caculated.
> > 	2: jump to the end of function and release zone->lru_lock.
> 
> It looks good than old. 
> In fact, What I wanted is your patch impact.
> 
> For example, 
> ----
> If we can't isolate pages from LRU list, 
> we don't have to account page movement, either.
> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> 
> This patch removes unnecessary overhead of page accouting 
> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> ---

Vincent, can you please resubmit the patch with new description?
Plus, You can add my reviewed-by sign too.





--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Wu Fengguang]

On Tue, Sep 01, 2009 at 06:54:01AM +0800, Vincent Li wrote:
> commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> sc->isolate_pages() return value properly. Add similar proper return value check for
> shrink_active_list() sc->isolate_pages().
> 
> Signed-off-by: Vincent Li <macli@brc.ubc.ca>

Looks good to me, thanks.

Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>

> ---
>  mm/vmscan.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 460a6f7..2d1c846 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	if (scanning_global_lru(sc)) {
>  		zone->pages_scanned += pgscanned;
>  	}
> -	reclaim_stat->recent_scanned[file] += nr_taken;
> -
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> +
> +	if (nr_taken == 0)
> +		goto done;
> +
> +	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)
>  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
>  	else
> @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
>  	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
>  	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> +
> +done:
>  	spin_unlock_irq(&zone->lru_lock);
>  }
>  
> -- 
> 1.6.0.4

--
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>

Re: [PATCH] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Tue, 1 Sep 2009, KOSAKI Motohiro wrote:

> > On Mon, 31 Aug 2009 17:01:03 -0700 (PDT)
> > Vincent Li <macli@brc.ubc.ca> wrote:
> > 
> > > On Tue, 1 Sep 2009, Minchan Kim wrote:
> > > 
> > > > On Mon, 31 Aug 2009 15:54:01 -0700
> > > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > > 
> > > > > commit 5343daceec (If sc->isolate_pages() return 0...) make shrink_inactive_list handle
> > > > > sc->isolate_pages() return value properly. Add similar proper return value check for
> > > > > shrink_active_list() sc->isolate_pages().
> > > > > 
> > > > > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > > > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > > > 
> > > > You should have write down your patch's effect clearly
> > > > in changelog although it's easy. ;-)
> > > 
> > > Yes, I should have. This patch is inspired by Kosaki's patch, I 
> > > thought mentioning that commit would make this patch as follow-up work and 
> > > changelog clear enough. Would following changelog ok?
> > > 
> > > ----
> > > Add proper return value check for shrink_active_list() 
> > > sc->isolate_pages(). 
> > > 
> > > When "nr_taken == 0"
> > > 	1: nr_scan related statistics should still be caculated.
> > > 	2: jump to the end of function and release zone->lru_lock.
> > 
> > It looks good than old. 
> > In fact, What I wanted is your patch impact.
> > 
> > For example, 
> > ----
> > If we can't isolate pages from LRU list, 
> > we don't have to account page movement, either.
> > Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > 
> > This patch removes unnecessary overhead of page accouting 
> > and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > ---
> 
> Vincent, can you please resubmit the patch with new description?
> Plus, You can add my reviewed-by sign too.

Ok, I will resubmit the patch with Kim's description and add reviewed-by 
sign. Thanks everyone for reviewing!

Vincent Li
Biomedical Research Center
University of British Columbia

--
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>