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. :)
Thanks.
--------->8----------------
>From ae653cf8b9f8c7423cad73af38cde94eec564b50 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
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@xxxxxxx>
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;
--
1.7.9.5
--
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>