Re: [PATCH 04/15] mm: compaction: introduce isolate_freepages_range()

[Mel Gorman]

On Thu, Jan 26, 2012 at 10:00:46AM +0100, Marek Szyprowski wrote:
> From: Michal Nazarewicz <mina86@xxxxxxxxxx>
> 
> This commit introduces isolate_freepages_range() function which
> generalises isolate_freepages_block() so that it can be used on
> arbitrary PFN ranges.
> 
> isolate_freepages_block() is left with only minor changes.
> 

The minor changes to isolate_freepages_block() look fine in
terms of how current compaction works. I have a minor comment on
isolate_freepages_range() but it is up to you whether to address them
or not. Whether you alter isolate_freepages_range() or not;

Acked-by: Mel Gorman <mel@xxxxxxxxx>

> <SNIP>
> @@ -105,6 +109,80 @@ static unsigned long isolate_freepages_block(struct zone *zone,
>  	return total_isolated;
>  }
>  
> +/**
> + * isolate_freepages_range() - isolate free pages.
> + * @start_pfn: The first PFN to start isolating.
> + * @end_pfn:   The one-past-last PFN.
> + *
> + * Non-free pages, invalid PFNs, or zone boundaries within the
> + * [start_pfn, end_pfn) range are considered errors, cause function to
> + * undo its actions and return zero.
> + *
> + * Otherwise, function returns one-past-the-last PFN of isolated page
> + * (which may be greater then end_pfn if end fell in a middle of
> + * a free page).
> + */
> +static unsigned long
> +isolate_freepages_range(unsigned long start_pfn, unsigned long end_pfn)
> +{
> +	unsigned long isolated, pfn, block_end_pfn, flags;
> +	struct zone *zone = NULL;
> +	LIST_HEAD(freelist);
> +	struct page *page;
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn += isolated) {
> +		if (!pfn_valid(pfn))
> +			break;
> +
> +		if (!zone)
> +			zone = page_zone(pfn_to_page(pfn));
> +		else if (zone != page_zone(pfn_to_page(pfn)))
> +			break;
> +

So what you are checking for here is if you straddle zones.
You could just initialise zone outside of the for loop. You can
then check outside the loop if end_pfn is in a different zone to
start_pfn. If it is, either adjust end_pfn accordingly or bail the
entire operation avoiding the need for release_freepages() later. This
will be a little cheaper.

> +		/*
> +		 * On subsequent iterations round_down() is actually not
> +		 * needed, but we keep it that we not to complicate the code.
> +		 */
> +		block_end_pfn = round_down(pfn, pageblock_nr_pages)
> +			+ pageblock_nr_pages;

Seems a little more involved than it needs to be. Something like
this might suit and be a bit nicer?

block_end_pfn = ALIGN(pfn+1, pageblock_nr_pages);

> +		block_end_pfn = min(block_end_pfn, end_pfn);
> +
> +		spin_lock_irqsave(&zone->lock, flags);
> +		isolated = isolate_freepages_block(pfn, block_end_pfn,
> +						   &freelist, true);
> +		spin_unlock_irqrestore(&zone->lock, flags);
> +
> +		/*
> +		 * In strict mode, isolate_freepages_block() returns 0 if
> +		 * there are any holes in the block (ie. invalid PFNs or
> +		 * non-free pages).
> +		 */
> +		if (!isolated)
> +			break;
> +
> +		/*
> +		 * If we managed to isolate pages, it is always (1 << n) *
> +		 * pageblock_nr_pages for some non-negative n.  (Max order
> +		 * page may span two pageblocks).
> +		 */
> +	}
> +
> +	/* split_free_page does not map the pages */
> +	list_for_each_entry(page, &freelist, lru) {
> +		arch_alloc_page(page, 0);
> +		kernel_map_pages(page, 1, 1);
> +	}
> +

This block is copied in two places - isolate_freepages and
isolate_freepages_range() so sharing a common helper would be nice. I
suspect you didn't because it would interfere with existing code more
than was strictly necessary which I complained about previously as
it made review harder. If that was your thinking, then just create
this helper in a separate patch. It's not critical though.

> +	if (pfn < end_pfn) {
> +		/* Loop terminated early, cleanup. */
> +		release_freepages(&freelist);
> +		return 0;
> +	}
> +
> +	/* We don't use freelists for anything. */
> +	return pfn;
> +}
> +
>  /* Returns true if the page is within a block suitable for migration to */
>  static bool suitable_migration_target(struct page *page)
>  {

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