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

[Joonsoo Kim]

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