Re: [PATCH 1/4] vmscan: change the number of the unmapped files in zone reclaim

[KOSAKI Motohiro]

> On Mon, May 18, 2009 at 11:35:31AM +0800, KOSAKI Motohiro wrote:
> > >> --- a/mm/vmscan.c
> > >> +++ b/mm/vmscan.c
> > >> @@ -2397,6 +2397,7 @@ static int __zone_reclaim(struct zone *z
> > >> ? ? ? ? ? ? ? .isolate_pages = isolate_pages_global,
> > >> ? ? ? };
> > >> ? ? ? unsigned long slab_reclaimable;
> > >> + ? ? long nr_unmapped_file_pages;
> > >>
> > >> ? ? ? disable_swap_token();
> > >> ? ? ? cond_resched();
> > >> @@ -2409,9 +2410,11 @@ static int __zone_reclaim(struct zone *z
> > >> ? ? ? reclaim_state.reclaimed_slab = 0;
> > >> ? ? ? p->reclaim_state = &reclaim_state;
> > >>
> > >> - ? ? if (zone_page_state(zone, NR_FILE_PAGES) -
> > >> - ? ? ? ? ? ? zone_page_state(zone, NR_FILE_MAPPED) >
> > >> - ? ? ? ? ? ? zone->min_unmapped_pages) {
> > >> + ? ? nr_unmapped_file_pages = zone_page_state(zone, NR_INACTIVE_FILE) +
> > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?zone_page_state(zone, NR_ACTIVE_FILE) -
> > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?zone_page_state(zone, NR_FILE_MAPPED);
> > >
> > > This can possibly go negative.
> > 
> > Is this a problem?
> > negative value mean almost pages are mapped. Thus
> > 
> >   (nr_unmapped_file_pages > zone->min_unmapped_pages)  => 0
> > 
> > is ok, I think.
> 
> I wonder why you didn't get a gcc warning, because zone->min_unmapped_pages
> is a "unsigned long".
> 
> Anyway, add a simple note to the code if it works *implicitly*?

hm, My gcc is wrong version? (gcc version 4.1.2 20070626 (Red Hat 4.1.2-14))
Anyway, you are right. thanks for good catch :)

incremental fixing patch is here.

Patch name: vmscan-change-the-number-of-the-unmapped-files-in-zone-reclaim-fix.patch
Applied after: vmscan-change-the-number-of-the-unmapped-files-in-zone-reclaim.patch
---
 mm/vmscan.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: b/mm/vmscan.c
===================================================================
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2397,7 +2397,9 @@ static int __zone_reclaim(struct zone *z
 		.isolate_pages = isolate_pages_global,
 	};
 	unsigned long slab_reclaimable;
-	long nr_unmapped_file_pages;
+	unsigned long nr_file_pages;
+	unsigned long nr_mapped;
+	unsigned long nr_unmapped_file_pages = 0;
 
 	disable_swap_token();
 	cond_resched();
@@ -2410,9 +2412,11 @@ static int __zone_reclaim(struct zone *z
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
-	nr_unmapped_file_pages = zone_page_state(zone, NR_INACTIVE_FILE) +
-				 zone_page_state(zone, NR_ACTIVE_FILE) -
-				 zone_page_state(zone, NR_FILE_MAPPED);
+	nr_file_pages = zone_page_state(zone, NR_INACTIVE_FILE) +
+			zone_page_state(zone, NR_ACTIVE_FILE);
+	nr_mapped = zone_page_state(zone, NR_FILE_MAPPED);
+	if (likely(nr_file_pages >= nr_mapped))
+		nr_unmapped_file_pages = nr_file_pages - nr_mapped;
 
 	if (nr_unmapped_file_pages > zone->min_unmapped_pages) {
 		/*



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/