Message-ID: <53560D3F.2030002@suse.cz>
Date:	Tue, 22 Apr 2014 08:33:35 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Minchan Kim <minchan@...nel.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Mel Gorman <mgorman@...e.de>,
	Heesub Shin <heesub.shin@...sung.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Dongjun Shin <d.j.shin@...sung.com>,
	Sunghwan Yun <sunghwan.yun@...sung.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Michal Nazarewicz <mina86@...a86.com>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Christoph Lameter <cl@...ux.com>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

On 22.4.2014 1:53, Minchan Kim wrote:
> On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
>> On 21.4.2014 21:41, Andrew Morton wrote:
>>> On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@...nel.org> wrote:
>>>
>>>> Hi Vlastimil,
>>>>
>>>> Below just nitpicks.
>>> It seems you were ignored ;)
>> Oops, I managed to miss your e-mail, sorry.
>>
>>>>>   {
>>>>>   	struct page *page;
>>>>> -	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
>>>>> +	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>>> Could you add comment for each variable?
>>>>
>>>> unsigned long pfn; /* scanning cursor */
>>>> unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
>>>> unsigned long next_free_pfn; /* start pfn for scaning at next truen */
>>>> unsigned long z_end_pfn; /* zone's end pfn */
>>>>
>>>>
>>>>> @@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>>>   	/*
>>>>> -	 * Take care that if the migration scanner is at the end of the zone
>>>>> -	 * that the free scanner does not accidentally move to the next zone
>>>>> -	 * in the next isolation cycle.
>>>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>>>> +	 * none, the pfn < low_pfn check will kick in.
>>>>         "none" what? I'd like to clear more.
>> If there are no updates to next_free_pfn within the for cycle. Which
>> matches Andrew's formulation below.
>>
>>> I did this:
>> Thanks!
>>
>>> --- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
>>> +++ a/mm/compaction.c
>>> @@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
>>>   				struct compact_control *cc)
>>>   {
>>>   	struct page *page;
>>> -	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
>>> +	unsigned long pfn;	     /* scanning cursor */
>>> +	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 */
>> Yes that works.
>>
>>>   	int nr_freepages = cc->nr_freepages;
>>>   	struct list_head *freelist = &cc->freepages;
>>> @@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
>>>   	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>   	/*
>>> -	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
>>> -	 * none, the pfn < low_pfn check will kick in.
>>> +	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> +	 * isolated, the pfn < low_pfn check will kick in.
>> OK.
>>
>>>   	 */
>>>   	next_free_pfn = 0;
>>>>> @@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
>>>>>   	 * so that compact_finished() may detect this
>>>>>   	 */
>>>>>   	if (pfn < low_pfn)
>>>>> -		cc->free_pfn = max(pfn, zone->zone_start_pfn);
>>>>> -	else
>>>>> -		cc->free_pfn = high_pfn;
>>>>> +		next_free_pfn = max(pfn, zone->zone_start_pfn);
>>>> Why we need max operation?
>>>> IOW, what's the problem if we do (next_free_pfn = pfn)?
>>> An answer to this would be useful, thanks.
>> The idea (originally, not new here) is that the free scanner wants
>> to remember the highest-pfn
>> block where it managed to isolate some pages. If the following page
>> migration fails, these isolated
>> pages might be put back and would be skipped in further compaction
>> attempt if we used just
>> "next_free_pfn = pfn", until the scanners get reset.
>>
>> The question of course is if such situations are frequent and makes
>> any difference to compaction
>> outcome. And the downsides are potentially useless rescans and code
>> complexity. Maybe Mel
>> remembers how important this is? It should probably be profiled
>> before changes are made.
> I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> At that time, I didn't Cced so I missed that code so let's ask this time.
> In that patch, you added this.
>
> if (pfn < low_pfn)
>    cc->free_pfn = max(pfn, zone->zone_start_pfn);
> else
>    cc->free_pfn = high_pfn;

Oh, right, this max(), not the one in the for loop. Sorry, I should have 
read more closely.
But still maybe it's a good opportunity to kill the other max() as well. 
I'll try some testing.

Anyway, this is what I answered to Mel when he asked the same thing when 
I sent
that 7ed695069c3c patch:

If a zone starts in a middle of a pageblock and migrate scanner isolates
enough pages early to stay within that pageblock, low_pfn will be at the
end of that pageblock and after the for cycle in this function ends, pfn
might be at the beginning of that pageblock. It might not be an actual
problem (this compaction will finish at this point, and if someone else
is racing, he will probably check the boundaries himself), but I played
it safe.


> So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> compact_finished to stop compaction. And your [1/2] patch in this patchset
> always makes free page scanner start on pageblock boundary so when the
> loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> would be lower than migration scanner so compact_finished will always detect
> it so I think you could just do
>
> if (pfn < low_pfn)
>    next_free_pfn = pfn;
>
> cc->free_pfn = next_free_pfn;

That could work. I was probably wrong about danger of racing in the 
reply to Mel,
because free_pfn is stored in cc (private), not zone (shared).

>
> Or, if you want to clear *reset*,
> if (pfn < lown_pfn)
>    next_free_pfn = zone->zone_start_pfn;
>
> cc->free_pfn = next_free_pfn;

That would work as well but is less straightforward I think. Might be 
misleading if
someone added tracepoints to track the free scanner progress with pfn's 
(which
might happen soon...)

> That's why I asked about max operation. What am I missing?
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@...ck.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists