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

KOSAKI Motohiro
2009-05-19T01:11:39Z

> 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) {
/*