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

Joonsoo Kim
2014-04-25T08:28:42Z

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