On 07/04/2012 04:42 PM, Andrew Morton wrote:
> On Wed, 04 Jul 2012 11:34:09 +0900 Minchan Kim <minchan@xxxxxxxxxx> wrote:
>
>>> The rest of this patch takes care to ensure that
>>> ->compact_cached_free_pfn is aligned to pageblock_nr_pages. But it now
>>> appears that this particular site will violate that.
>>>
>>> What's up? Do we need to fix this site, or do we remove all that
>>> make-compact_cached_free_pfn-aligned code?
>>
>>
>> I vote removing the warning because it doesn't related to Rik's incremental compaction.
>> Let's see.
>>
>> high_pfn = min(low_pfn, pfn) = cc->migrate_pfn + pageblock_nr_pages.
>> In here, cc->migrate_pfn isn't necessarily pageblock aligined.
>> So if we don't consider compact_cached_free_pfn, it can hit.
>>
>> static void isolate_freepages()
>> {
>> high_pfn = min(low_pfn, pfn) = cc->migrate_pfn + pageblock_nr_pages;
>> for (..) {
>> ...
>> WARN_ON_ONCE(high_pfn & (pageblock_nr_pages - 1));
>>
>> }
>> }
>
> Please, look at the patch. In numerous places it is aligning
> compact_cached_free_pfn to a multiple of pageblock_nr_pages. But in
> one place it doesn't do that. So are all those alignment operations
> necessary?
I mean if you *really* want to check the align, you should do following as
barrios@bbox:~/linux-memcg$ git diff
diff --git a/mm/compaction.c b/mm/compaction.c
index 6bb3e9f..12416d4 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -467,16 +467,18 @@ static void isolate_freepages(struct zone *zone,
}
spin_unlock_irqrestore(&zone->lock, flags);
- WARN_ON_ONCE(high_pfn & (pageblock_nr_pages - 1));
/*
* Record the highest PFN we isolated pages from. When next
* looking for free pages, the search will restart here as
* page migration may have returned some pages to the allocator
*/
- if (isolated)
+ if (isolated) {
high_pfn = max(high_pfn, pfn);
- if (cc->order > 0)
- zone->compact_cached_free_pfn = high_pfn;
+ if (cc->order > 0) {
+ WARN_ON_ONCE(high_pfn & (pageblock_nr_pages - 1));
+ zone->compact_cached_free_pfn = high_pfn;
+ }
+ }
}
/* split_free_page does not map the pages */
Because high_pfn could be not aligned in loop if it doesn't reset by max(high_pfn, pfn).
and it's legal. So regardless of Rik's patch, if you add such warning in that code,
it could emit WARNING, too. Rik already sent a patch which was similar to above
but he wanted to solve WARN_ON_ONCE problem by someone else.
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>