On 04/23/2014 04:58 AM, Joonsoo Kim wrote:
> On Tue, Apr 22, 2014 at 03:17:30PM +0200, Vlastimil Babka wrote:
>> On 04/22/2014 08:52 AM, Minchan Kim wrote:
>>> On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
>>>> 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@xxxxxxxxxx> 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...)
>>>
>>> My preference is to add following with pair of compact_finished
>>>
>>> static inline void finish_compact(struct compact_control *cc)
>>> {
>>> cc->free_pfn = cc->migrate_pfn;
>>> }
>>
>> Yes setting free_pfn to migrate_pfn is probably the best way, as these
>> are the values compared in compact_finished. But I wouldn't introduce a
>> new function just for one instance of this. Also compact_finished()
>> doesn't test just the scanners to decide whether compaction should
>> continue, so the pairing would be imperfect anyway.
>> So Andrew, if you agree can you please fold in the patch below.
>>
>>> But I don't care.
>>> If you didn't send this patch as clean up, I would never interrupt
>>> on the way but you said it's cleanup patch and the one made me spend a
>>> few minutes to understand the code so it's not a clean up patch. ;-).
>>> So, IMO, it's worth to tidy it up.
>>
>> Yes, I understand and agree.
>>
>> ------8<------
>> From: Vlastimil Babka <vbabka@xxxxxxx>
>> Date: Tue, 22 Apr 2014 13:55:36 +0200
>> Subject: mm-compaction-cleanup-isolate_freepages-fix2
>>
>> Cleanup detection of compaction scanners crossing in isolate_freepages().
>> To make sure compact_finished() observes scanners crossing, we can just set
>> free_pfn to migrate_pfn instead of confusing max() construct.
>>
>> Suggested-by: Minchan Kim <minchan@xxxxxxxxxx>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>> Cc: Christoph Lameter <cl@xxxxxxxxx>
>> Cc: Dongjun Shin <d.j.shin@xxxxxxxxxxx>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxx>
>> Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
>> Cc: Minchan Kim <minchan@xxxxxxxxxx>
>> Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
>> Cc: Rik van Riel <riel@xxxxxxxxxx>
>> Cc: Sunghwan Yun <sunghwan.yun@xxxxxxxxxxx>
>> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
>>
>> ---
>> mm/compaction.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 37c15fe..1c992dc 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -768,7 +768,7 @@ static void isolate_freepages(struct zone *zone,
>> * so that compact_finished() may detect this
>> */
>> if (pfn < low_pfn)
>> - next_free_pfn = max(pfn, zone->zone_start_pfn);
>> + next_free_pfn = cc->migrate_pfn;
>>
>> cc->free_pfn = next_free_pfn;
>> cc->nr_freepages = nr_freepages;
>> --
>> 1.8.4.5
>>
>
> 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.
> unsigned long isolated;
> - unsigned long end_pfn;
>
> /*
> * This can iterate a massively long zone without finding any
> @@ -738,13 +738,6 @@ 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);
> nr_freepages += isolated;
> @@ -754,9 +747,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 = pfn;
> }
> }
>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>