mm: vmscan: Do not throttle based on pfmemalloc reserves if node has no ZONE_NORMAL

PATCH In review Mel Gorman 2 participants 3 messages Apr 22, 2014 – Apr 23, 2014

Subsystems (3)

Memory management MGLRU MM reclaim

lore

Share & tools

lore

Mel Gorman

author

2014-04-22 08:38 UTC #0 lore raw

PATCH 1/1 mm: vmscan: Do not throttle based on pfmemalloc reserves if node has no ZONE_NORMAL

throttle_direct_reclaim() is meant to trigger during swap-over-network during which the min watermark is treated as a pfmemalloc reserve. It throttes on the first node in the zonelist but this is flawed.

On a NUMA machine running a 32-bit kernel (I know) allocation requests freom CPUs on node 1 would detect no pfmemalloc reserves and the process gets throttled. This patch adjusts throttling of direct reclaim to throttle based on the first node in the zonelist that has a usable ZONE_NORMAL or lower zone.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/vmscan.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

mm/vmscan.c +27 -6

index 3f56c8d..9c4918e9 100644
@@ -2507,10 +2507,17 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
 
 	for (i = 0; i <= ZONE_NORMAL; i++) {
 		zone = &pgdat->node_zones[i];
+		if (!populated_zone(zone))
+			continue;
+
 		pfmemalloc_reserve += min_wmark_pages(zone);
 		free_pages += zone_page_state(zone, NR_FREE_PAGES);
 	}
 
+	/* If there are no reserves (unexpected config) then do not throttle */
+	if (!pfmemalloc_reserve)
+		return true;
+
 	wmark_ok = free_pages > pfmemalloc_reserve / 2;
 
 	/* kswapd must be awake if processes are being throttled */

@@ -2535,9 +2542,9 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
 static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
+	struct zoneref *z;
 	struct zone *zone;
-	int high_zoneidx = gfp_zone(gfp_mask);
-	pg_data_t *pgdat;
+	pg_data_t *pgdat = NULL;
 
 	/*
 	 * Kernel threads should not be throttled as they may be indirectly

@@ -2556,10 +2563,24 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (fatal_signal_pending(current))
 		goto out;
 
-	/* Check if the pfmemalloc reserves are ok */
-	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
-	pgdat = zone->zone_pgdat;
-	if (pfmemalloc_watermark_ok(pgdat))
+	/*
+	 * Check if the pfmemalloc reserves are ok by finding the first node
+	 * with a usable ZONE_NORMAL or lower zone
+	 */
+        for_each_zone_zonelist_nodemask(zone, z, zonelist,
+                                        gfp_mask, nodemask) {
+		if (zone_idx(zone) > ZONE_NORMAL)
+			continue;
+
+		/* Throttle based on the first usable node */
+		pgdat = zone->zone_pgdat;
+		if (pfmemalloc_watermark_ok(pgdat))
+			goto out;
+		break;
+	}
+
+	/* If no zone was usable by the allocation flags then do not throttle */
+	if (!pgdat)
 		goto out;
 
 	/* Account for the throttling */

--
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>

Andrew Morton

maintainer

2014-04-22 19:31 UTC #1 lore raw

Quoted 9 lines · On Tue, 22 Apr 2014 09:38:52 +0100 Mel Gorman <[email protected]> wrote:

throttle_direct_reclaim() is meant to trigger during swap-over-network during which the min watermark is treated as a pfmemalloc reserve. It throttes on the first node in the zonelist but this is flawed.
On a NUMA machine running a 32-bit kernel (I know) allocation requests freom CPUs on node 1 would detect no pfmemalloc reserves and the process gets throttled. This patch adjusts throttling of direct reclaim to throttle based on the first node in the zonelist that has a usable ZONE_NORMAL or lower zone.

I'm unable to determine from the above whether we should backport this fix. Please don't forget to describe the end-user visible effects of a bug when that isn't obvious.

mm/vmscan.c static bool pfmemalloc_watermark_ok(pg_data_t *pgdat) · line 2511

Quoted 8 lines

mm/vmscan.c +2 -0

@@ -2507,10 +2507,17 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
 
 	for (i = 0; i <= ZONE_NORMAL; i++) {
 		zone = &pgdat->node_zones[i];
+		if (!populated_zone(zone))
+			continue;

What's this? Performance tweak? Or does min_wmark_pages() return non-zero for an unpopulated zone, which seems odd.

Quoted 25 lines

mm/vmscan.c +6 -2

 		pfmemalloc_reserve += min_wmark_pages(zone);
 		free_pages += zone_page_state(zone, NR_FREE_PAGES);
 	}
 
+	/* If there are no reserves (unexpected config) then do not throttle */
+	if (!pfmemalloc_reserve)
+		return true;
+
 	wmark_ok = free_pages > pfmemalloc_reserve / 2;
 
 	/* kswapd must be awake if processes are being throttled */

@@ -2535,9 +2542,9 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
 static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
+	struct zoneref *z;
 	struct zone *zone;
-	int high_zoneidx = gfp_zone(gfp_mask);
-	pg_data_t *pgdat;
+	pg_data_t *pgdat = NULL;
 
 	/*
 	 * Kernel threads should not be throttled as they may be indirectly

@@ -2556,10 +2563,24 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (fatal_signal_pending(current))

mm/vmscan.c static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2569

Quoted 10 lines

mm/vmscan.c +4 -4

 		goto out;
 
-	/* Check if the pfmemalloc reserves are ok */
-	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
-	pgdat = zone->zone_pgdat;
-	if (pfmemalloc_watermark_ok(pgdat))
+	/*
+	 * Check if the pfmemalloc reserves are ok by finding the first node
+	 * with a usable ZONE_NORMAL or lower zone
+	 */

That comment tells us what the code does but not why it does it.

- Why do we ignore zones >= ZONE_NORMAL?

- Why do we throttle when there may be as-yet-unexamined nodes which have reclaimable pages?

mm/vmscan.c static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2571

Quoted 2 lines

mm/vmscan.c +2 -0

+        for_each_zone_zonelist_nodemask(zone, z, zonelist,
+                                        gfp_mask, nodemask) {

Those two lines have spaces-instead-of-tabs.

mm/vmscan.c static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2584

Quoted 13 lines

mm/vmscan.c +12 -0

+		if (zone_idx(zone) > ZONE_NORMAL)
+			continue;
+
+		/* Throttle based on the first usable node */
+		pgdat = zone->zone_pgdat;
+		if (pfmemalloc_watermark_ok(pgdat))
+			goto out;
+		break;
+	}
+
+	/* If no zone was usable by the allocation flags then do not throttle */
+	if (!pgdat)
 		goto out;

Mel Gorman

author

2014-04-23 13:52 UTC #2 lore raw

Quoted 12 lines · Andrew Morton

Quoted 9 lines · On Tue, 22 Apr 2014 09:38:52 +0100 Mel Gorman <[email protected]> wrote:
throttle_direct_reclaim() is meant to trigger during swap-over-network during which the min watermark is treated as a pfmemalloc reserve. It throttes on the first node in the zonelist but this is flawed.
On a NUMA machine running a 32-bit kernel (I know) allocation requests freom CPUs on node 1 would detect no pfmemalloc reserves and the process gets throttled. This patch adjusts throttling of direct reclaim to throttle based on the first node in the zonelist that has a usable ZONE_NORMAL or lower zone.

Quoted 4 lines

I'm unable to determine from the above whether we should backport this fix. Please don't forget to describe the end-user visible effects of a bug when that isn't obvious.

The user-visible impact is that a process running on CPU whose local memory node has no ZONE_NORMAL will stall for prolonged periods of time, possibly indefintely. This is due to throttle_direct_reclaim thinking the pfmemalloc reserves are depleted when in fact they don't exist on that node.

Strictly speaking this is stable material. I should have flagged it as such but hadn't as I was treating 32-bit kernels running on NUMA hardware as being a poor choice.

Quoted 4 lines

mm/vmscan.c +0 -0

@@ -2507,10 +2507,17 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)

Quoted 8 lines

mm/vmscan.c static bool pfmemalloc_watermark_ok(pg_data_t *pgdat) · line 2511
Quoted 4 lines
mm/vmscan.c +2 -0
 	for (i = 0; i <= ZONE_NORMAL; i++) {
 		zone = &pgdat->node_zones[i];
+		if (!populated_zone(zone))
+			continue;
What's this? Performance tweak? Or does min_wmark_pages() return non-zero for an unpopulated zone, which seems odd.

Minor performance tweak. It's a force of habit to skip populated zones when doing a zone walk like this.

Quoted 38 lines

Quoted 25 lines

mm/vmscan.c +6 -2

 		pfmemalloc_reserve += min_wmark_pages(zone);
 		free_pages += zone_page_state(zone, NR_FREE_PAGES);
 	}
 
+	/* If there are no reserves (unexpected config) then do not throttle */
+	if (!pfmemalloc_reserve)
+		return true;
+
 	wmark_ok = free_pages > pfmemalloc_reserve / 2;
 
 	/* kswapd must be awake if processes are being throttled */

@@ -2535,9 +2542,9 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
 static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
+	struct zoneref *z;
 	struct zone *zone;
-	int high_zoneidx = gfp_zone(gfp_mask);
-	pg_data_t *pgdat;
+	pg_data_t *pgdat = NULL;
 
 	/*
 	 * Kernel threads should not be throttled as they may be indirectly

@@ -2556,10 +2563,24 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (fatal_signal_pending(current))

mm/vmscan.c static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2569

Quoted 10 lines

mm/vmscan.c +4 -4

 		goto out;
 
-	/* Check if the pfmemalloc reserves are ok */
-	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
-	pgdat = zone->zone_pgdat;
-	if (pfmemalloc_watermark_ok(pgdat))
+	/*
+	 * Check if the pfmemalloc reserves are ok by finding the first node
+	 * with a usable ZONE_NORMAL or lower zone
+	 */

That comment tells us what the code does but not why it does it.

Quoted 5 lines

- Why do we ignore zones >= ZONE_NORMAL?
- Why do we throttle when there may be as-yet-unexamined nodes which have reclaimable pages?

/*
* Check if the pfmemalloc reserves are ok by finding the first node
* with a usable ZONE_NORMAL or lower zone. The expectation is that
* GFP_KERNEL will be required for allocating network buffers when
* swapping over the network so ZONE_HIGHMEM is unusable.
*
* Throttling is based on the first usable node and throttled processes
* wait on a queue until kswapd makes progress and wakes them. There
* is an affinity then between processes waking up and where reclaim
* progress has been made assuming the process wakes on the same node.
* More importantly, processes running on remote nodes will not compete
* for remote pfmemalloc reserves and processes on different nodes
* should make reasonable progress.
*/

Quoted 6 lines

mm/vmscan.c static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2571
Quoted 3 lines
mm/vmscan.c +2 -0
+        for_each_zone_zonelist_nodemask(zone, z, zonelist,
+                                        gfp_mask, nodemask) {
Those two lines have spaces-instead-of-tabs.

Sorry, that was careless.

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>