Fix bug in try_to_free_pages and balance_pgdat when they fail to reclaim pages

The same bug is contained in both try_to_free_pages and balance_pgdat. On reclaiming the requisite number of pages we correctly set prev_priority back to DEF_PRIORITY.

However, we ALSO do this even
if we loop over all priorities and fail to reclaim.

Setting prev_priority artificially high causes reclaimers to set distress artificially low, and fail to reclaim mapped pages, when they are, in fact, under severe memory pressure (their priority may be as low as 0). This causes the OOM killer to fire incorrectly.

This patch changes that to set prev_priority to 0 instead, if we fail to reclaim.

Signed-off-by Martin J. Bligh <[email protected]>

------------------------------------------------------------------------

diff -aurpN -X /home/mbligh/.diff.exclude linux-2.6.18/mm/vmscan.c 2.6.18-prev_reset/mm/vmscan.c

mm/vmscan.c +11 -6

@@ -962,7 +962,6 @@ static unsigned long shrink_zones(int pr
 unsigned long try_to_free_pages(struct zone **zones, gfp_t gfp_mask)
 {
 	int priority;
-	int ret = 0;
 	unsigned long total_scanned = 0;
 	unsigned long nr_reclaimed = 0;
 	struct reclaim_state *reclaim_state = current->reclaim_state;

@@ -1000,8 +999,15 @@ unsigned long try_to_free_pages(struct z
 		}
 		total_scanned += sc.nr_scanned;
 		if (nr_reclaimed >= sc.swap_cluster_max) {
-			ret = 1;
-			goto out;
+			for (i = 0; zones[i] != 0; i++) {
+				struct zone *zone = zones[i];
+
+				if (!cpuset_zone_allowed(zone, __GFP_HARDWALL))
+					continue;
+
+				zone->prev_priority = zone->temp_priority;
+			}
+			return 1;
 		}
 
 		/*

@@ -1021,16 +1027,15 @@ unsigned long try_to_free_pages(struct z
 		if (sc.nr_scanned && priority < DEF_PRIORITY - 2)
 			blk_congestion_wait(WRITE, HZ/10);
 	}
-out:
 	for (i = 0; zones[i] != 0; i++) {
 		struct zone *zone = zones[i];
 
 		if (!cpuset_zone_allowed(zone, __GFP_HARDWALL))
 			continue;
 
-		zone->prev_priority = zone->temp_priority;
+		zone->prev_priority = 0;
 	}
-	return ret;
+	return 0;
 }
 
 /*

@@ -1186,7 +1191,10 @@ out:
 	for (i = 0; i < pgdat->nr_zones; i++) {

Quoted 3 lines · Martin Bligh

The same bug is contained in both try_to_free_pages and balance_pgdat. On reclaiming the requisite number of pages we correctly set prev_priority back to DEF_PRIORITY.

AFAIKS, we set prev_priority to the priority at which the zone was deemed to require no more reclaiming, not DEF_PRIORITY.

Quoted 2 lines

However, we ALSO do this even
if we loop over all priorities and fail to reclaim.

If that happens, shouldn't prev_priority be set to 0?

I don't agree the patch is correct.

We saw problems with this before releasing SLES10 too. See zone_is_near_oom and other changesets from around that era. I would like to know what workload was prevented from going OOM with these changes, but zone_is_near_oom didn't help -- it must have been very marginal (or there may indeed be a bug somewhere).

Quoted 8 lines · Nick Piggin

Quoted 3 lines · Martin Bligh

The same bug is contained in both try_to_free_pages and balance_pgdat. On reclaiming the requisite number of pages we correctly set prev_priority back to DEF_PRIORITY.

Quoted 2 lines

AFAIKS, we set prev_priority to the priority at which the zone was deemed to require no more reclaiming, not DEF_PRIORITY.

Well, it's zone->temp_priority, which was set to DEF_PRIORITY at the top of the function, though I suppose something else might have changed it since.

Quoted 7 lines

Quoted 3 lines

However, we ALSO do this even
if we loop over all priorities and fail to reclaim.

If that happens, shouldn't prev_priority be set to 0?

Yes, but it's not. We fall off the bottom of the loop, and set it back to temp_priority. At best, the code is unclear.

I suppose shrink_zones() might in theory knock temp_priority down as it goes, so it might come out right. But given that it's a global (per zone), not per-reclaimer, I fail to see how that's really safe. Supposing someone else has just started reclaim, and is still at prio 12?

Moreover, whilst try_to_free_pages calls shrink_zones, balance_pgdat does not. Nothing else I can see sets temp_priority.

Quoted 2 lines

I don't agree the patch is correct.

You think it's doing something wrong? Or just unnecessary?

I'm inclined to think the whole concept of temp_priority and prev_priority are pretty broken. This may not fix the whole thing, but it seems to me to make it better than it was before.

Quoted 5 lines

We saw problems with this before releasing SLES10 too. See zone_is_near_oom and other changesets from around that era. I would like to know what workload was prevented from going OOM with these changes, but zone_is_near_oom didn't help -- it must have been very marginal (or there may indeed be a bug somewhere).

Google production workload. Multiple reclaimers operating - one is down to priority 0 on the reclaim, but distress is still set to 0, thanks to prev_priority being borked. Hence we don't reclaim mapped pages, the reclaim fails, OOM killer kicks in.

Forward ported from an earlier version of 2.6 ... but I don't see why we need extra heuristics here, it seems like a clear and fairly simple bug. We're in deep crap with reclaim, and we go set the global indicator back to "oh no, everything's fine". Not a good plan.

Quoted 3 lines

Well, it's zone->temp_priority, which was set to DEF_PRIORITY at the top of the function, though I suppose something else might have changed it since.

Yes.

Quoted 4 lines

Quoted 1 line

If that happens, shouldn't prev_priority be set to 0?

Yes, but it's not. We fall off the bottom of the loop, and set it back to temp_priority. At best, the code is unclear.

But temp_priority should be set to 0 at that point.

Quoted 5 lines

I suppose shrink_zones() might in theory knock temp_priority down as it goes, so it might come out right. But given that it's a global (per zone), not per-reclaimer, I fail to see how that's really safe. Supposing someone else has just started reclaim, and is still at prio 12?

But your loops are not exactly per reclaimer either. Granted there is a large race window in the current code, but this patch isn't the way to fix that particular problem.

Quoted 2 lines

Moreover, whilst try_to_free_pages calls shrink_zones, balance_pgdat does not. Nothing else I can see sets temp_priority.

balance_pgdat.

Unnecesary and indicates something else is broken if you are seeing problems here.

Quoted 3 lines

I'm inclined to think the whole concept of temp_priority and prev_priority are pretty broken. This may not fix the whole thing, but it seems to me to make it better than it was before.

I think it is broken too. I liked my split active lists, but at that point vmscan.c was in don't-touch mode.

OK, so it sounds like temp_priority is being overwritten by the race. I'd consider throwing out temp_priority completely, and just going with adjusting prev_priority as we go.

Quoted 4 lines

Forward ported from an earlier version of 2.6 ... but I don't see why we need extra heuristics here, it seems like a clear and fairly simple bug. We're in deep crap with reclaim, and we go set the global indicator back to "oh no, everything's fine". Not a good plan.

All that reclaim_mapped code is pretty arbitrary anyway. What is needed is the zone_is_near_oom so we can decouple all those heuristics from the OOM decision.

So do you still see the problem on upstream kernel without your patches applied?

Quoted 1 line

But temp_priority should be set to 0 at that point.

It that were true, it'd be great. But how?
This is everything that touches it:

0 mmzone.h <global> 208 int temp_priority;
1 page_alloc.c free_area_init_core 2019 zone->temp_priority =

                                        zone->prev_priority = DEF_PRIORITY;
2 vmscan.c     shrink_zones         937 zone->temp_priority = priority;
3 vmscan.c     try_to_free_pages    987 zone->temp_priority = DEF_PRIORITY;
4 vmscan.c     try_to_free_pages   1031 zone->prev_priority =
                                        zone->temp_priority;
5 vmscan.c     balance_pgdat       1081 zone->temp_priority = DEF_PRIORITY;
6 vmscan.c     balance_pgdat       1143 zone->temp_priority = priority;
7 vmscan.c     balance_pgdat       1189 zone->prev_priority =
                                        zone->temp_priority;

8 vmstat.c zoneinfo_show 593 zone->temp_priority,

Only thing that looks interesting here is shrink_zones.

Quoted 3 lines

But your loops are not exactly per reclaimer either. Granted there is a large race window in the current code, but this patch isn't the way to fix that particular problem.

Why not? Perhaps it's not a panacea, but it's a definite improvement.

Quoted 6 lines

Quoted 3 lines

Moreover, whilst try_to_free_pages calls shrink_zones, balance_pgdat does not. Nothing else I can see sets temp_priority.

balance_pgdat.

That's only called from kswapd. If we're in balance_pgdat, we ARE kswapd. We can't fix ourself. So effectively we're doing:

while (priority--) {
if (we reclaimed OK)
goto out;
}
out:
prev_priority = DEF_PRIORITY;

We've just walked the whole bloody list with priority set to 0.

We failed to reclaim a few pages.

We know the world is in deep pain.

Why the hell would we elevate prev_priority?

Quoted 2 lines

Unnecesary and indicates something else is broken if you are seeing problems here.

You think we should set prev_priority up, when we've just walked the whole list at prio 0 and can't reclaim anything? Unless so, I fail to see how the patch is unnecessary.

And yes, I'm sure other things are broken, but again, this fixes a clear bug.

Quoted 3 lines

So do you still see the problem on upstream kernel

without your patches applied?

I can't slap an upstream bleeding edge kernel across a few thousand production machines, and wait to see if the world blows up, sorry. If I can make a reproduce test case, I'll send it out, but thus far we've been unsuccessful.

But I can see it happening in earlier versions, and I can read the code in 2.6.18, and see obvious bugs.