On Tue, 11 Mar 2014 00:28:16 +0100 Michal Nazarewicz <mina86@xxxxxxxxxx> wrote:
> On Mon, Mar 10 2014, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
> >
> > Signed-off-by: Laura Abbott <lauraa@xxxxxxxxxxxxxx>
> > Acked-by: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: Mel Gorman <mgorman@xxxxxxx>
> > Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
>
> Not sure if it's not too late, but:
>
> Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
Probably to late, unless Linus is feeling frisky ;)
> Also, a few comments inline:
>
> > @@ -289,12 +289,10 @@ static unsigned long isolate_freepages_b
> >
> > /* Recheck this is a buddy page under lock */
> > if (!PageBuddy(page))
> > - continue;
> > + goto isolate_fail;
> >
> > /* Found a free page, break it into order-0 pages */
> > isolated = split_free_page(page);
> > - if (!isolated && strict)
> > - break;
>
> I feel it would be more readable if this became:
>
> if (!isolated)
> goto isolate_fail;
>
> > total_isolated += isolated;
> > for (i = 0; i < isolated; i++) {
> > list_add(&page->lru, freelist);
> > @@ -305,7 +303,15 @@ static unsigned long isolate_freepages_b
> > if (isolated) {
> > blockpfn += isolated - 1;
> > cursor += isolated - 1;
> > + continue;
> > }
>
> And then here, the whole body of the if could be moved outside of the if
> statement.
>
> > +
> > +isolate_fail:
> > + if (strict)
> > + break;
> > + else
> > + continue;
> > +
>
> The else part is a bit redundant. It should be removed in my opinion.
Yes, I felt the same way so I queued up
http://ozlabs.org/~akpm/mmots/broken-out/mm-compactionc-isolate_freepages_block-small-tuneup.patch
which is similar.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html