Skip to contentSkip to conversation

mm: do not sleep in balance_pgdat if there's no i/o congestion

PATCH In review Zlatko Calusic 6 participants 16 messages Dec 19, 2012 – Dec 31, 2012

Subsystems (3)
Memory management MGLRU MM reclaim
lore
Share & tools
lore

Zlatko Calusic

author
#0 lore raw

PATCH 1/1 mm: do not sleep in balance_pgdat if there's no i/o congestion

On a 4GB RAM machine, where Normal zone is much smaller than DMA32 zone, the Normal zone gets fragmented in time. This requires relatively more pressure in balance_pgdat to get the zone above the required watermark. Unfortunately, the congestion_wait() call in there slows it down for a completely wrong reason, expecting that there's a lot of writeback/swapout, even when there's none (much more common). After a few days, when fragmentation progresses, this flawed logic translates to a very high CPU iowait times, even though there's no I/O congestion at all. If THP is enabled, the problem occurs sooner, but I was able to see it even on !THP kernels, just by giving it a bit more time to occur.

The proper way to deal with this is to not wait, unless there's congestion. Thanks to Mel Gorman, we already have the function that perfectly fits the job. The patch was tested on a machine which nicely revealed the problem after only 1 day of uptime, and it's been working great.

---
 mm/vmscan.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
mm/vmscan.c +6 -6
index b7ed376..4588d1d 100644
@@ -2546,7 +2546,7 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 							int *classzone_idx)
 {
-	int all_zones_ok;
+	struct zone *unbalanced_zone;
 	unsigned long balanced;
 	int i;
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */

@@ -2580,7 +2580,7 @@ loop_again:
 		unsigned long lru_pages = 0;
 		int has_under_min_watermark_zone = 0;
 
-		all_zones_ok = 1;
+		unbalanced_zone = NULL;
 		balanced = 0;
 
 		/*

@@ -2719,7 +2719,7 @@ loop_again:
 			}
 
 			if (!zone_balanced(zone, testorder, 0, end_zone)) {
-				all_zones_ok = 0;
+				unbalanced_zone = zone;
 				/*
 				 * We are still under min water mark.  This
 				 * means that we have a GFP_ATOMIC allocation

@@ -2752,7 +2752,7 @@ loop_again:
 				pfmemalloc_watermark_ok(pgdat))
 			wake_up(&pgdat->pfmemalloc_wait);
 
-		if (all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
 			break;		/* kswapd: all done */
 		/*
 		 * OK, kswapd is getting into trouble.  Take a nap, then take

@@ -2762,7 +2762,7 @@ loop_again:
 			if (has_under_min_watermark_zone)
 				count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
 			else
-				congestion_wait(BLK_RW_ASYNC, HZ/10);
+				wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
 		}
 
 		/*

@@ -2781,7 +2781,7 @@ out:
 	 * high-order: Balanced zones must make up at least 25% of the node
 	 *             for the node to be balanced
 	 */
-	if (!(all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))) {
+	if (unbalanced_zone && (!order || !pgdat_balanced(pgdat, balanced, *classzone_idx))) {
 		cond_resched();
 
 		try_to_freeze();
Signature · 10 lines
1.7.10.4

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#1 lore raw

On a 4GB RAM machine, where Normal zone is much smaller than DMA32 zone, the Normal zone gets fragmented in time. This requires relatively more pressure in balance_pgdat to get the zone above the required watermark. Unfortunately, the congestion_wait() call in there slows it down for a completely wrong reason, expecting that there's a lot of writeback/swapout, even when there's none (much more common). After a few days, when fragmentation progresses, this flawed logic translates to a very high CPU iowait times, even though there's no I/O congestion at all. If THP is enabled, the problem occurs sooner, but I was able to see it even on !THP kernels, just by giving it a bit more time to occur.

The proper way to deal with this is to not wait, unless there's congestion. Thanks to Mel Gorman, we already have the function that perfectly fits the job. The patch was tested on a machine which nicely revealed the problem after only 1 day of uptime, and it's been working great.

Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
mm/vmscan.c +6 -6
index b7ed376..4588d1d 100644
@@ -2546,7 +2546,7 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 							int *classzone_idx)
 {
-	int all_zones_ok;
+	struct zone *unbalanced_zone;
 	unsigned long balanced;
 	int i;
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */

@@ -2580,7 +2580,7 @@ loop_again:
 		unsigned long lru_pages = 0;
 		int has_under_min_watermark_zone = 0;
 
-		all_zones_ok = 1;
+		unbalanced_zone = NULL;
 		balanced = 0;
 
 		/*

@@ -2719,7 +2719,7 @@ loop_again:
 			}
 
 			if (!zone_balanced(zone, testorder, 0, end_zone)) {
-				all_zones_ok = 0;
+				unbalanced_zone = zone;
 				/*
 				 * We are still under min water mark.  This
 				 * means that we have a GFP_ATOMIC allocation

@@ -2752,7 +2752,7 @@ loop_again:
 				pfmemalloc_watermark_ok(pgdat))
 			wake_up(&pgdat->pfmemalloc_wait);
 
-		if (all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
 			break;		/* kswapd: all done */
 		/*
 		 * OK, kswapd is getting into trouble.  Take a nap, then take

@@ -2762,7 +2762,7 @@ loop_again:
 			if (has_under_min_watermark_zone)
 				count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
 			else
-				congestion_wait(BLK_RW_ASYNC, HZ/10);
+				wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
 		}
 
 		/*

@@ -2781,7 +2781,7 @@ out:
 	 * high-order: Balanced zones must make up at least 25% of the node
 	 *             for the node to be balanced
 	 */
-	if (!(all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))) {
+	if (unbalanced_zone && (!order || !pgdat_balanced(pgdat, balanced, *classzone_idx))) {
 		cond_resched();
 
 		try_to_freeze();

-- 1.7.10.4

Signature · 7 lines
Zlatko (this time with proper Signed-off-by line)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#2 lore raw

mm/vmscan.c static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining, · line 2549

Quoted 5 lines · Zlatko Calusic

static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
int *classzone_idx)
{

mm/vmscan.c +1 -1
-       int all_zones_ok;
+       struct zone *unbalanced_zone;

nit: less hunks if not erase that mark

Hillf

Quoted 40 lines
        unsigned long balanced;
        int i;
        int end_zone = 0;       /* Inclusive.  0 = ZONE_DMA */
mm/vmscan.c +4 -4
@@ -2580,7 +2580,7 @@ loop_again:
                unsigned long lru_pages = 0;
                int has_under_min_watermark_zone = 0;
 
-               all_zones_ok = 1;
+               unbalanced_zone = NULL;
                balanced = 0;
 
                /*

@@ -2719,7 +2719,7 @@ loop_again:
                        }
 
                        if (!zone_balanced(zone, testorder, 0, end_zone)) {
-                               all_zones_ok = 0;
+                               unbalanced_zone = zone;
                                /*
                                 * We are still under min water mark.  This
                                 * means that we have a GFP_ATOMIC allocation

@@ -2752,7 +2752,7 @@ loop_again:
                                pfmemalloc_watermark_ok(pgdat))
                        wake_up(&pgdat->pfmemalloc_wait);
 
-               if (all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+               if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
                        break;          /* kswapd: all done */
                /*
                 * OK, kswapd is getting into trouble.  Take a nap, then take

@@ -2762,7 +2762,7 @@ loop_again:
                        if (has_under_min_watermark_zone)
                                count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
                        else
-                               congestion_wait(BLK_RW_ASYNC, HZ/10);
+                               wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
                }
 
                /*

mm/vmscan.c out:

Quoted 9 lines
mm/vmscan.c +1 -2
@@ -2781,7 +2781,7 @@ out:
         * high-order: Balanced zones must make up at least 25% of the node
         *             for the node to be balanced
         */
-       if (!(all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))) {
+       if (unbalanced_zone && (!order || !pgdat_balanced(pgdat, balanced, *classzone_idx))) {
                cond_resched();
 
                try_to_freeze();
-- 1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#3 lore raw
Quoted 10 lines · Hillf Danton

mm/vmscan.c static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining, · line 2549

Quoted 5 lines · Zlatko Calusic

static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
int *classzone_idx)
{

mm/vmscan.c +1 -1
-       int all_zones_ok;
+       struct zone *unbalanced_zone;

nit: less hunks if not erase that mark

Hillf

This one left unanswered and forgotten because I didn't understand what you meant. Could you elaborate?

Signature · 7 lines
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#4 lore raw
Quoted 13 lines · Zlatko Calusic
Quoted 12 lines · Hillf Danton
Quoted 7 lines · Zlatko Calusic

static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
int
*classzone_idx)
{

mm/vmscan.c +1 -1
-       int all_zones_ok;
+       struct zone *unbalanced_zone;
Quoted 8 lines
Quoted 3 lines

nit: less hunks if not erase that mark

Hillf

This one left unanswered and forgotten because I didn't understand what you meant. Could you elaborate?

Sure, the patch looks simpler(and nicer) if we dont erase all_zones_ok.

Hillf

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw
Quoted 17 lines · Hillf Danton
Quoted 15 lines · Zlatko Calusic
Quoted 14 lines · Hillf Danton

mm/vmscan.c static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining, · line 2549

Quoted 7 lines · Zlatko Calusic

static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
int
*classzone_idx)
{

mm/vmscan.c +1 -1
-       int all_zones_ok;
+       struct zone *unbalanced_zone;

nit: less hunks if not erase that mark

Quoted 9 lines
Quoted 6 lines
Quoted 1 line

Hillf

This one left unanswered and forgotten because I didn't understand what you meant. Could you elaborate?

Sure, the patch looks simpler(and nicer) if we dont erase all_zones_ok.

Ah, yes. I gave it a good thought. But, when I introduced unbalanced_zone it just didn't make much sense to me to have two variables with very similar meaning. If I decided to keep all_zones_ok, it would be either:

all_zones_ok = true
unbalanced_zone = NULL
(meaning: if no zone in unbalanced, then all zones must be ok)

or

all_zones_ok = false
unbalanced_zone = struct zone *
(meaning: if there's an unbalanced zone, then certainly not all zones are ok)

So I decided to use only unbalanced_zone (because I had to!), and remove all_zones_ok to avoid redundancy. I hope it makes sense.

If you check my latest (and still queued) optimization (mm: avoid calling pgdat_balanced() needlessly), there again popped up a need for a boolean, but I called it pgdat_is_balanced this time, just to match the name of two other functions. It could've also been called all_zones_ok if you prefer the name? Of course, I have no strong feelings about the name, both are OK, so if you want me to redo the patch, just say.

Generally speaking, while I always attempt to make a smaller patch (less hunks and less changes = easier to review), before that I'll always try to make the code that results from the commit cleaner, simpler, more readable.

For example, I'll always check that I don't mess with whitespace needlessly, unless I think it's actually desirable, here's just one example:

"mm: avoid calling pgdat_balanced() needlessly" changes

---

        } while (--sc.priority >= 0);

out:

        if (!pgdat_balanced(pgdat, order, *classzone_idx)) {

---

to

---

        } while (--sc.priority >= 0);

out:

        if (!pgdat_is_balanced) {

---

because I find the latter more correct place for the label "out".

Thanks for the comment.

Signature · 7 lines
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Acked-by on v1 1/1
#6 lore raw
In reply to Zlatko Calusic
Quoted 12 lines · Zlatko Calusic

On a 4GB RAM machine, where Normal zone is much smaller than DMA32 zone, the Normal zone gets fragmented in time. This requires relatively more pressure in balance_pgdat to get the zone above the required watermark. Unfortunately, the congestion_wait() call in there slows it down for a completely wrong reason, expecting that there's a lot of writeback/swapout, even when there's none (much more common). After a few days, when fragmentation progresses, this flawed logic translates to a very high CPU iowait times, even though there's no I/O congestion at all. If THP is enabled, the problem occurs sooner, but I was able to see it even on !THP kernels, just by giving it a bit more time to occur.

Quoted 9 lines

The proper way to deal with this is to not wait, unless there's congestion. Thanks to Mel Gorman, we already have the function that perfectly fits the job. The patch was tested on a machine which nicely revealed the problem after only 1 day of uptime, and it's been working great.

---
 mm/vmscan.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Acked-by Mel Gorman <[email protected]
Signature · 8 lines
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#7 lore raw
Quoted 23 lines · On Thu, 20 Dec 2012 11:12:08 +0000 Mel Gorman <[email protected]> wrote:
Quoted 12 lines · Zlatko Calusic

On a 4GB RAM machine, where Normal zone is much smaller than DMA32 zone, the Normal zone gets fragmented in time. This requires relatively more pressure in balance_pgdat to get the zone above the required watermark. Unfortunately, the congestion_wait() call in there slows it down for a completely wrong reason, expecting that there's a lot of writeback/swapout, even when there's none (much more common). After a few days, when fragmentation progresses, this flawed logic translates to a very high CPU iowait times, even though there's no I/O congestion at all. If THP is enabled, the problem occurs sooner, but I was able to see it even on !THP kernels, just by giving it a bit more time to occur.

Quoted 9 lines

The proper way to deal with this is to not wait, unless there's congestion. Thanks to Mel Gorman, we already have the function that perfectly fits the job. The patch was tested on a machine which nicely revealed the problem after only 1 day of uptime, and it's been working great.

---
 mm/vmscan.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Quoted 1 line
Acked-by Mel Gorman <[email protected]

There seems to be some complexity/duplication here between the new unbalanced_zone() and pgdat_balanced().

Can we modify pgdat_balanced() so that it also handles order=0, then do

- if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+ if (!pgdat_balanced(...))

?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#8 lore raw

PATCH 1/1 mm: modify pgdat_balanced() so that it also handles order=0

Quoted 10 lines · Andrew Morton

There seems to be some complexity/duplication here between the new unbalanced_zone() and pgdat_balanced().

Can we modify pgdat_balanced() so that it also handles order=0, then do

(quoted diff) +1 -1
-		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (!pgdat_balanced(...))

?

Makes sense, I like the idea! Took me some time to wrap my mind around all the logic in balance_pgdat(), while writing my previous patch. Also had to revert one if-case logic to avoid double negation, which would be even harder to grok. But unbalanced_zone (var, not a function!) has to stay because wait_iff_congested() needs a struct zone* param. Here's my take on the subject:

---8<---
mm: modify pgdat_balanced() so that it also handles order=0

Teach pgdat_balanced() about order-0 allocations so that we can simplify code in a few places in vmstat.c.

Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c | 101 +++++++++++++++++++++++++++---------------------------------
 1 file changed, 45 insertions(+), 56 deletions(-)
mm/vmscan.c +45 -56
index adc7e90..0d15d99 100644
@@ -2452,12 +2452,16 @@ static bool zone_balanced(struct zone *zone, int order,
 }
 
 /*
- * pgdat_balanced is used when checking if a node is balanced for high-order
- * allocations. Only zones that meet watermarks and are in a zone allowed
- * by the callers classzone_idx are added to balanced_pages. The total of
- * balanced pages must be at least 25% of the zones allowed by classzone_idx
- * for the node to be considered balanced. Forcing all zones to be balanced
- * for high orders can cause excessive reclaim when there are imbalanced zones.
+ * pgdat_balanced() is used when checking if a node is balanced.
+ *
+ * For order-0, all zones must be balanced!
+ *
+ * For high-order allocations only zones that meet watermarks and are in a
+ * zone allowed by the callers classzone_idx are added to balanced_pages. The
+ * total of balanced pages must be at least 25% of the zones allowed by
+ * classzone_idx for the node to be considered balanced. Forcing all zones to
+ * be balanced for high orders can cause excessive reclaim when there are
+ * imbalanced zones.
  * The choice of 25% is due to
  *   o a 16M DMA zone that is balanced will not balance a zone on any
  *     reasonable sized machine

@@ -2467,17 +2471,43 @@ static bool zone_balanced(struct zone *zone, int order,
  *     Similarly, on x86-64 the Normal zone would need to be at least 1G
  *     to balance a node on its own. These seemed like reasonable ratios.
  */
-static bool pgdat_balanced(pg_data_t *pgdat, unsigned long balanced_pages,
-						int classzone_idx)
+static bool pgdat_balanced(pg_data_t *pgdat, int order, int classzone_idx)
 {
 	unsigned long present_pages = 0;
+	unsigned long balanced_pages = 0;
 	int i;
 
-	for (i = 0; i <= classzone_idx; i++)
-		present_pages += pgdat->node_zones[i].present_pages;
+	/* Check the watermark levels */
+	for (i = 0; i <= classzone_idx; i++) {
+		struct zone *zone = pgdat->node_zones + i;
+
+		if (!populated_zone(zone))
+			continue;
+
+		present_pages += zone->present_pages;
+
+		/*
+		 * A special case here:
+		 *
+		 * balance_pgdat() skips over all_unreclaimable after
+		 * DEF_PRIORITY. Effectively, it considers them balanced so
+		 * they must be considered balanced here as well!
+		 */
+		if (zone->all_unreclaimable) {
+			balanced_pages += zone->present_pages;
+			continue;
+		}
+
+		if (zone_balanced(zone, order, 0, i))
+			balanced_pages += zone->present_pages;
+		else if (!order)
+			return false;
+	}
 
-	/* A special case here: if zone has no page, we think it's balanced */
-	return balanced_pages >= (present_pages >> 2);
+	if (order)
+		return balanced_pages >= (present_pages >> 2);
+	else
+		return true;
 }
 
 /*

@@ -2511,39 +2541,7 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 		return false;
 	}
 
-	/* Check the watermark levels */
-	for (i = 0; i <= classzone_idx; i++) {
-		struct zone *zone = pgdat->node_zones + i;
-
-		if (!populated_zone(zone))
-			continue;
-
-		/*
-		 * balance_pgdat() skips over all_unreclaimable after
-		 * DEF_PRIORITY. Effectively, it considers them balanced so
-		 * they must be considered balanced here as well if kswapd
-		 * is to sleep
-		 */
-		if (zone->all_unreclaimable) {
-			balanced += zone->present_pages;
-			continue;
-		}
-
-		if (!zone_balanced(zone, order, 0, i))
-			all_zones_ok = false;
-		else
-			balanced += zone->present_pages;
-	}
-
-	/*
-	 * For high-order requests, the balanced zones must contain at least
-	 * 25% of the nodes pages for kswapd to sleep. For order-0, all zones
-	 * must be balanced
-	 */
-	if (order)
-		return pgdat_balanced(pgdat, balanced, classzone_idx);
-	else
-		return all_zones_ok;
+	return pgdat_balanced(pgdat, order, classzone_idx);
 }
 
 /*

@@ -2571,7 +2569,6 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 							int *classzone_idx)
 {
 	struct zone *unbalanced_zone;
-	unsigned long balanced;
 	int i;
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
 	unsigned long total_scanned;

@@ -2605,7 +2602,6 @@ loop_again:
 		int has_under_min_watermark_zone = 0;
 
 		unbalanced_zone = NULL;
-		balanced = 0;
 
 		/*
 		 * Scan in the highmem->dma direction for the highest

@@ -2761,8 +2757,6 @@ loop_again:
 				 * speculatively avoid congestion waits
 				 */
 				zone_clear_flag(zone, ZONE_CONGESTED);
-				if (i <= *classzone_idx)
-					balanced += zone->present_pages;
 			}
 
 		}

@@ -2776,7 +2770,7 @@ loop_again:
 				pfmemalloc_watermark_ok(pgdat))
 			wake_up(&pgdat->pfmemalloc_wait);
 
-		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (pgdat_balanced(pgdat, order, *classzone_idx))
 			break;		/* kswapd: all done */
 		/*
 		 * OK, kswapd is getting into trouble.  Take a nap, then take

@@ -2800,12 +2794,7 @@ loop_again:
 	} while (--sc.priority >= 0);
 out:
 
-	/*
-	 * order-0: All zones must meet high watermark for a balanced node
-	 * high-order: Balanced zones must make up at least 25% of the node
-	 *             for the node to be balanced
-	 */
-	if (unbalanced_zone && (!order || !pgdat_balanced(pgdat, balanced, *classzone_idx))) {
+	if (!pgdat_balanced(pgdat, order, *classzone_idx)) {
 		cond_resched();
 
 		try_to_freeze();
Signature · 10 lines
1.8.1.rc0

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#9 lore raw

PATCH v2 1/1 mm: modify pgdat_balanced() so that it also handles order=0

Quoted 9 lines · Zlatko Calusic
Quoted 8 lines · Andrew Morton

There seems to be some complexity/duplication here between the new unbalanced_zone() and pgdat_balanced().

Can we modify pgdat_balanced() so that it also handles order=0, then do

(quoted diff) +1 -1
-		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (!pgdat_balanced(...))
Quoted 9 lines
Quoted 2 lines

?

Makes sense, I like the idea! Took me some time to wrap my mind around all the logic in balance_pgdat(), while writing my previous patch. Also had to revert one if-case logic to avoid double negation, which would be even harder to grok. But unbalanced_zone (var, not a function!) has to stay because wait_iff_congested() needs a struct zone* param. Here's my take on the subject:

And now also with 3 unused variables removed, no other changes. prepare_kswapd_sleep() now looks so beautiful. ;)

I've been testing the patch on 3 different machines, and no problem at all. One of those I pushed hard, and it survived.

---8<---
mm: modify pgdat_balanced() so that it also handles order=0

Teach pgdat_balanced() about order-0 allocations so that we can simplify code in a few places in vmstat.c.

Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c | 105 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 45 insertions(+), 60 deletions(-)
mm/vmscan.c +45 -60
index adc7e90..23291b9 100644
@@ -2452,12 +2452,16 @@ static bool zone_balanced(struct zone *zone, int order,
 }
 
 /*
- * pgdat_balanced is used when checking if a node is balanced for high-order
- * allocations. Only zones that meet watermarks and are in a zone allowed
- * by the callers classzone_idx are added to balanced_pages. The total of
- * balanced pages must be at least 25% of the zones allowed by classzone_idx
- * for the node to be considered balanced. Forcing all zones to be balanced
- * for high orders can cause excessive reclaim when there are imbalanced zones.
+ * pgdat_balanced() is used when checking if a node is balanced.
+ *
+ * For order-0, all zones must be balanced!
+ *
+ * For high-order allocations only zones that meet watermarks and are in a
+ * zone allowed by the callers classzone_idx are added to balanced_pages. The
+ * total of balanced pages must be at least 25% of the zones allowed by
+ * classzone_idx for the node to be considered balanced. Forcing all zones to
+ * be balanced for high orders can cause excessive reclaim when there are
+ * imbalanced zones.
  * The choice of 25% is due to
  *   o a 16M DMA zone that is balanced will not balance a zone on any
  *     reasonable sized machine

@@ -2467,17 +2471,43 @@ static bool zone_balanced(struct zone *zone, int order,
  *     Similarly, on x86-64 the Normal zone would need to be at least 1G
  *     to balance a node on its own. These seemed like reasonable ratios.
  */
-static bool pgdat_balanced(pg_data_t *pgdat, unsigned long balanced_pages,
-						int classzone_idx)
+static bool pgdat_balanced(pg_data_t *pgdat, int order, int classzone_idx)
 {
 	unsigned long present_pages = 0;
+	unsigned long balanced_pages = 0;
 	int i;
 
-	for (i = 0; i <= classzone_idx; i++)
-		present_pages += pgdat->node_zones[i].present_pages;
+	/* Check the watermark levels */
+	for (i = 0; i <= classzone_idx; i++) {
+		struct zone *zone = pgdat->node_zones + i;
 
-	/* A special case here: if zone has no page, we think it's balanced */
-	return balanced_pages >= (present_pages >> 2);
+		if (!populated_zone(zone))
+			continue;
+
+		present_pages += zone->present_pages;
+
+		/*
+		 * A special case here:
+		 *
+		 * balance_pgdat() skips over all_unreclaimable after
+		 * DEF_PRIORITY. Effectively, it considers them balanced so
+		 * they must be considered balanced here as well!
+		 */
+		if (zone->all_unreclaimable) {
+			balanced_pages += zone->present_pages;
+			continue;
+		}
+
+		if (zone_balanced(zone, order, 0, i))
+			balanced_pages += zone->present_pages;
+		else if (!order)
+			return false;
+	}
+
+	if (order)
+		return balanced_pages >= (present_pages >> 2);
+	else
+		return true;
 }
 
 /*

@@ -2489,10 +2519,6 @@ static bool pgdat_balanced(pg_data_t *pgdat, unsigned long balanced_pages,
 static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 					int classzone_idx)
 {
-	int i;
-	unsigned long balanced = 0;
-	bool all_zones_ok = true;
-
 	/* If a direct reclaimer woke kswapd within HZ/10, it's premature */
 	if (remaining)
 		return false;

@@ -2511,39 +2537,7 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 		return false;
 	}
 
-	/* Check the watermark levels */
-	for (i = 0; i <= classzone_idx; i++) {
-		struct zone *zone = pgdat->node_zones + i;
-
-		if (!populated_zone(zone))
-			continue;
-
-		/*
-		 * balance_pgdat() skips over all_unreclaimable after
-		 * DEF_PRIORITY. Effectively, it considers them balanced so
-		 * they must be considered balanced here as well if kswapd
-		 * is to sleep
-		 */
-		if (zone->all_unreclaimable) {
-			balanced += zone->present_pages;
-			continue;
-		}
-
-		if (!zone_balanced(zone, order, 0, i))
-			all_zones_ok = false;
-		else
-			balanced += zone->present_pages;
-	}
-
-	/*
-	 * For high-order requests, the balanced zones must contain at least
-	 * 25% of the nodes pages for kswapd to sleep. For order-0, all zones
-	 * must be balanced
-	 */
-	if (order)
-		return pgdat_balanced(pgdat, balanced, classzone_idx);
-	else
-		return all_zones_ok;
+	return pgdat_balanced(pgdat, order, classzone_idx);
 }
 
 /*

@@ -2571,7 +2565,6 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 							int *classzone_idx)
 {
 	struct zone *unbalanced_zone;
-	unsigned long balanced;
 	int i;
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
 	unsigned long total_scanned;

@@ -2605,7 +2598,6 @@ loop_again:
 		int has_under_min_watermark_zone = 0;
 
 		unbalanced_zone = NULL;
-		balanced = 0;
 
 		/*
 		 * Scan in the highmem->dma direction for the highest

@@ -2761,8 +2753,6 @@ loop_again:
 				 * speculatively avoid congestion waits
 				 */
 				zone_clear_flag(zone, ZONE_CONGESTED);
-				if (i <= *classzone_idx)
-					balanced += zone->present_pages;
 			}
 
 		}

@@ -2776,7 +2766,7 @@ loop_again:
 				pfmemalloc_watermark_ok(pgdat))
 			wake_up(&pgdat->pfmemalloc_wait);
 
-		if (!unbalanced_zone || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
+		if (pgdat_balanced(pgdat, order, *classzone_idx))
 			break;		/* kswapd: all done */
 		/*
 		 * OK, kswapd is getting into trouble.  Take a nap, then take

@@ -2800,12 +2790,7 @@ loop_again:
 	} while (--sc.priority >= 0);
 out:
 
-	/*
-	 * order-0: All zones must meet high watermark for a balanced node
-	 * high-order: Balanced zones must make up at least 25% of the node
-	 *             for the node to be balanced
-	 */
-	if (unbalanced_zone && (!order || !pgdat_balanced(pgdat, balanced, *classzone_idx))) {
+	if (!pgdat_balanced(pgdat, order, *classzone_idx)) {
 		cond_resched();
 
 		try_to_freeze();
Signature · 10 lines
1.8.1.rc0

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#10 lore raw

PATCH 1/1 mm: avoid calling pgdat_balanced() needlessly

Now that balance_pgdat() is slightly tidied up, thanks to more capable pgdat_balanced(), it's become obvious that pgdat_balanced() is called to check the status, then break the loop if pgdat is balanced, just to be immediately called again. The second call is completely unnecessary, of course.

The patch introduces pgdat_is_balanced boolean, which helps resolve the above suboptimal behavior, with the added benefit of slightly better documenting one other place in the function where we jump and skip lots of code.

Signed-off-by Zlatko Calusic <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Hugh Dickins <[email protected]>
---
 mm/vmscan.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
mm/vmscan.c +11 -4
index 23291b9..02bcfa3 100644
@@ -2564,6 +2564,7 @@ static bool prepare_kswapd_sleep(pg_data_t *pgdat, int order, long remaining,
 static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 							int *classzone_idx)
 {
+	bool pgdat_is_balanced = false;
 	struct zone *unbalanced_zone;
 	int i;
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */

@@ -2638,8 +2639,11 @@ loop_again:
 				zone_clear_flag(zone, ZONE_CONGESTED);
 			}
 		}
-		if (i < 0)
+
+		if (i < 0) {
+			pgdat_is_balanced = true;
 			goto out;
+		}
 
 		for (i = 0; i <= end_zone; i++) {
 			struct zone *zone = pgdat->node_zones + i;

@@ -2766,8 +2770,11 @@ loop_again:
 				pfmemalloc_watermark_ok(pgdat))
 			wake_up(&pgdat->pfmemalloc_wait);
 
-		if (pgdat_balanced(pgdat, order, *classzone_idx))
+		if (pgdat_balanced(pgdat, order, *classzone_idx)) {
+			pgdat_is_balanced = true;
 			break;		/* kswapd: all done */
+		}
+
 		/*
 		 * OK, kswapd is getting into trouble.  Take a nap, then take
 		 * another pass across the zones.

@@ -2788,9 +2795,9 @@ loop_again:
 		if (sc.nr_reclaimed >= SWAP_CLUSTER_MAX)
 			break;
 	} while (--sc.priority >= 0);
-out:
 
-	if (!pgdat_balanced(pgdat, order, *classzone_idx)) {
+out:
+	if (!pgdat_is_balanced) {
 		cond_resched();
 
 		try_to_freeze();
Signature · 10 lines
1.8.1.rc0

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#11 lore raw

PATCH 1/1 mm: fix null pointer dereference in wait_iff_congested()

From: Zlatko Calusic <[email protected]>

The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this:

BUG: unable to handle kernel NULL pointer dereference
IP: [<ffffffff811542d9>] wait_iff_congested+0x59/0x140

This trivial patch fixes it.

Reported-by Zhouping Liu <[email protected]>
Reported-and-tested-by Sedat Dilek <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Hugh Dickins <[email protected]>
Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +1 -1
index 02bcfa3..e55ce55 100644
@@ -2782,7 +2782,7 @@ loop_again:
 		if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
 			if (has_under_min_watermark_zone)
 				count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
-			else
+			else if (unbalanced_zone)
 				wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
 		}
Signature · 10 lines
1.8.1.rc3

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#12 lore raw

Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()

Hello Zlatko,

Quoted 5 lines · Zlatko Calusic

From: Zlatko Calusic <[email protected]>

The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this:

For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex.

As I see the code, we have following line above wait_iff_congested.

if (!unbalanced_zone || blah blah)

        break;

How can NULL unbalanced_zone reach wait_iff_congested?

Quoted 40 lines
BUG: unable to handle kernel NULL pointer dereference
IP: [<ffffffff811542d9>] wait_iff_congested+0x59/0x140

This trivial patch fixes it.

Reported-by Zhouping Liu <[email protected]>
Reported-and-tested-by Sedat Dilek <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Hugh Dickins <[email protected]>
Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +1 -1
index 02bcfa3..e55ce55 100644
@@ -2782,7 +2782,7 @@ loop_again:
 		if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
 			if (has_under_min_watermark_zone)
 				count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
-			else
+			else if (unbalanced_zone)
 				wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
 		}
Signature · 10 lines
1.8.1.rc3

-- 
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#13 lore raw

Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()

Quoted 13 lines · Minchan Kim

Hello Zlatko,

Quoted 5 lines · Zlatko Calusic

From: Zlatko Calusic <[email protected]>

The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this:

For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex.

Quoted 7 lines

As I see the code, we have following line above wait_iff_congested.

if (!unbalanced_zone || blah blah)

         break;

How can NULL unbalanced_zone reach wait_iff_congested?

Hello Minchan, and thanks for the comment.

That line was there before commit 4ae0a48b got in, and you're right, it's what was protecting wait_iff_congested() from being called with NULL zone*. But then all that logic got colapsed to a simple pgdat_balanced() call and that's when I introduced the bug, I lost the protection.

What I _think_ is happening (pseudo code following...) is that after scanning the zone in the dma->highmem direction, and concluding that all zones are balanced (unbalanced_zone remains NULL!), wake_up(&pgdat->pfmemalloc_wait) wakes up a lot of memory hungry processes (especially true in various aggressive test/benchmarks) that immediately drain and unbalance one or more zones. Then pgdat_balanced() call which immediately follows will be false, but we still have unbalanced_zone = NULL, rememeber? Oops...

But, all that is a speculation that I can't prove atm. Of course, if anybody thinks that's a credible explanation, I could add it as a commit comment, or even as a code comment, but I didn't want to be overly imaginative. The fix itself is simple and real.

Regards,

Signature · 7 lines
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#14 lore raw

Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()

Hi Zlatko,

Quoted 19 lines · Zlatko Calusic
Quoted 18 lines · Minchan Kim

Hello Zlatko,

Quoted 5 lines · Zlatko Calusic

From: Zlatko Calusic <[email protected]>

The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this:

For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex.

As I see the code, we have following line above wait_iff_congested.

if (!unbalanced_zone || blah blah)

        break;
Quoted 6 lines
Quoted 2 lines

How can NULL unbalanced_zone reach wait_iff_congested?

Hello Minchan, and thanks for the comment.

That line was there before commit 4ae0a48b got in, and you're right,

Argh, I didn't see 4ae0a48b in 3.8-rc1.

Quoted 14 lines

it's what was protecting wait_iff_congested() from being called with NULL zone*. But then all that logic got colapsed to a simple pgdat_balanced() call and that's when I introduced the bug, I lost the protection.

What I _think_ is happening (pseudo code following...) is that after scanning the zone in the dma->highmem direction, and concluding that all zones are balanced (unbalanced_zone remains NULL!), wake_up(&pgdat->pfmemalloc_wait) wakes up a lot of memory hungry processes (especially true in various aggressive test/benchmarks) that immediately drain and unbalance one or more zones. Then pgdat_balanced() call which immediately follows will be false, but we still have unbalanced_zone = NULL, rememeber? Oops...

Quoted 4 lines

But, all that is a speculation that I can't prove atm. Of course, if anybody thinks that's a credible explanation, I could add it as a commit comment, or even as a code comment, but I didn't want to be overly imaginative. The fix itself is simple and real.

Never mind. My confusing is caused my missing 4ae0a48b in lasest tree. Thanks, Zlatko.

Quoted 10 lines

Regards,

Signature · 7 lines
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#15 lore raw

Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()

In reply to Zlatko Calusic

Just FYI:

This patch landed upstream [1].
Thanks for all involved people.

- Sedat -

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=ecccd1248d6e6986130ffcc3b0d003cb46a485c0

Quoted 37 lines · Zlatko Calusic

From: Zlatko Calusic <[email protected]>

The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this:

BUG: unable to handle kernel NULL pointer dereference
IP: [<ffffffff811542d9>] wait_iff_congested+0x59/0x140

This trivial patch fixes it.

Reported-by Zhouping Liu <[email protected]>
Reported-and-tested-by Sedat Dilek <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Hugh Dickins <[email protected]>
Signed-off-by Zlatko Calusic <[email protected]>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +1 -2
index 02bcfa3..e55ce55 100644
@@ -2782,7 +2782,7 @@ loop_again:
                if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
                        if (has_under_min_watermark_zone)
                                count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
-                       else
+                       else if (unbalanced_zone)
                                wait_iff_congested(unbalanced_zone, BLK_RW_ASYNC, HZ/10);
                }
 
--

1.8.1.rc3

Quoted 2 lines

--
Zlatko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>