Date:	Thu, 06 Mar 2014 09:41:13 -0800
From:	Laura Abbott <lauraa@...eaurora.org>
To:	Vlastimil Babka <vbabka@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mel Gorman <mgorman@...e.de>
CC:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] mm/compaction: Break out of loop on !PageBuddy in isolate_freepages_block

On 3/6/2014 2:22 AM, Vlastimil Babka wrote:
> On 03/06/2014 03:26 AM, Laura Abbott wrote:
>> We received several reports of bad page state when freeing CMA pages
>> previously allocated with alloc_contig_range:
>>
>> <1>[ 1258.084111] BUG: Bad page state in process Binder_A  pfn:63202
>> <1>[ 1258.089763] page:d21130b0 count:0 mapcount:1 mapping:  (null)
>> index:0x7dfbf
>> <1>[ 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 we end up over isolating by more than
>> one page (e.g. last since page needed is a higher order page), it
>> is not possible to detect that the page was skipped. The fix is to
>
> I found it hard to grasp this sentence at first. Perhaps something like
> "if in the last iteration of the loop we isolate more than one page
> (e.g. ...), the check for total_isolated may pass and we fail to detect
> that a page was skipped" would be better?
>

Yes, that sounds much better.

>> 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.
>
> That looks sound , but I wonder if it makes sense to keep the
> nr_strict_required stuff after this change. The check could then simply
> use 'if (pfn < end_pfn)' the same way as isolate_freepages_range does,
> right?
>

I had that thought as well. I'll fix that up for v2 along with the rest 
of your comments.

>> Signed-off-by: Laura Abbott <lauraa@...eaurora.org>
>> ---
>>   mm/compaction.c |   25 +++++++++++++++++++------
>>   1 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index b48c525..3190cef 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -263,12 +263,21 @@ static unsigned long
>> isolate_freepages_block(struct compact_control *cc,
>>           struct page *page = cursor;
>>
>>           nr_scanned++;
>> -        if (!pfn_valid_within(blockpfn))
>> -            continue;
>> +        if (!pfn_valid_within(blockpfn)) {
>> +            if (strict)
>> +                break;
>> +            else
>> +                continue;
>> +        }
>> +
>>           if (!valid_page)
>>               valid_page = page;
>> -        if (!PageBuddy(page))
>> -            continue;
>> +        if (!PageBuddy(page)) {
>> +            if (strict)
>> +                break;
>> +            else
>> +                continue;
>> +        }
>>
>>           /*
>>            * The zone lock must be held to isolate freepages.
>> @@ -288,8 +297,12 @@ static unsigned long
>> isolate_freepages_block(struct compact_control *cc,
>>               break;
>>
>>           /* Recheck this is a buddy page under lock */
>> -        if (!PageBuddy(page))
>> -            continue;
>> +        if (!PageBuddy(page)) {
>> +            if (strict)
>> +                break;
>> +            else
>> +                continue;
>> +        }
>
> To avoid this triple if-else occurence, you could instead do a "goto
> isolate_failed;" and put the if-else under said label at the end of the
> loop, also allowing extra cleanup, something like this:
>
> @@ -298,8 +298,6 @@ static unsigned long isolate_freepages_block(struct
> compact_control *cc,
>
>                  /* Found a free page, break it into order-0 pages */
>                  isolated = split_free_page(page);
> -               if (!isolated && strict)
> -                       break;
>                  total_isolated += isolated;
>                  for (i = 0; i < isolated; i++) {
>                          list_add(&page->lru, freelist);
> @@ -310,7 +308,13 @@ static unsigned long isolate_freepages_block(struct
> compact_control *cc,
>                  if (isolated) {
>                          blockpfn += isolated - 1;
>                          cursor += isolated - 1;
> +                       continue;
>                  }
> +isolate_fail:
> +               if (strict)
> +                       break;
> +               else
> +                       continue;
>
>
> Thanks,
> Vlastimil
>
>>           /* Found a free page, break it into order-0 pages */
>>           isolated = split_free_page(page);
>>
>

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists