Skip to contentSkip to conversation

mm: compaction: make compact_control order signed

PATCH In review Dan Carpenter 3 participants 6 messages Feb 1, 2012

Subsystems (3)
Memory management Kernel janitors Page allocator
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mm: compaction: make compact_control order signed

"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 <[email protected]>
mm/compaction.c +1 -1
index 382831e..5f80a11 100644
@@ -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;
 };
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · Dan Carpenter

"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 :)

Quoted 1 line
Signed-off-by Dan Carpenter<[email protected]>
Reviewed-by Rik van Riel <[email protected]>
Signature · 1 line
All rights reversed
maintainer
#2 lore raw
In reply to Dan Carpenter
Quoted 13 lines · On Wed, 1 Feb 2012 17:41:01 +0300 Dan Carpenter <[email protected]>…

"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 <[email protected]>
mm/compaction.c +0 -0
index 382831e..5f80a11 100644

mm/compaction.c struct compact_control { · line 41

Quoted 8 lines
mm/compaction.c +1 -1
@@ -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?

#3 lore raw
Quoted 19 lines · Andrew Morton
Quoted 16 lines · On Wed, 1 Feb 2012 17:41:01 +0300 Dan Carpenter<[email protected]>…

"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<[email protected]>
mm/compaction.c +0 -0
index 382831e..5f80a11 100644
@@ -35,7 +35,7 @@ struct compact_control {
  	unsigned long migrate_pfn;	/* isolate_migratepages search base */
  	bool sync;			/* Synchronous migration */
Quoted 9 lines

mm/compaction.c struct compact_control { · line 41

Quoted 5 lines
mm/compaction.c +1 -1
-	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.

Signature · 1 line
All rights reversed
maintainer
#4 lore raw
Quoted 26 lines · On Wed, 01 Feb 2012 16:17:10 -0500 Rik van Riel <[email protected]> wrote:
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -35,7 +35,7 @@ struct compact_control {
  	unsigned long migrate_pfn;	/* isolate_migratepages search base */
  	bool sync;			/* Synchronous migration */
Quoted 9 lines

Review struct compact_control { · line 41

Quoted 5 lines
(quoted diff) +1 -1
-	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 :(

Quoted 4 lines

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:

mm/compaction.c renamed from mm/compaction.c~mm-compaction-make-compact_control-order-signed-fix +1 -1
@@ -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) {

_

#5 lore raw
Quoted 40 lines · Andrew Morton
Quoted 26 lines · On Wed, 01 Feb 2012 16:17:10 -0500 Rik van Riel<[email protected]> wrote:
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -35,7 +35,7 @@ struct compact_control {
   	unsigned long migrate_pfn;	/* isolate_migratepages search base */
   	bool sync;			/* Synchronous migration */
Quoted 9 lines

Review struct compact_control { · line 41

Quoted 5 lines
(quoted diff) +1 -1
-	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 :(

Quoted 4 lines

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:

mm/compaction.c static int __compact_pgdat(pg_data_t *pg · line 692

Quoted 8 lines
mm/compaction.c renamed from mm/compaction.c~mm-compaction-make-compact_control-order-signed-fix +1 -1
@@ -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.

Signature · 1 line
All rights reversed