[PATCH] modified memory_pressure calculation

[PATCH] modified memory_pressure calculation

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

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

[-- Attachment #2: patch-memory_pressure --]
[-- Type: text/plain, Size: 3727 bytes --]

diff -u 2.4/mm/page_alloc.c build-2.4/mm/page_alloc.c
--- 2.4/mm/page_alloc.c	Sat May 26 10:06:29 2001
+++ build-2.4/mm/page_alloc.c	Sun May 27 14:18:35 2001
@@ -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
--- 2.4/mm/swap.c	Mon Jan 22 22:30:21 2001
+++ build-2.4/mm/swap.c	Sun May 27 14:24:29 2001
@@ -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
--- 2.4/mm/vmscan.c	Sat May 26 10:06:29 2001
+++ build-2.4/mm/vmscan.c	Sun May 27 14:27:23 2001
@@ -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);
 				}
 			}

Re: [PATCH] modified memory_pressure calculation

[Rik van Riel]

On Sun, 27 May 2001, Manfred Spraul wrote:

> * if reclaim_page() finds a page that is Referenced, Dirty or Locked
>   then it must increase memory_pressure.

Why ?

> * I don't understand the purpose of the second ++ in alloc_pages().

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

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

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 aardvark@nl.linux.org (spam digging piggy)

--
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-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Manfred Spraul]

Rik van Riel wrote:
> 
> On Sun, 27 May 2001, Manfred Spraul wrote:
> 
> > * if reclaim_page() finds a page that is Referenced, Dirty or Locked
> >   then it must increase memory_pressure.
> 
> Why ?
>
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.

> > * I don't understand the purpose of the second ++ in alloc_pages().
> 
> It's broken and should be removed. Thanks for spotting
> this one.
> 
> > What about the attached patch [vs. 2.4.5]? It's just an idea, untested.
> 
> Just remove the in_interrupt() check near PF_MEMALLOC, will you?
>
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.

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

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

>          if (z->free_pages < z->pages_min / 4 &&
> -           !(current->flags & PF_MEMALLOC))
> +            (in_interrupt() || !(current->flags & PF_MEMALLOC)))
>		continue;

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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Rik van Riel]

On Sun, 27 May 2001, Manfred Spraul wrote:

> >          if (z->free_pages < z->pages_min / 4 &&
> > -           !(current->flags & PF_MEMALLOC))
> > +            (in_interrupt() || !(current->flags & PF_MEMALLOC)))
> >		continue;
> 
> 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.

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 aardvark@nl.linux.org (spam digging piggy)

--
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-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Rik van Riel wrote:
> 
> On Sun, 27 May 2001, Manfred Spraul wrote:
> 
> > >          if (z->free_pages < z->pages_min / 4 &&
> > > -           !(current->flags & PF_MEMALLOC))
> > > +            (in_interrupt() || !(current->flags & PF_MEMALLOC)))
> > >             continue;
> >
> > 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.
> 
> You're right, my mistake.
>
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

[-- Attachment #2: patch-PF --]
[-- Type: text/plain, Size: 738 bytes --]

diff -u 2.4/mm/page_alloc.c build-2.4/mm/page_alloc.c
--- 2.4/mm/page_alloc.c	Sat May 26 10:06:29 2001
+++ build-2.4/mm/page_alloc.c	Sun May 27 20:12:23 2001
@@ -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)

Re: [PATCH] modified memory_pressure calculation

[Rik van Riel]

On Sun, 27 May 2001, Manfred Spraul wrote:

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

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 aardvark@nl.linux.org (spam digging piggy)

--
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-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Marcelo Tosatti]

On Sun, 27 May 2001, Manfred Spraul wrote:

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

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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Manfred Spraul]

Marcelo Tosatti wrote:
> 
> 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.
>

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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [PATCH] modified memory_pressure calculation

[Marcelo Tosatti]

On Mon, 28 May 2001, Manfred Spraul wrote:

> Marcelo Tosatti wrote:
> > 
> > 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.
> >
> 
> Correct. And page_launder should increase memory_pressure fore each page
> it moves back into the active list.

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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/