Re: [patch] mm: compaction: make compact_control order signed

[Rik van Riel <riel@redhat.com>]

On 02/01/2012 04:24 PM, Andrew Morton wrote:
> On Wed, 01 Feb 2012 16:17:10 -0500
> Rik van Riel<riel@redhat.com>  wrote:
>
>>>> @@ -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.
>
> I suppose it would be nicer to make the code in __compact_pgdat() match
> all the other places whcih do this:
>
> --- a/mm/compaction.c~mm-compaction-make-compact_control-order-signed-fix
> +++ a/mm/compaction.c
> @@ -686,7 +686,7 @@ static int __compact_pgdat(pg_data_t *pg
>   		INIT_LIST_HEAD(&cc->freepages);
>   		INIT_LIST_HEAD(&cc->migratepages);
>
> -		if (cc->order<  0 || !compaction_deferred(zone, cc->order))
> +		if (cc->order = -1 || !compaction_deferred(zone, cc->order))
>   			compact_zone(zone, cc);
>
>   		if (cc->order>  0) {

Agreed, with that and Dan's patch, things should all be
as expected.

-- 
All rights reversed