Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Vlastimil Babka <vbabka@suse.cz>]

On 04/25/2014 10:29 AM, Joonsoo Kim wrote:
> On Wed, Apr 23, 2014 at 04:31:14PM +0200, Vlastimil Babka wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> How about doing more clean-up at this time?
>>>>>
>>>>> What I did is that taking end_pfn out of the loop and consider zone
>>>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>>>> Another things I did are removing max() operation and un-needed
>>>>> assignment to isolate variable.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --------->8------------
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index 1c992dc..95a506d 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>                                struct compact_control *cc)
>>>>>   {
>>>>>        struct page *page;
>>>>> -     unsigned long pfn;           /* scanning cursor */
>>>>> +     unsigned long pfn;           /* start of scanning window */
>>>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>>>        unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>>>        unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>>>        int nr_freepages = cc->nr_freepages;
>>>>>        struct list_head *freelist = &cc->freepages;
>>>>>
>>>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * is using.
>>>>>         */
>>>>>        pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>>        /*
>>>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>>>> -      * isolated, the pfn < low_pfn check will kick in.
>>>>> +      * Take care when isolating in last pageblock of a zone which
>>>>> +      * ends in the middle of a pageblock.
>>>>>         */
>>>>> -     next_free_pfn = 0;
>>>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>
>>>>> -     z_end_pfn = zone_end_pfn(zone);
>>>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>>>> +     next_free_pfn = 0;
>>>>>
>>>>>        /*
>>>>>         * Isolate free pages until enough are available to migrate the
>>>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>>>         * and free page scanners meet or enough free pages are isolated.
>>>>>         */
>>>>>        for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>>>> -                                     pfn -= pageblock_nr_pages) {
>>>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>>>
>>>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>>>> always be in the middle of a pageblock and you will not scan half of all
>>>> pageblocks.
>>>>
>>>
>>> Okay. I think a way to fix it.
>>> By assigning pfn(start of scanning window) to
>>> end_pfn(end of scanning window) for the next loop, we can solve the problem
>>> you mentioned. How about below?
>>>
>>> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {
>>
>> Hm that's perhaps a bit subtle but it would work.
>> Maybe better names for pfn and end_pfn would be block_start_pfn and
>> block_end_pfn. And in those comments, s/scanning window/current pageblock/.
>> And please don't move the low_pfn assignment like you did. The comment
>> above the original location explains it, the comment above the new
>> location doesn't. It's use in the loop is also related to 'pfn', not
>> 'end_pfn'.
>
> Okay.
> Following patch solves all your concerns.
> End result looks so nice to me. :)

Great, thanks!

> Thanks.
>
> --------->8----------------
>  From ae653cf8b9f8c7423cad73af38cde94eec564b50 Mon Sep 17 00:00:00 2001
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Date: Fri, 25 Apr 2014 17:12:58 +0900
> Subject: [PATCH] mm-compaction-cleanup-isolate_freepages-fix3
>
> What I did here is taking end_pfn out of the loop and considering zone
> boundary once. After then, we can just set previous pfn to end_pfn on
> every iteration to move scanning window. With this change, we can remove
> local variable, z_end_pfn.
>
> Another things I did are removing max() operation and un-needed
> assignment to isolate variable.
>
> In addition, I change both the variable names, from pfn and
> end_pfn to block_start_pfn and block_end_pfn, respectively.
> They represent their meaning perfectly.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: Vlastimil Babka <vbabka@suse.de>

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1c992dc..ba80bea 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>   				struct compact_control *cc)
>   {
>   	struct page *page;
> -	unsigned long pfn;	     /* scanning cursor */
> +	unsigned long block_start_pfn;	/* start of current pageblock */
> +	unsigned long block_end_pfn;	/* end of current pageblock */
>   	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
>   	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> -	unsigned long z_end_pfn;     /* zone's end pfn */
>   	int nr_freepages = cc->nr_freepages;
>   	struct list_head *freelist = &cc->freepages;
>
> @@ -682,31 +682,33 @@ static void isolate_freepages(struct zone *zone,
>   	 * Initialise the free scanner. The starting point is where we last
>   	 * successfully isolated from, zone-cached value, or the end of the
>   	 * zone when isolating for the first time. We need this aligned to
> -	 * the pageblock boundary, because we do pfn -= pageblock_nr_pages
> -	 * in the for loop.
> +	 * the pageblock boundary, because we do
> +	 * block_start_pfn -= pageblock_nr_pages in the for loop.
> +	 * For ending point, take care when isolating in last pageblock of a
> +	 * a zone which ends in the middle of a pageblock.
>   	 * The low boundary is the end of the pageblock the migration scanner
>   	 * is using.
>   	 */
> -	pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_start_pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
> +	block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
> +						zone_end_pfn(zone));
>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>
>   	/*
> -	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> -	 * isolated, the pfn < low_pfn check will kick in.
> +	 * If no pages are isolated, the block_start_pfn < low_pfn check
> +	 * will kick in.
>   	 */
>   	next_free_pfn = 0;
>
> -	z_end_pfn = zone_end_pfn(zone);
> -
>   	/*
>   	 * Isolate free pages until enough are available to migrate the
>   	 * pages on cc->migratepages. We stop searching if the migrate
>   	 * and free page scanners meet or enough free pages are isolated.
>   	 */
> -	for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> -					pfn -= pageblock_nr_pages) {
> +	for (;block_start_pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
> +				block_end_pfn = block_start_pfn,
> +				block_start_pfn -= pageblock_nr_pages) {
>   		unsigned long isolated;
> -		unsigned long end_pfn;
>
>   		/*
>   		 * This can iterate a massively long zone without finding any
> @@ -715,7 +717,7 @@ static void isolate_freepages(struct zone *zone,
>   		 */
>   		cond_resched();
>
> -		if (!pfn_valid(pfn))
> +		if (!pfn_valid(block_start_pfn))
>   			continue;
>
>   		/*
> @@ -725,7 +727,7 @@ static void isolate_freepages(struct zone *zone,
>   		 * i.e. it's possible that all pages within a zones range of
>   		 * pages do not belong to a single zone.
>   		 */
> -		page = pfn_to_page(pfn);
> +		page = pfn_to_page(block_start_pfn);
>   		if (page_zone(page) != zone)
>   			continue;
>
> @@ -738,15 +740,8 @@ static void isolate_freepages(struct zone *zone,
>   			continue;
>
>   		/* Found a block suitable for isolating free pages from */
> -		isolated = 0;
> -
> -		/*
> -		 * Take care when isolating in last pageblock of a zone which
> -		 * ends in the middle of a pageblock.
> -		 */
> -		end_pfn = min(pfn + pageblock_nr_pages, z_end_pfn);
> -		isolated = isolate_freepages_block(cc, pfn, end_pfn,
> -						   freelist, false);
> +		isolated = isolate_freepages_block(cc, block_start_pfn,
> +					block_end_pfn, freelist, false);
>   		nr_freepages += isolated;
>
>   		/*
> @@ -754,9 +749,9 @@ static void isolate_freepages(struct zone *zone,
>   		 * looking for free pages, the search will restart here as
>   		 * page migration may have returned some pages to the allocator
>   		 */
> -		if (isolated) {
> +		if (isolated && next_free_pfn == 0) {
>   			cc->finished_update_free = true;
> -			next_free_pfn = max(next_free_pfn, pfn);
> +			next_free_pfn = block_start_pfn;
>   		}
>   	}
>
> @@ -767,7 +762,7 @@ static void isolate_freepages(struct zone *zone,
>   	 * If we crossed the migrate scanner, we want to keep it that way
>   	 * so that compact_finished() may detect this
>   	 */
> -	if (pfn < low_pfn)
> +	if (block_start_pfn < low_pfn)
>   		next_free_pfn = cc->migrate_pfn;
>
>   	cc->free_pfn = next_free_pfn;
>