On Mon, Mar 10 2014, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Laura Abbott <lauraa@xxxxxxxxxxxxxx>
> 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 <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>
Also, a few comments inline:
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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 +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature