On Mon, Mar 10 2014, [email protected] wrote: > From: Laura Abbott <[email protected]> > Subject: mm/compaction: break out of loop on !PageBuddy in > isolate_freepages_block > > We received several reports of bad page state when freeing CMA pages > previously allocated with alloc_contig_range: > > [ 1258.084111] BUG: Bad page state in process Binder_A pfn:63202 > [ 1258.089763] page:d21130b0 count:0 mapcount:1 mapping: (null) index:0x7dfbf > [ 1258.096109] page flags: 0x40080068(uptodate|lru|active|swapbacked) > > Based on the page state, it looks like the page was still in use. The > page flags do not make sense for the use case though. Further debugging > showed that despite alloc_contig_range returning success, at least one > page in the range still remained in the buddy allocator. > > There is an issue with isolate_freepages_block. In strict mode (which CMA > uses), if any pages in the range cannot be isolated, > isolate_freepages_block should return failure 0. The current check keeps > track of the total number of isolated pages and compares against the size > of the range: > > if (strict && nr_strict_required > total_isolated) > total_isolated = 0; > > After taking the zone lock, if one of the pages in the range is not in the > buddy allocator, we continue through the loop and do not increment > total_isolated. If in the last iteration of the loop we isolate more than > one page (e.g. last page needed is a higher order page), the check for > total_isolated may pass and we fail to detect that a page was skipped. > The fix is to bail out if the loop immediately if we are in strict mode. > There's no benfit to continuing anyway since we need all pages to be > isolated. Additionally, drop the error checking based on > nr_strict_required and just check the pfn ranges. This matches with what > isolate_freepages_range does. > > 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]> Also, a few comments inline: > Cc: <[email protected]> > Signed-off-by: Andrew Morton <[email protected]> > --- > > mm/compaction.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff -puN > mm/compaction.c~mm-compaction-break-out-of-loop-on-pagebuddy-in-isolate_freepages_block > mm/compaction.c > --- > a/mm/compaction.c~mm-compaction-break-out-of-loop-on-pagebuddy-in-isolate_freepages_block > +++ a/mm/compaction.c > @@ -251,7 +251,6 @@ static unsigned long isolate_freepages_b > { > int nr_scanned = 0, total_isolated = 0; > struct page *cursor, *valid_page = NULL; > - unsigned long nr_strict_required = end_pfn - blockpfn; > unsigned long flags; > bool locked = false; > > @@ -264,11 +263,12 @@ static unsigned long isolate_freepages_b > > nr_scanned++; > if (!pfn_valid_within(blockpfn)) > - continue; > + goto isolate_fail; > + > if (!valid_page) > valid_page = page; > if (!PageBuddy(page)) > - continue; > + goto isolate_fail; > > /* > * The zone lock must be held to isolate freepages. > @@ -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. > } > > trace_mm_compaction_isolate_freepages(nr_scanned, total_isolated); > @@ -315,7 +321,7 @@ static unsigned long isolate_freepages_b > * pages requested were isolated. If there were any failures, 0 is > * returned and CMA will fail. > */ > - if (strict && nr_strict_required > total_isolated) > + if (strict && blockpfn < end_pfn) > total_isolated = 0; > > if (locked) > _ -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
signature.asc
Description: PGP signature
