[PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Richard Jerrell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1006 bytes --]

2.4.1 has a memory leak (temporary) where anonymous memory pages that have
been moved into the swap cache will stick around after their vma has been
unmapped by the owning process.  These pages are not free'd in free_pte()
because they are still referenced by the page cache.  In addition, if the
pages are dirty, they will be written out to the swap device before they
are reclaimed even though the owning process no longer will be using the
pages.

free_pte in mm/memory.c has been modified to check to see if the page is
only being referenced by the swap cache (and possibly buffers).  If so,
the buffers (if existant) are free'd and the page and swap cache
entry are removed immediately.

Essentially, this is the same patch as before, but there was one condition
in which case we would leak and extra reference to the targeted page if
the counts would not allow us to remove the swap cache entry.  The leak in
2.4.1 also applies to 2.4.2 and 2.4.3-pre5.

Rich Jerrell
jerrell@missioncriticallinux.com

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1499 bytes --]

diff --recursive -u -N linux-2.4.1/mm/memory.c linux-2.4.1-paging-fix/mm/memory.c
--- linux-2.4.1/mm/memory.c	Sat Jan 27 22:12:35 2001
+++ linux-2.4.1-paging-fix/mm/memory.c	Thu Feb 15 13:36:06 2001
@@ -281,6 +285,34 @@
 		return 1;
 	}
 	swap_free(pte_to_swp_entry(pte));
+	{
+		int num, target = 1;
+		struct page *page = lookup_swap_cache(pte_to_swp_entry(pte));
+		                    /* returns the page and takes a reference */
+		
+		if (!page || (!VALID_PAGE(page)) || PageReserved(page))
+			return 0;
+		
+		num = atomic_read(&page->count);
+		if(page->buffers) target++;		/* 1 count if we have buffers */
+		if(PageSwapCache(page)) target++;	/* 1 count for the page cache */
+							/* 1 count for our reference  */
+
+		if((num == target) && PageSwapCache(page)) {
+			/* SwapCache entry is the only thing referencing this page   */
+			/* (and maybe buffers) asides from us, so to prevent it from */
+			/* sitting around and wasting time/memory, throw it away     */
+			if((page->buffers)) {
+				if(!try_to_free_buffers(page,1)) {	/* Can't get rid of buffers   */
+					page_cache_release(page);	/* get rid of our reference   */
+					return 0;			/* and let someone else do it */
+				}
+			}
+			free_page_and_swap_cache(page);	/* Expects the page to be mapped, so will */
+			return 1;			/* account for the reference we have      */
+		}
+		page_cache_release(page);       /* Remove our reference, we can't do anything */
+	}
 	return 0;
 }
 

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Rik van Riel]

On Thu, 22 Mar 2001, Richard Jerrell wrote:

> 2.4.1 has a memory leak (temporary) where anonymous memory pages
> that have been moved into the swap cache will stick around after
> their vma has been unmapped by the owning process.

> free_pte in mm/memory.c has been modified to check to see if the
> page is only being referenced by the swap cache

Your idea is nice, but the patch lacks a few things:

- SMP locking, what if some other process faults in this page
  between the atomic_read of the page count and the test later?
- testing if our process is the _only_ user of this swap page,
  for eg. apache you'll have lots of COW-shared pages .. it would
  be good to keep the page in memory for our siblings

regards,

Rik
--
Linux MM bugzilla: http://linux-mm.org/bugzilla.shtml

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

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

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Rik van Riel]

On Fri, 23 Mar 2001, Richard Jerrell wrote:
> > Your idea is nice, but the patch lacks a few things:
> > 
> > - SMP locking, what if some other process faults in this page
> >   between the atomic_read of the page count and the test later?
> 
> It can't happen.  free_pte is called with the page_table_lock held in 
> addition to having the mmap_sem downed.

The page_table_lock and the mmap_sem only protect the *current*
task. Think about something like an apache with 500 children who
COW share the same page...

> > - testing if our process is the _only_ user of this swap page,
> >   for eg. apache you'll have lots of COW-shared pages .. it would
> >   be good to keep the page in memory for our siblings
> 
> This is already done in free_page_and_swap_cache.

Ok ...

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://www.conectiva.com/	http://distro.conectiva.com.br/

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Richard Jerrell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 919 bytes --]

> Your idea is nice, but the patch lacks a few things:
> 
> - SMP locking, what if some other process faults in this page
>   between the atomic_read of the page count and the test later?

It can't happen.  free_pte is called with the page_table_lock held in 
addition to having the mmap_sem downed.

> - testing if our process is the _only_ user of this swap page,
>   for eg. apache you'll have lots of COW-shared pages .. it would
>   be good to keep the page in memory for our siblings

This is already done in free_page_and_swap_cache.

There is a problem with a possible kernel panic in that
try_to_free_buffers is called with a wait of 1 (thanks to Andrew Morton
for pointing that out) and we might reschedule while we wait on io.  So,
to fix it, here is an even newer (and simpler) patch.  Everything is
handled in free_page_and_swap_cache, so we just call that if we can
successfully look up the entry.

Rich


[-- Attachment #2: Type: TEXT/PLAIN, Size: 711 bytes --]

diff --recursive -u -N linux-2.4.1/mm/memory.c linux-2.4.1-paging-fix/mm/memory.c
--- linux-2.4.1/mm/memory.c	Sat Jan 27 22:12:35 2001
+++ linux-2.4.1-paging-fix/mm/memory.c	Thu Feb 15 13:36:06 2001
@@ -281,7 +281,16 @@
 		return 1;
 	}
 	swap_free(pte_to_swp_entry(pte));
-	return 0;
+	{
+		struct page *page = lookup_swap_cache(pte_to_swp_entry(pte));
+		/* returns the page and takes a reference */
+
+		if (!page || (!VALID_PAGE(page)) || PageReserved(page))
+		return 0;
+
+		free_page_and_swap_cache(page); /* Expects the page to be mapped, so will */
+		return 1;                       /* account for the reference we have      */
+	}
 }
 
 static inline void forget_pte(pte_t page)

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Stephen Tweedie]

Hi,

On Thu, Mar 22, 2001 at 05:21:46PM -0500, Richard Jerrell wrote:
> 2.4.1 has a memory leak (temporary) where anonymous memory pages that have
> been moved into the swap cache will stick around after their vma has been
> unmapped by the owning process.  These pages are not free'd in free_pte()
> because they are still referenced by the page cache.  In addition, if the
> pages are dirty, they will be written out to the swap device before they
> are reclaimed even though the owning process no longer will be using the
> pages.
> 
> free_pte in mm/memory.c has been modified to check to see if the page is
> only being referenced by the swap cache (and possibly buffers).

But is it worth it?

fork and exit are very hot paths in the kernel, and this patch can force
a page cache lookup on a large number of pte which wouldn't be looked
up before.

The classic case is sendmail or apache, where you can have a parent
process rapidly forking a large number of children.  If part of the
parent gets swapped out due to lack of use, then the children all
inherit swapped ptes and each such page will result in an extra page
cache lookup in zap_page_range on exit with this change.

Given that the leak is, as you say, temporary, and that the leak will
be recovered as soon as we start swapping again, do we really want to
pollute the fast path for the sake of a bit more speed during
swapping?

Cheers,
 Stephen

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Rik van Riel]

On Tue, 27 Mar 2001, Richard Jerrell wrote:

> Instead of removing the swap cache page at process exit and possibly
> expending time doing disk IO as you have pointed out, we check during
> refill_inactive_scan and page_launder for a page that is

Three comments:

1. we take an extra reference on the page, how does that
   affect the test for if the page is shared or not ?
2. we call delete_from_swap_cache with the pagemap_lru_lock
   held, since this tries to grab the pagecache_lock we can
   easily deadlock with the rest of the kernel (where the
   locking order is opposite)
3. there are no comments in the code explaining what this
   suspicious-looking piece of code does ;)

regards,

Rik
--
Linux MM bugzilla: http://linux-mm.org/bugzilla.shtml

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

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

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Richard Jerrell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1901 bytes --]

> fork and exit are very hot paths in the kernel, and this patch can force
> a page cache lookup on a large number of pte which wouldn't be looked
> up before.

True, but I don't know how large of a performance hit the system takes.

> Given that the leak is, as you say, temporary, and that the leak will
> be recovered as soon as we start swapping again, do we really want to
> pollute the fast path for the sake of a bit more speed during
> swapping?

It isn't speed of swapping that is the biggest problem.  The problem is
that if you run a memory intensive task, exit after being placed on an
lru, and run it again, there won't be enough memory to execute because all
the memory you used previously is now sitting in the swap cache.  That
isn't to say that without being patched the speed isn't poor.  After all,
we'd be paging out a dead processes pages.

But you are right, this fix is slow and that can be improved.  So,
hopefully this patch is satisfactory in respect to speed and fixing the
leak.  And will also remove the panic which is possible with the other
patches (can't do a lookup_swap_cache with a spinlock held).

Instead of removing the swap cache page at process exit and possibly
expending time doing disk IO as you have pointed out, we check during
refill_inactive_scan and page_launder for a page that is 

1)  in the swap cache 
2)  is not locked 
3)  is only being referenced by the swap cache, us, and possibly by
    buffers
4)  has no one else referencing the swap cell

If that is true, we can safely remove that page without writing it to
disk.  In addition, the number of swap cache pages are included in the
amount returned from vm_enough_memory to get rid of the temporary leak.

So, the exit path remains unchanged, reclaiming a page is faster for when
the page is no longer being mapped, and the lazy reclaiming for multiply
referenced pages remains intact.

Rich

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2555 bytes --]

diff -rNu linux-2.4.1/include/linux/swap.h linux-2.4.1-paging-fix/include/linux/swap.h
--- linux-2.4.1/include/linux/swap.h	Tue Jan 30 02:24:56 2001
+++ linux-2.4.1-paging-fix/include/linux/swap.h	Tue Mar 27 10:43:22 2001
@@ -69,6 +69,7 @@
 FASTCALL(unsigned int nr_free_buffer_pages(void));
 extern int nr_active_pages;
 extern int nr_inactive_dirty_pages;
+extern int nr_swap_cache_pages;
 extern atomic_t nr_async_pages;
 extern struct address_space swapper_space;
 extern atomic_t page_cache_size;
diff -rNu linux-2.4.1/mm/mmap.c linux-2.4.1-paging-fix/mm/mmap.c
--- linux-2.4.1/mm/mmap.c	Mon Jan 29 11:10:41 2001
+++ linux-2.4.1-paging-fix/mm/mmap.c	Tue Mar 27 10:38:17 2001
@@ -63,6 +63,7 @@
 	free += atomic_read(&page_cache_size);
 	free += nr_free_pages();
 	free += nr_swap_pages;
+	free += nr_swap_cache_pages;
 	return free > pages;
 }
 
diff -rNu linux-2.4.1/mm/swap_state.c linux-2.4.1-paging-fix/mm/swap_state.c
--- linux-2.4.1/mm/swap_state.c	Fri Dec 29 18:04:27 2000
+++ linux-2.4.1-paging-fix/mm/swap_state.c	Tue Mar 27 10:41:04 2001
@@ -17,6 +17,8 @@
 
 #include <asm/pgtable.h>
 
+int nr_swap_cache_pages = 0;
+
 static int swap_writepage(struct page *page)
 {
 	rw_swap_page(WRITE, page, 0);
@@ -58,6 +60,7 @@
 #ifdef SWAP_CACHE_INFO
 	swap_cache_add_total++;
 #endif
+	nr_swap_cache_pages++;
 	if (!PageLocked(page))
 		BUG();
 	if (PageTestandSetSwapCache(page))
@@ -96,6 +99,7 @@
 #ifdef SWAP_CACHE_INFO
 	swap_cache_del_total++;
 #endif
+	nr_swap_cache_pages--;
 	remove_from_swap_cache(page);
 	swap_free(entry);
 }
diff -rNu linux-2.4.1/mm/vmscan.c linux-2.4.1-paging-fix/mm/vmscan.c
--- linux-2.4.1/mm/vmscan.c	Mon Jan 15 15:36:49 2001
+++ linux-2.4.1-paging-fix/mm/vmscan.c	Tue Mar 27 12:31:04 2001
@@ -466,6 +466,17 @@
 			continue;
 		}
 
+		if (PageSwapCache(page)) {
+			page_cache_get(page);
+			if(!is_page_shared(page)) {
+				delete_from_swap_cache_nolock(page);
+				UnlockPage(page);
+				page_cache_release(page);
+				continue;
+			}
+			page_cache_release(page);
+		}
+
 		/*
 		 * Dirty swap-cache page? Write it out if
 		 * last copy..
@@ -650,6 +661,17 @@
 			list_del(page_lru);
 			nr_active_pages--;
 			continue;
+		}
+
+		if (PageSwapCache(page)) {
+			page_cache_get(page);
+			if(!is_page_shared(page) && !TryLockPage(page)) {
+				delete_from_swap_cache_nolock(page);
+				UnlockPage(page);
+				page_cache_release(page);
+				continue;
+			}
+			page_cache_release(page);
 		}
 
 		/* Do aging on the pages. */

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Linus Torvalds]

On Tue, 27 Mar 2001, Richard Jerrell wrote:
>
> Instead of removing the swap cache page at process exit and possibly
> expending time doing disk IO as you have pointed out, we check during
> refill_inactive_scan and page_launder for a page that is

I think this patch looks pretty good.  However, I don't think you can
safely do a "is_shared()" query without holding the page lock. 

I'd be happy to be shown otherwise, of course.  I'm just generally very
wary of "is_shared()", and that function makes me nervous.  I'd almost
prefer to get rid of it, and test for the stuff it tests for directly
(most places that test this are likely to not need all the tests
anyway). 

I also have this suspicion that most of the advantage of this patch
could easily be gotten by just testing for the exclusive "no longer
used" case in the swap-cache "writepage()" function.  That would have
the advantage of localizing the test more, and minimizing special-case
swap-cache tests in the general VM codepaths. 

Comments?

                Linus

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Rik van Riel]

On Tue, 27 Mar 2001, Richard Jerrell wrote:

> Oops... I sent out the wrong version of the patch the first time.  
> This one has comments, promise.  And it has one less bug.  :)

Looks good to me (at first glance).  Any volunteer to
stress-test this on an SMP machine ?

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://www.conectiva.com/	http://distro.conectiva.com.br/

Re: [PATCH] mm/memory.c, 2.4.1 : memory leak with swap cache (updated)

[Richard Jerrell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 718 bytes --]

> 1. we take an extra reference on the page, how does that
>    affect the test for if the page is shared or not ?

is_page_shared expects us to have our own reference to the page.

> 2. we call delete_from_swap_cache with the pagemap_lru_lock
>    held, since this tries to grab the pagecache_lock we can
>    easily deadlock with the rest of the kernel (where the
>    locking order is opposite)

You're right.  Oversight on my part.  Here is another version of the
patch.

> 3. there are no comments in the code explaining what this
>    suspicious-looking piece of code does ;)

Oops... I sent out the wrong version of the patch the first time.  This
one has comments, promise.  And it has one less bug.  :)

Rich

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3485 bytes --]

diff -rNu linux-2.4.1/include/linux/swap.h linux-2.4.1-paging-fix/include/linux/swap.h
--- linux-2.4.1/include/linux/swap.h	Tue Jan 30 02:24:56 2001
+++ linux-2.4.1-paging-fix/include/linux/swap.h	Tue Mar 27 14:00:17 2001
@@ -69,6 +69,7 @@
 FASTCALL(unsigned int nr_free_buffer_pages(void));
 extern int nr_active_pages;
 extern int nr_inactive_dirty_pages;
+extern int nr_swap_cache_pages;
 extern atomic_t nr_async_pages;
 extern struct address_space swapper_space;
 extern atomic_t page_cache_size;
diff -rNu linux-2.4.1/mm/mmap.c linux-2.4.1-paging-fix/mm/mmap.c
--- linux-2.4.1/mm/mmap.c	Mon Jan 29 11:10:41 2001
+++ linux-2.4.1-paging-fix/mm/mmap.c	Tue Mar 27 10:38:17 2001
@@ -63,6 +63,7 @@
 	free += atomic_read(&page_cache_size);
 	free += nr_free_pages();
 	free += nr_swap_pages;
+	free += nr_swap_cache_pages;
 	return free > pages;
 }
 
diff -rNu linux-2.4.1/mm/swap_state.c linux-2.4.1-paging-fix/mm/swap_state.c
--- linux-2.4.1/mm/swap_state.c	Fri Dec 29 18:04:27 2000
+++ linux-2.4.1-paging-fix/mm/swap_state.c	Tue Mar 27 10:41:04 2001
@@ -17,6 +17,8 @@
 
 #include <asm/pgtable.h>
 
+int nr_swap_cache_pages = 0;
+
 static int swap_writepage(struct page *page)
 {
 	rw_swap_page(WRITE, page, 0);
@@ -58,6 +60,7 @@
 #ifdef SWAP_CACHE_INFO
 	swap_cache_add_total++;
 #endif
+	nr_swap_cache_pages++;
 	if (!PageLocked(page))
 		BUG();
 	if (PageTestandSetSwapCache(page))
@@ -96,6 +99,7 @@
 #ifdef SWAP_CACHE_INFO
 	swap_cache_del_total++;
 #endif
+	nr_swap_cache_pages--;
 	remove_from_swap_cache(page);
 	swap_free(entry);
 }
diff -rNu linux-2.4.1/mm/vmscan.c linux-2.4.1-paging-fix/mm/vmscan.c
--- linux-2.4.1/mm/vmscan.c	Mon Jan 15 15:36:49 2001
+++ linux-2.4.1-paging-fix/mm/vmscan.c	Tue Mar 27 14:37:18 2001
@@ -394,6 +394,21 @@
 	return page;
 }
 
+/** 
+ * Short-circuits the dead swap cache page to prevent
+ * unnecessary disk IO
+ */
+static inline void delete_dead_swap_cache_page(struct page *page) {
+	if (block_flushpage(page, 0))
+		lru_cache_del(page);
+	
+	ClearPageDirty(page);
+	spin_unlock(&pagemap_lru_lock);
+	__delete_from_swap_cache(page);
+	spin_lock(&pagemap_lru_lock);
+	page_cache_release(page);
+}
+
 /**
  * page_launder - clean dirty inactive pages, move to inactive_clean list
  * @gfp_mask: what operations we are allowed to do
@@ -467,6 +482,24 @@
 		}
 
 		/*
+		 * Prevent a dead process's pages from being
+		 * written to swap when no one else needs
+		 * them.  Lazy form of removing these pages
+		 * at exit time.
+		 */
+		if (PageSwapCache(page)) {
+			page_cache_get(page);
+			if(!is_page_shared(page)) {
+        			delete_dead_swap_cache_page(page);
+
+				UnlockPage(page);
+				page_cache_release(page);
+				continue;
+			}
+			page_cache_release(page);
+		}
+
+		/*
 		 * Dirty swap-cache page? Write it out if
 		 * last copy..
 		 */
@@ -650,6 +683,24 @@
 			list_del(page_lru);
 			nr_active_pages--;
 			continue;
+		}
+
+		/*
+		 * Prevent a dead process's pages from being
+		 * written to swap when no one else needs
+		 * them.  Lazy form of removing these pages
+		 * at exit time.
+		 */
+		if (PageSwapCache(page)) {
+			page_cache_get(page);
+			if(!is_page_shared(page) && !TryLockPage(page)) {
+				delete_dead_swap_cache_page(page);
+
+				UnlockPage(page);
+				page_cache_release(page);
+				continue;
+			}
+			page_cache_release(page);
 		}
 
 		/* Do aging on the pages. */