On Tue, 11 Mar 2014 00:28:16 +0100 Michal Nazarewicz <[email protected]> wrote:
> On Mon, Mar 10 2014, [email protected] wrote: > > > > Signed-off-by: Laura Abbott <[email protected]> > > Acked-by: Minchan Kim <[email protected]> > > Cc: Mel Gorman <[email protected]> > > Acked-by: Vlastimil Babka <[email protected]> > > Cc: Joonsoo Kim <[email protected]> > > Acked-by: Bartlomiej Zolnierkiewicz <[email protected]> > > Cc: Michal Nazarewicz <[email protected]> > > Not sure if it's not too late, but: > > Acked-by: Michal Nazarewicz <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
