[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Subtle MM bug
On Mon, 8 Jan 2001, Stephen C. Tweedie wrote:
> > _really_ well on many loads, but this one we do badly on. And from what
> > I've been able to see so far, it's because we're just too damn good at
> > waiting on page_launder() and doing refill_inactive_scan().
>
> do_try_to_free_pages() is trying to
>
> /*
> * If needed, we move pages from the active list
> * to the inactive list. We also "eat" pages from
> * the inode and dentry cache whenever we do this.
> */
> if (free_shortage() || inactive_shortage()) {
> shrink_dcache_memory(6, gfp_mask);
> shrink_icache_memory(6, gfp_mask);
> ret += refill_inactive(gfp_mask, user);
> } else {
>
> So we're refilling the inactive list regardless of its current size
> whenever free_shortage() is true. In the situation you describe,
> there's no point refilling the inactive list too far beyond the
> ability of the swapper to launder it, regardless of whether
> free_shortage() is set.
Agreed.
After some fights me and Rik agreed on doing a per-zone inactive shortage
check in inactive_shortage().
This allow us to check _only_ for inactive_shortage() before calling
refill_inactive().
>
> refill_inactive contains exactly the opposite logic: it breaks out if
>
> /*
> * If we either have enough free memory, or if
> * page_launder() will be able to make enough
> * free memory, then stop.
> */
> if (!inactive_shortage() || !free_shortage())
> goto done;
>
> but that still means that we're doing unnecessary inactive list
> refilling whenever free_shortage() is true: this test only occurs
> after we've tried at least one swap_out(). We're calling
> refill_inactive if either condition is true, but we're staying inside
> it only if both conditions are true.
>
> Shouldn't we really just be making the refill_inactive() here depend
> on inactive_shortage() alone, not free_shortage()? By refilling the
> inactive list too agressively we actually end up discarding aging
> information which might be of use to us.
Yes.
I've removed the free_shortage() of refill_inactive() in the patch.
Comments are welcome.
--- linux.orig/mm/vmscan.c Thu Jan 4 02:45:26 2001
+++ linux/mm/vmscan.c Mon Jan 8 20:43:59 2001
@@ -808,6 +808,9 @@
int inactive_shortage(void)
{
int shortage = 0;
+ pg_data_t *pgdat = pgdat_list;
+
+ /* Is the inactive dirty list too small? */
shortage += freepages.high;
shortage += inactive_target;
@@ -818,7 +821,27 @@
if (shortage > 0)
return shortage;
- return 0;
+ /* If not, do we have enough per-zone pages on the inactive list? */
+
+ shortage = 0;
+
+ do {
+ int i;
+ for(i = 0; i < MAX_NR_ZONES; i++) {
+ int zone_shortage;
+ zone_t *zone = pgdat->node_zones+ i;
+
+ zone_shortage = zone->pages_high;
+ zone_shortage -= zone->inactive_dirty_pages;
+ zone_shortage -= zone->inactive_clean_pages;
+ zone_shortage -= zone->free_pages;
+ if (zone_shortage > 0)
+ shortage += zone_shortage;
+ }
+ pgdat = pgdat->node_next;
+ } while (pgdat);
+
+ return shortage;
}
/*
@@ -861,12 +884,13 @@
}
/*
- * don't be too light against the d/i cache since
- * refill_inactive() almost never fail when there's
- * really plenty of memory free.
+ * Only free memory from i/d caches if we have
+ * are under low memory.
*/
- shrink_dcache_memory(priority, gfp_mask);
- shrink_icache_memory(priority, gfp_mask);
+ if(free_shortage()) {
+ shrink_dcache_memory(priority, gfp_mask);
+ shrink_icache_memory(priority, gfp_mask);
+ }
/*
* Then, try to page stuff out..
@@ -878,11 +902,10 @@
}
/*
- * If we either have enough free memory, or if
- * page_launder() will be able to make enough
+ * If page_launder() will be able to make enough
* free memory, then stop.
*/
- if (!inactive_shortage() || !free_shortage())
+ if (!inactive_shortage())
goto done;
/*
@@ -922,14 +945,20 @@
/*
* If needed, we move pages from the active list
- * to the inactive list. We also "eat" pages from
- * the inode and dentry cache whenever we do this.
+ * to the inactive list.
+ */
+ if (inactive_shortage())
+ ret += refill_inactive(gfp_mask, user);
+
+ /*
+ * Delete pages from the inode and dentry cache
+ * if memory is low.
*/
- if (free_shortage() || inactive_shortage()) {
+ if (free_shortage()) {
shrink_dcache_memory(6, gfp_mask);
shrink_icache_memory(6, gfp_mask);
- ret += refill_inactive(gfp_mask, user);
- } else {
+ } else {
+
/*
* Reclaim unused slab cache memory.
*/
--
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.eu.org/Linux-MM/