Messages in this thread Patch in this message | | | Date | Wed, 04 Jul 2012 17:01:40 +0900 | From | Minchan Kim <> | Subject | Re: [PATCH -mm v2] mm: have order > 0 compaction start off where it left |
| |
On 07/04/2012 04:42 PM, Andrew Morton wrote:
> On Wed, 04 Jul 2012 11:34:09 +0900 Minchan Kim <minchan@kernel.org> 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
| |