From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx193.postini.com [74.125.245.193]) by kanga.kvack.org (Postfix) with SMTP id 33FA96B13F0 for ; Wed, 1 Feb 2012 16:17:39 -0500 (EST) Message-ID: <4F29ABD6.70704@redhat.com> Date: Wed, 01 Feb 2012 16:17:10 -0500 From: Rik van Riel MIME-Version: 1.0 Subject: Re: [patch] mm: compaction: make compact_control order signed References: <20120201144101.GA5397@elgon.mountain> <20120201124651.9203acde.akpm@linux-foundation.org> In-Reply-To: <20120201124651.9203acde.akpm@linux-foundation.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Dan Carpenter , Mel Gorman , Minchan Kim , Andrea Arcangeli , linux-mm@kvack.org, kernel-janitors@vger.kernel.org On 02/01/2012 03:46 PM, Andrew Morton wrote: > On Wed, 1 Feb 2012 17:41:01 +0300 > Dan Carpenter wrote: > >> "order" is -1 when compacting via /proc/sys/vm/compact_memory. Making >> it unsigned causes a bug in __compact_pgdat() when we test: >> >> if (cc->order< 0 || !compaction_deferred(zone, cc->order)) >> compact_zone(zone, cc); >> >> Signed-off-by: Dan Carpenter >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 382831e..5f80a11 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -35,7 +35,7 @@ struct compact_control { >> unsigned long migrate_pfn; /* isolate_migratepages search base */ >> bool sync; /* Synchronous migration */ >> >> - unsigned int order; /* order a direct compactor needs */ >> + int order; /* order a direct compactor needs */ >> int migratetype; /* MOVABLE, RECLAIMABLE etc */ >> struct zone *zone; >> }; > > One would expect this to significantly change the behaviour of > /proc/sys/vm/compact_memory. Enfeebled minds want to know: is > the new behaviour better or worse than the old behaviour? The old behaviour and the behaviour post Dan's fix are the same. My patch temporarily broke things, by testing for order < 0, instead of the explicit cc->order == -1 used elsewhere in the code. I did not notice it in my own testing because I tested on 3.2.0 and sent you patches against 3.3-current. It looks like this line of code is the one difference between both trees I was working on :( In my test tree, I had (cc->sync || !compaction_deferred(zone, cc->order)). Arguably, testing for cc->order == -1 (or cc->order < 0) is better anyway. -- All rights reversed -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org