delayacct: fix swapin delay accounting (maybe)

RFC PATCH In review Nick Piggin 2 participants 2 messages Sep 3, 2007 – Sep 4, 2007

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Nick Piggin

author

2007-09-03 19:58 UTC #0 lore raw

RFC PATCH 1/1 delayacct: fix swapin delay accounting (maybe)

Hi,

I can't convince myself that delay accounting for swapin is quite right at the moment (not having a test setup handy to run and check for myself). Maybe I'm not reading the swapin code very well...

lookup_swap_cache, and read_swap_cache_async should be non-blocking operations for the most part.

read_swap_cache_async might, when allocating the new page, go into reclaim and take a long time to come back. However is that any more a "swapin" delay than eg. when we sleep on mmap_sem when first taking the fault, or any other types of fault which require allocations? None of which we account for as swapin delay.

But the most obvious delay, where we actually lock the page waiting for the swap IO to finish, does not seem to be accounted at all!

My proposed fix is to just move the swaping delay accounting to the point where the VM does actually wait, for the swapin.

I have no idea what uses swapin delay accounting, but it would be good to see if this makes a positive (or at least not negative) impact on those users...

Thanks,
Nick

mm/memory.c +2 -3

Index: linux-2.6/mm/memory.c
===================================================================
@@ -2158,7 +2158,6 @@ static int do_swap_page(struct mm_struct
 		migration_entry_wait(mm, pmd, address);
 		goto out;
 	}
-	delayacct_set_flag(DELAYACCT_PF_SWAPIN);
 	page = lookup_swap_cache(entry);
 	if (!page) {
 		grab_swap_token(); /* Contend for token _before_ read-in */

@@ -2172,7 +2171,6 @@ static int do_swap_page(struct mm_struct
 			page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 			if (likely(pte_same(*page_table, orig_pte)))
 				ret = VM_FAULT_OOM;
-			delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 			goto unlock;
 		}

@@ -2181,9 +2179,10 @@ static int do_swap_page(struct mm_struct
 		count_vm_event(PGMAJFAULT);
 	}
 
-	delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 	mark_page_accessed(page);
+	delayacct_set_flag(DELAYACCT_PF_SWAPIN);
 	lock_page(page);
+	delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 
 	/*
 	 * Back out if somebody else already faulted in this pte.

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Fengguang Wu

2007-09-04 06:48 UTC #1 lore raw

Quoted 9 lines · Nick Piggin

Hi,
I can't convince myself that delay accounting for swapin is quite right at the moment (not having a test setup handy to run and check for myself). Maybe I'm not reading the swapin code very well...
lookup_swap_cache, and read_swap_cache_async should be non-blocking operations for the most part.

Quoted 8 lines

read_swap_cache_async might, when allocating the new page, go into reclaim and take a long time to come back. However is that any more a "swapin" delay than eg. when we sleep on mmap_sem when first taking the fault, or any other types of fault which require allocations? None of which we account for as swapin delay.
But the most obvious delay, where we actually lock the page waiting for the swap IO to finish, does not seem to be accounted at all!

Good catch! I think you are right after some proof reading.

That lock_page() should be the first IO block point. There may be double read_swap_cache_async() calls for the same offset: once in swapin_readahead(), the other immediately after swapin_readahead(). But the second one will return directly. So there's no other block/delay point for the newly submitted read IO.

Fengguang

Quoted 45 lines

My proposed fix is to just move the swaping delay accounting to the point where the VM does actually wait, for the swapin.

I have no idea what uses swapin delay accounting, but it would be good to see if this makes a positive (or at least not negative) impact on those users...

Thanks,
Nick

--
Index: linux-2.6/mm/memory.c
===================================================================

mm/memory.c +2 -4

@@ -2158,7 +2158,6 @@ static int do_swap_page(struct mm_struct
 		migration_entry_wait(mm, pmd, address);
 		goto out;
 	}
-	delayacct_set_flag(DELAYACCT_PF_SWAPIN);
 	page = lookup_swap_cache(entry);
 	if (!page) {
 		grab_swap_token(); /* Contend for token _before_ read-in */

@@ -2172,7 +2171,6 @@ static int do_swap_page(struct mm_struct
 			page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 			if (likely(pte_same(*page_table, orig_pte)))
 				ret = VM_FAULT_OOM;
-			delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 			goto unlock;
 		}

@@ -2181,9 +2179,10 @@ static int do_swap_page(struct mm_struct
 		count_vm_event(PGMAJFAULT);
 	}
 
-	delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 	mark_page_accessed(page);
+	delayacct_set_flag(DELAYACCT_PF_SWAPIN);
 	lock_page(page);
+	delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 
 	/*
 	 * Back out if somebody else already faulted in this pte.
 
--

Quoted 4 lines

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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>