* [patch] mm: compaction: make compact_control order signed
@ 2012-02-01 14:41 Dan Carpenter
2012-02-01 14:59 ` Rik van Riel
2012-02-01 20:46 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2012-02-01 14:41 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, Minchan Kim, Rik van Riel, Andrea Arcangeli, linux-mm,
kernel-janitors
"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 <dan.carpenter@oracle.com>
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;
};
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch] mm: compaction: make compact_control order signed
2012-02-01 14:41 [patch] mm: compaction: make compact_control order signed Dan Carpenter
@ 2012-02-01 14:59 ` Rik van Riel
2012-02-01 20:46 ` Andrew Morton
1 sibling, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2012-02-01 14:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andrew Morton, Mel Gorman, Minchan Kim, Andrea Arcangeli,
linux-mm, kernel-janitors
On 02/01/2012 09:41 AM, 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);
Good catch! I had not even thought to check whether
order was signed in struct compact_control, when I
saw code using -1 as an order in various places :)
> Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
Reviewed-by: Rik van Riel <riel@redhat.com>
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] mm: compaction: make compact_control order signed
2012-02-01 14:41 [patch] mm: compaction: make compact_control order signed Dan Carpenter
2012-02-01 14:59 ` Rik van Riel
@ 2012-02-01 20:46 ` Andrew Morton
2012-02-01 21:17 ` Rik van Riel
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-02-01 20:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mel Gorman, Minchan Kim, Rik van Riel, Andrea Arcangeli, linux-mm,
kernel-janitors
On Wed, 1 Feb 2012 17:41:01 +0300
Dan Carpenter <dan.carpenter@oracle.com> 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 <dan.carpenter@oracle.com>
>
> 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?
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm: compaction: make compact_control order signed
2012-02-01 20:46 ` Andrew Morton
@ 2012-02-01 21:17 ` Rik van Riel
2012-02-01 21:24 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Rik van Riel @ 2012-02-01 21:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Dan Carpenter, Mel Gorman, Minchan Kim, Andrea Arcangeli,
linux-mm, kernel-janitors
On 02/01/2012 03:46 PM, Andrew Morton wrote:
> On Wed, 1 Feb 2012 17:41:01 +0300
> Dan Carpenter<dan.carpenter@oracle.com> 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<dan.carpenter@oracle.com>
>>
>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm: compaction: make compact_control order signed
2012-02-01 21:17 ` Rik van Riel
@ 2012-02-01 21:24 ` Andrew Morton
2012-02-01 21:28 ` Rik van Riel
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-02-01 21:24 UTC (permalink / raw)
To: Rik van Riel
Cc: Dan Carpenter, Mel Gorman, Minchan Kim, Andrea Arcangeli,
linux-mm, kernel-janitors
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) {
_
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm: compaction: make compact_control order signed
2012-02-01 21:24 ` Andrew Morton
@ 2012-02-01 21:28 ` Rik van Riel
0 siblings, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2012-02-01 21:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Dan Carpenter, Mel Gorman, Minchan Kim, Andrea Arcangeli,
linux-mm, kernel-janitors
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
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-02-01 21:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-01 14:41 [patch] mm: compaction: make compact_control order signed Dan Carpenter
2012-02-01 14:59 ` Rik van Riel
2012-02-01 20:46 ` Andrew Morton
2012-02-01 21:17 ` Rik van Riel
2012-02-01 21:24 ` Andrew Morton
2012-02-01 21:28 ` Rik van Riel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).