modified memory_pressure calculation

I think the current memory_pressure calculation is broken - at least memory_pressure does not contain the number of pages necessary in the inactive_clean_list to handle 1 second of allocations.

* if reclaim_page() finds a page that is Referenced, Dirty or Locked then it must increase memory_pressure.
* I don't understand the purpose of the second ++ in alloc_pages().

What about the attached patch [vs. 2.4.5]? It's just an idea, untested.

If the behaviour is worse then we must figure out what memory_pressure actually is under the various workloads. AFAICS it has nothing to do with the number of memory allocations per second.

Please cc me, I'm not subscribed to linux-mm.

--
Manfred

diff -u 2.4/mm/page_alloc.c build-2.4/mm/page_alloc.c
@@ -141,8 +141,11 @@
 	 * since it's nothing important, but we do want to make sure
 	 * it never gets negative.
 	 */
-	if (memory_pressure > NR_CPUS)
-		memory_pressure--;
+	{
+		int mp = memory_pressure-(1<<order);
+		if (mp > 0)
+			memory_pressure = mp;
+	}
 }
 
 #define MARK_USED(index, order, area) \

@@ -282,7 +285,7 @@
 	/*
 	 * Allocations put pressure on the VM subsystem.
 	 */
-	memory_pressure++;
+	memory_pressure += (1<<order);
 
 	/*
 	 * (If anyone calls gfp from interrupts nonatomically then it

@@ -437,7 +440,6 @@
 		 * 	  the inactive clean list. (done by page_launder)
 		 */
 		if (gfp_mask & __GFP_WAIT) {
-			memory_pressure++;
 			try_to_free_pages(gfp_mask);
 			goto try_again;
 		}

@@ -476,7 +478,7 @@
 
 		/* XXX: is pages_min/4 a good amount to reserve for this? */
 		if (z->free_pages < z->pages_min / 4 &&
-				!(current->flags & PF_MEMALLOC))
+			(in_interrupt() || !(current->flags & PF_MEMALLOC)))
 			continue;
 		page = rmqueue(z, order);
 		if (page)

diff -u 2.4/mm/swap.c build-2.4/mm/swap.c
@@ -46,8 +46,19 @@
  * is doing, averaged over a minute. We use this to determine how
  * many inactive pages we should have.
  *
- * In reclaim_page and __alloc_pages: memory_pressure++
+ * In __alloc_pages: memory_pressure++
+ * 	each allocation uses memory
  * In __free_pages_ok: memory_pressure--
+ * kreclaimd: memory_pressure++ before __free_pages_ok
+ * 	A memory free from outside of the {in,}active_list
+ * 	reduces the necessary number of freeable pages in the
+ * 	inactive_clean_list.
+ * in reclaim_pages: memory_pressure++ for each unfreeable page found
+ *	 	in the inactive_clean_list.
+ *	 Pages in the inactive_clean_list can be reused by the current
+ *	 owner. This "increases" the memory pressure since more pages
+ *	 must be in the inactive_clean_list to have a certain number of
+ *	 freeable pages in the inactive_clean_list.
  * In recalculate_vm_stats the value is decayed (once a second)
  */
 int memory_pressure;

diff -u 2.4/mm/vmscan.c build-2.4/mm/vmscan.c
@@ -355,6 +355,7 @@
 			printk("VM: reclaim_page, wrong page on list.\n");
 			list_del(page_lru);
 			page->zone->inactive_clean_pages--;
+			memory_pressure++;
 			continue;
 		}
 

@@ -363,6 +364,7 @@
 				(!page->buffers && page_count(page) > 1)) {
 			del_page_from_inactive_clean_list(page);
 			add_page_to_active_list(page);
+			memory_pressure++;
 			continue;
 		}
 

@@ -370,6 +372,7 @@
 		if (page->buffers || PageDirty(page) || TryLockPage(page)) {
 			del_page_from_inactive_clean_list(page);
 			add_page_to_inactive_dirty_list(page);
+			memory_pressure++;
 			continue;
 		}
 

@@ -389,6 +392,7 @@
 		list_del(page_lru);
 		zone->inactive_clean_pages--;
 		UnlockPage(page);
+		memory_pressure++;
 	}
 	/* Reset page pointer, maybe we encountered an unfreeable page. */
 	page = NULL;

@@ -404,7 +408,6 @@
 out:
 	spin_unlock(&pagemap_lru_lock);
 	spin_unlock(&pagecache_lock);
-	memory_pressure++;
 	return page;
 }
 

@@ -1046,6 +1049,14 @@
 					page = reclaim_page(zone);
 					if (!page)
 						break;
+					/* We move pages from the inactive_clean_list
+					 * into the buddy. This doesn't cause any change
+					 * in the actual memory pressure.
+					 * The 'memory_pressure++' is 
+					 * required to undo the 'memory_pressure--'
+					 * in __free_pages_ok.
+					 */
+					memory_pressure++;
 					__free_page(page);
 				}
 			}

Why ?

It's broken and should be removed. Thanks for spotting this one.

Just remove the in_interrupt() check near PF_MEMALLOC, will you?

Adding that check makes it possible for a pingflood to deadlock kswapd, as the network card can allocate the very last pages in the system and kswapd needs those pages to free up memory.

regards,

Rik
--
Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose...

http://www.surriel.com/ http://distro.conectiva.com/

Send all your spam to [email protected] (spam digging piggy)

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

Because it means that a page that was not really unused was detected by reclaim_page.
It's not really an increase of the memory pressure, it's a decrease of the efficiency of the inactive_clean_list.

I'll think about it again.

Of course.
I forgot to remove that line, it's unrelated to the memory_pressure calculation. But I think it's another problem I spotted.

Are you sure? It should be the other way around:

It's 'if (in_interrupt()) continue', not 'if (in_interrupt()) alloc'. Currently a network card can allocate the last few pages if the interrupt occurs in the context of the PF_MEMALLOC thread. I think PF_MEMALLOC memory should never be available to interrupt handlers.

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

You're right, my mistake.

Rik
--
Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose...

http://www.surriel.com/ http://distro.conectiva.com/

Send all your spam to [email protected] (spam digging piggy)

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

Ok, then the attached patch should be ok [SMP safe 'memory_pressure--' + the change above].

I've moved the modified memory_pressure calculation into my 'not_now' folder - not enough time for proper testing, and the change definitively needs thorough testing.

We should take into account that the current page owner can reactivage a page, i.e. nr_inactive_{dirty,clean}_pages overestimates the number of really inactive pages in these lists.

My modified memory_pressure calculation would be one way to implement that.

--
Manfred

diff -u 2.4/mm/page_alloc.c build-2.4/mm/page_alloc.c
@@ -141,8 +141,11 @@
 	 * since it's nothing important, but we do want to make sure
 	 * it never gets negative.
 	 */
-	if (memory_pressure > NR_CPUS)
-		memory_pressure--;
+	{
+		int mp = memory_pressure-1;
+		if (mp > 0)
+			memory_pressure = mp;
+	}
 }
 
 #define MARK_USED(index, order, area) \

@@ -476,7 +479,7 @@
 
 		/* XXX: is pages_min/4 a good amount to reserve for this? */
 		if (z->free_pages < z->pages_min / 4 &&
-				!(current->flags & PF_MEMALLOC))
+			(in_interrupt() || !(current->flags & PF_MEMALLOC)))
 			continue;
 		page = rmqueue(z, order);
 		if (page)

Looks good. Could you send it to Linus when he comes back from Japan?

Rik
--
Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose...

http://www.surriel.com/ http://distro.conectiva.com/

Send all your spam to [email protected] (spam digging piggy)

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

Pasting a part of your patch:

@@ -363,6 +364,7 @@

                                (!page->buffers && page_count(page) >
1)) {
                        del_page_from_inactive_clean_list(page);
                        add_page_to_active_list(page);
+                       memory_pressure++;
                        continue;
                }

@@ -370,6 +372,7 @@

                if (page->buffers || PageDirty(page) || TryLockPage(page)) {
                        del_page_from_inactive_clean_list(page);
                        add_page_to_inactive_dirty_list(page);
+                       memory_pressure++;
                        continue;
                }

I disagree with the second hunk.

memory_pressure is used to calculate the size of _both_ the inactive dirty and clean lists.

Since you're adding the page back to the inactive dirty list, you should not increase memory_pressure.

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

Correct. And page_launder should increase memory_pressure fore each page it moves back into the active list.

But I don't like the overloading of memory_pressure. Initially I thought that one ++ is missing.

I think an additional statistic variable would be better. [reactivation_rate, or whatever].

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

With the current VM code we can reach a high deactivation rate under most heavy losts. Way too high, actually.

With a lot of heavy allocator(s), the deactivation target will become high pretty fast, and tasks will have their pages freed way too fast.

Allocators which try to free pages themselves by calling try_to_free_pages (in __alloc_pages()) are aging the active pages, and if we have lots of them doing that, we have a problem.

It looks correct to make page_launder() increase memory_pressure for each page moved to the active list, but then the problem I described above will become even worse.

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