Re: [PATCH] mm: vmscam: check page order in isolating lru pages

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Tue, 3 Jan 2012 16:05:05 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sat, 31 Dec 2011 22:55:22 +0800
> Hillf Danton <dhillf@gmail.com> wrote:
> 
> > From: Hillf Danton <dhillf@gmail.com>
> > Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
> > 
> > Before try to isolate physically contiguous pages, check for page order is
> > added, and if it is not regular page, we should give up the attempt.
> 
> Well..  why?  Neither the changelog nor the code comments explain why
> we skip these pages.  They should!
> 
> > --- a/mm/vmscan.c	Thu Dec 29 20:20:16 2011
> > +++ b/mm/vmscan.c	Sat Dec 31 22:44:16 2011
> > @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
> >  		unsigned long end_pfn;
> >  		unsigned long page_pfn;
> >  		int zone_id;
> > +		unsigned int isolated_pages = 1;
> > 
> >  		page = lru_to_page(src);
> >  		prefetchw_prev_lru_page(page, src, flags);
> > @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
> >  		case 0:
> >  			mem_cgroup_lru_del(page);
> >  			list_move(&page->lru, dst);
> > -			nr_taken += hpage_nr_pages(page);
> > +			isolated_pages = hpage_nr_pages(page);
> >  			break;
> > 
> >  		case -EBUSY:
> > @@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
> >  			BUG();
> >  		}
> > 
> > +		nr_taken += isolated_pages;
> >  		if (!order)
> >  			continue;
> > +		/* try pfn-based isolation only for regular page */
> > +		if (isolated_pages != 1)
> > +			continue;
> > 
> >  		/*
> >  		 * Attempt to take all pages in the order aligned region
> > @@ -1227,7 +1232,6 @@ static unsigned long isolate_lru_pages(u
> >  				break;
> > 
> >  			if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> > -				unsigned int isolated_pages;
> > 
> >  				mem_cgroup_lru_del(cursor_page);
> >  				list_move(&cursor_page->lru, dst);
> 
> The code has become rather awkward.
> 
> I don't like the trick of reusing a local (isolated_pages) for other
> purposes later on in the function.  This introduces risk that someone
> will add a usage of the local for its original application after it has
> been reused.  And it's a little bit deceiving for readers - they first
> have to work out "oh, it's being reused for something else".  It would
> be better to use two identifiers.  The compiler is good at reusing
> registers (and sometimes stack slots) if the earlier local has gone
> dead.
> 
> Also, why do we test hpage_nr_pages() here?  Why not directly test
> PageTransHuge()?
> 
> 
> iow, something like this?
> 
> --- a/mm/vmscan.c~mm-vmscam-check-page-order-in-isolating-lru-pages-fix
> +++ a/mm/vmscan.c
> @@ -1173,7 +1173,6 @@ static unsigned long isolate_lru_pages(u
>  		unsigned long end_pfn;
>  		unsigned long page_pfn;
>  		int zone_id;
> -		unsigned int isolated_pages = 1;
>  
>  		page = lru_to_page(src);
>  		prefetchw_prev_lru_page(page, src, flags);
> @@ -1184,7 +1183,7 @@ static unsigned long isolate_lru_pages(u
>  		case 0:
>  			mem_cgroup_lru_del(page);
>  			list_move(&page->lru, dst);
> -			isolated_pages = hpage_nr_pages(page);
> +			nr_taken += hpage_nr_pages(page);
>  			break;
>  
>  		case -EBUSY:
> @@ -1196,11 +1195,11 @@ static unsigned long isolate_lru_pages(u
>  			BUG();
>  		}
>  
> -		nr_taken += isolated_pages;
>  		if (!order)
>  			continue;
> -		/* try pfn-based isolation only for regular page */
> -		if (isolated_pages != 1)
> +
> +		/* Try pfn-based isolation only for regular pages */
> +		if (PageTransHuge(page) != 1)
>  			continue;
>  
>  		/*
> @@ -1243,6 +1242,7 @@ static unsigned long isolate_lru_pages(u
>  				break;
>  
>  			if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> +				unsigned int isolated_pages;
>  
>  				mem_cgroup_del_lru(cursor_page);
>  				list_move(&cursor_page->lru, dst);
> 
> 
> If hpage_nr_pages() is the official way of testing for a thp page
> then I guess this is the wrong thing to do!
> 

I think MAX_ORDER can be greater than THP size. So,


if (hpage_nr_pages(page) >= (1 << order))
	continue;

will be good. we can remove  !order check by this.

Thanks,
-Kame




--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>