patch for 2.1.102 swap code

Hi Bill,

Thanks!

I don't think this should happen: do you have solid evidence that it is a problem? The code in read_swap_cache_async() does

	if (!add_to_swap_cache(new_page, entry)) {
		free_page(new_page_addr);
		return 0;
	}
	swap_duplicate(entry);		/* Account for the swap cache */
	set_bit(PG_locked, &new_page->flags);
	rw_swap_page(READ, entry, (char *) new_page_addr, wait);

which should guarantee a used entry while the IO is in progress. Even if the only use of the entry is the swap cache, it should still be there, and because the page is locked during the IO, it should not be possible for the swap cache reference to be removed before the check in rw_swap_page(). There may well be a more subtle problem here, but I don't think the solution is yet another swap_duplicate(), and I haven't seen reports that might suggest we're losing an unlock in the swapping code anywhere.

Hmm. That shouldn't be a problem if everything is working correctly. However, your first change (the extra swap_duplicate) will leave the swap count elevated while swapin is occurring, and that could certainly lead to this symptom in swapoff(). Does the swapoff problem still occur on an unmodified kernel?

There is also a matching atomic_inc() up above. All swapout is done in try_to_swap_out(), which doesn't do a free_page() to match the unhook of the pte until after the rw_swap_page completes. Swapin should all be done via the swap cache now, and that will also guarantee an extra reference against the page for as long as rw_swap_page is running. However, there are a few borderline cases such as trying to remove swap cache from a locked page which I'll have a check for, as they might make this dangerous.

Me too, and I haven't found anything definitely incriminating so far. The one thing I _have_ found is a day-one threads bug in anonymous page-in. do_no_page() handles write accesses to demand-zero memory with:

anonymous_page:

	entry = pte_wrprotect(mk_pte(ZERO_PAGE, vma->vm_page_prot));
	if (write_access) {
		unsigned long page = __get_free_page(GFP_KERNEL);
		if (!page)
			goto sigbus;
		clear_page(page);
		entry = pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
		vma->vm_mm->rss++;
		tsk->min_flt++;
		flush_page_to_ram(page);
	}
	put_page(page_table, entry);

The __get_free_page() may block, however, and in a threaded environment this will cause the loss of user data plus a memory leak if two threads hit this race. However, I don't think it's related to the current writable cached page problems.

Could you cast your eyes over the patch below? It builds fine and passes the tests I've thrown at it so far, but I'd like a second opinion before forwarding it as a patch for 2.0.

--Stephen

----------------------------------------------------------------
Index: mm/memory.c
===================================================================
RCS file: /home/rcs/CVS/kswap3/linux/mm/memory.c,v retrieving revision 1.1.2.2

diff -u -r1.1.2.2 memory.c
@@ -839,9 +839,14 @@
 anonymous_page:
 	entry = pte_wrprotect(mk_pte(ZERO_PAGE, vma->vm_page_prot));
 	if (write_access) {
+		pte_t old_entry = *page_table; 
 		unsigned long page = __get_free_page(GFP_KERNEL);
 		if (!page)
 			goto sigbus;
+		if (pte_val(old_entry) != pte_val(*page_table)) {
+			free_page(page);
+			return;
+		}
 		clear_page(page);
 		entry = pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
 		vma->vm_mm->rss++;

Date: Sun, 24 May 1998 18:28:48 +0100
From: "Stephen C. Tweedie" <[email protected]>

The __get_free_page() may block, however, and in a threaded environment this will cause the loss of user data plus a memory leak if two threads hit this race. However, I don't think it's related to the current writable cached page problems.

The mmap semaphore is held, it cannot happen.

Later,
David S. Miller
[email protected]

Hi Stephen,

The problem with the swap entry being unused could occur before reaching the code above. If the swap cache lookup fails, the process will have to allocate a page and may block, allowing multiple processes to block on get_free_page. Then the process that completes first could end up releasing the page and swap cache, so that when the other processes wake up from get_free_page the swap entry will no longer be valid. In the above code sequence, the add_to_swap_cache will succeed, swap_duplicate will complain and fail, and rw_swap_cache will complain and return leaving the page locked. There would be warning messages of the problem, but I'd rather avoid it in the first place.

I'm reasonably certain this sequence of events could occur, and other places in the kernel (e.g. swap_in) have tests for the case where multiple processes try to read in the same swap page. The changes in my patch will prevent the swap entry from disappearing when we want to read it in, and will keep read_swap_cache from returning spurious failures.

Yes, I've seen the problem before making the other changes, and there have been some problem reports on the kernel list.

I'm less certain of the possibility of the page being unused in this case, but in any event replacing the atomic_dec() with a free_page seems prudent to me, as there have been a number of other kernel memory leaks caused by an atomic_dec instead of a free_page. But at the very least we should put the printk warning there so that if the problem does occur it can be corrected in the future.

The patch looks reasonable to me, but as DaveM mentioned in a later mail, the
do_wp_page case is supposed to be protected with a semaphore.

Regards,
Bill

Date: Mon, 25 May 1998 08:52:46 -0400
From: Bill Hawes <[email protected]>

The patch looks reasonable to me, but as DaveM mentioned in a later mail, the do_wp_page case is supposed to be protected with a semaphore.

Alas, I thought about this some more. And one piece of code needs to be fixed for this invariant about the semaphore being held in the fault processing code paths to be true everywhere... ptrace()...

Later,
David S. Miller
[email protected]

Hi,

Yep --- I was just about to reply to your last mail with this point when I got your follow-up. I've also had one report that the writable cached page reports started when debugging an electric-fenced binary under gdb. Has anyody seen these vm messages who has definitely NOT been running gdb?

There's also the point that the whole swapout code munges page tables without ever taking the mm semaphore, but that case ought to be protected by the combination of (a) having the kernel spinlock and (b) never stalling between starting a vma walk and modifying the pte. (The swapout code is pretty paranoid about this.) However, I'm not absolutely 100% sure that we don't have any unfortunate races left by this exception. (For example, do we ever protect a vma by the mm semaphore without also doing a lock_kernel()?)

--Stephen

Hi Bill,

That's why read_swap_cache_async repeats the initial entry lookup after calling __get_free_page(). Unfortunately, I hadn't realised that swap_duplicate() had the error check against swap_map[entry]==0. Moving the swap_duplicate up to before the call to __get_free_page should avoid that case.

Excellent --- that should mean it's easy to reproduce, and I've got a test box set up to do tracing on all this code. Is there anything in particular you do to trigger the situation? I've been going over the obvious places in try_to_swap_out and friends, but haven't found anything yet where we might block between updating a pte and modifying the corresponding pte count.

Yes, the printk will help. Swap should _definitely_ be done through the swap cache, and anything which violates this is broken, so although there should never be a chance of freeing the page, the warning is a useful thing to have.

Yep, I've responded to that separately.

--Stephen

Hmm, could read_swap_cache_async() be used to implement swap readahead?

Rik.

+-------------------------------------------+--------------------------+
| Linux: - LinuxHQ MM-patches page          | Scouting       webmaster |
|        - kswapd ask-him & complain-to guy | Vries    cubscout leader |
|     http://www.phys.uu.nl/~riel/          | <[email protected]> |
+-------------------------------------------+--------------------------+

Hi Rik,

Absolutely, it was designed with that in mind. It's a bit close to 2.1 to actually use it, but I'll be doing a lot of work to tighten things up in 2.3 around the swap code, and readahead will be one component of that.

--Stephen

Hi Stephen,

Moving the swap_duplicate() call above the get_free_page() helps, but does not entirely avoid the race: it's possible for lookup_swap_cache() to block on a locked page, and when the process wakes up the swap entry may have disappeared. In order for read_swap_cache to fulfill its contract ("this swap entry exists, go get it for me") it must increment the swap map count before any blocking operation. Hence I moved the swap_duplicate() call above the lookup.

I could check for the case of finding the unlocked swap cache page, and only increment the map count if a wait was needed; this would avoid having to increment and decrement the map count if the page is found, at the expense of a little more complexity. I'll post a mopdified patch for comment ...

I've observed the swapoff messages after running swapoff on a quiescent system that had been swapping heavily previously, and also when running swapoff with the system currently swapping. Try setting up a condition of heavy swapping but with adequate memory available (e.g. two kernel compiles in 32M), and then cycle swapoff -a and swapon -a.

Running swapoff is a good test for the VM system; unfortunately the current kernels have an unrelated problem with kswapd trying too hard to keep memory blocks, so that swapoff may put the system in an unrecoverable kswapd loop. If you try swapoff when the system doesn't have enough memory, the system will lock rather than let swapoff return failure. But this is a problem of swap policy rather than mechanism, and we can try to fix it after the swap mechanics are 100% solid.

Regards,
Bill

ST> Hmm. That shouldn't be a problem if everything is working correctly. ST> However, your first change (the extra swap_duplicate) will leave the ST> swap count elevated while swapin is occurring, and that could certainly ST> lead to this symptom in swapoff(). Does the swapoff problem still occur ST> on an unmodified kernel?

Note: there is a problem with swapoff that should at least be considered. If you use have a SYSV shared memory, and don't map it into a process, and that memory get's swapped out, swapoff will not be able to find it.

This is a very long standing bug and appears not to be a problem in practice. But it is certainly a potential problem.

Eric

Hi,

Thanks; it's added to my list.

--Stephen

ST> Hi,

ST> Thanks; it's added to my list.

Here is a preliminary patch that should fix the problem.

diff -uNrX /home/eric/projects/linux/linux-ignore-files linux-2.1.101.x0/include/linux/swap.h linux-2.1.101.x2/include/linux/swap.h
@@ -75,7 +75,18 @@
 void si_swapinfo(struct sysinfo *);
 unsigned long get_swap_page(void);
 extern void FASTCALL(swap_free(unsigned long));
+  
+  /* So that external drivers can use swap, swapoff */
+struct swap_unuse {
+	int (*func)(unsigned int type, void *arg);
+	void *arg;
+	struct swap_unuse *prev;
+	struct swap_unuse *next;
+};
 
+void register_swap_unuse_function (struct swap_unuse *swap_unuse);
+void unregister_swap_unuse_function (struct swap_unuse *swap_unuse);
+ 
 /*
  * vm_ops not present page codes for shared memory.
  *

diff -uNrX /home/eric/projects/linux/linux-ignore-files linux-2.1.101.x0/mm/Makefile linux-2.1.101.x2/mm/Makefile
@@ -12,4 +12,6 @@
 	    vmalloc.o slab.o \
 	    swap.o vmscan.o page_io.o page_alloc.o swap_state.o swapfile.o
 
+OX_OBJS := swap_syms.o
+
 include $(TOPDIR)/Rules.make

diff -uNrX /home/eric/projects/linux/linux-ignore-files linux-2.1.101.x0/mm/swap_syms.c linux-2.1.101.x2/mm/swap_syms.c
@@ -0,0 +1,15 @@
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/swap.h>
+
+/* Explicit swapping */
+EXPORT_SYMBOL(rw_swap_page);
+EXPORT_SYMBOL(rw_swap_page_nocache);
+EXPORT_SYMBOL(swap_free);
+EXPORT_SYMBOL(get_swap_page);
+EXPORT_SYMBOL(si_swapinfo);
+EXPORT_SYMBOL(register_swap_unuse_function);
+EXPORT_SYMBOL(unregister_swap_unuse_function);
+EXPORT_SYMBOL(swapper_inode);

Hi,

ST> Hi,

ST> Thanks; it's added to my list.

Thanks; it's queued for attention. I'm off to Usenix tomorrow, but I'll be doing lots and lots of VM stuff when I get back (I'm going full time courtesy of Red Hat --- yay!), so I'll check and test and then submit to Linus if all looks OK.

--Stephen