Re: [PATCH -mm v2] mm: have order > 0 compaction start off whereit left

[Minchan Kim]

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/