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

[Mel Gorman]

On Sat, Dec 31, 2011 at 10:55:22PM +0800, Hillf Danton wrote:
> From: Hillf Danton <dhillf@xxxxxxxxx>
> 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.
> 
> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
> Acked-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxx>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> ---
> 
> --- 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;
> 

Please put more detail in your changelogs explaining the intention
of your patch.  Judging from it, this is a marginal performance
improvement when THPs are being isolated from the LRU by bypassing
lumpy reclaim.

However, basing the check on "isolated_pages" is obscure and it also
disables lumpy reclaim for the cases where order > HPAGE_SHIFT . This
is very rare (might never even happen) but it's still broken. Minimally
the check should have been something like

if (!order || isolated_pages >= (1 << order))
	continue;

with a comment explaining that there is no point taking pages around
a naturally-aligned region if we just isolated a page larger than it.
This would look better, avoid reusing isolated_pages, be less obscure
and still work for cases where the requested order is larger than
a THP.

Nak to this version.

-- 
Mel Gorman
SUSE Labs
--
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/