readahead: fault retry breaks mmap file read random detection

PATCH In review Shaohua Li 3 participants 4 messages Aug 22, 2012 – Aug 30, 2012

Subsystems (18)

Memory management Arm (32-bit) x86 PowerPC +14 more subsystems

lore

Share & tools

lore

Shaohua Li

author

2012-08-22 03:40 UTC #0 lore raw

PATCH 1/1 readahead: fault retry breaks mmap file read random detection

.fault now can retry. The retry can break state machine of .fault. In filemap_fault, if page is miss, ra->mmap_miss is increased. In the second try, since the page is in page cache now, ra->mmap_miss is decreased. And these are done in one fault, so we can't detect random mmap file access.

Add a new flag to indicate .fault is tried once. In the second try, skip ra->mmap_miss decreasing. The filemap_fault state machine is ok with it.

I only tested x86, didn't test other archs, but looks the change for other archs is obvious, but who knows :)

Signed-off-by Shaohua Li <[email protected]>

---
 arch/arm/mm/fault.c        |    1 +
 arch/avr32/mm/fault.c      |    1 +
 arch/cris/mm/fault.c       |    1 +
 arch/hexagon/mm/vm_fault.c |    1 +
 arch/ia64/mm/fault.c       |    1 +
 arch/m68k/mm/fault.c       |    1 +
 arch/microblaze/mm/fault.c |    1 +
 arch/mips/mm/fault.c       |    1 +
 arch/openrisc/mm/fault.c   |    1 +
 arch/powerpc/mm/fault.c    |    1 +
 arch/s390/mm/fault.c       |    1 +
 arch/sh/mm/fault.c         |    1 +
 arch/sparc/mm/fault_32.c   |    1 +
 arch/sparc/mm/fault_64.c   |    1 +
 arch/tile/mm/fault.c       |    1 +
 arch/um/kernel/trap.c      |    1 +
 arch/x86/mm/fault.c        |    1 +
 arch/xtensa/mm/fault.c     |    1 +
 include/linux/mm.h         |    1 +
 mm/filemap.c               |    4 ++--
 20 files changed, 21 insertions(+), 2 deletions(-)

arch/x86/mm/fault.c +1 -0

Index: linux/arch/x86/mm/fault.c
===================================================================
@@ -1201,6 +1201,7 @@ good_area:
 			/* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
 			 * of starvation. */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 			goto retry;
 		}
 	}

include/linux/mm.h +1 -0

Index: linux/include/linux/mm.h
===================================================================
@@ -157,6 +157,7 @@ extern pgprot_t protection_map[16];
 #define FAULT_FLAG_ALLOW_RETRY	0x08	/* Retry fault if blocking */
 #define FAULT_FLAG_RETRY_NOWAIT	0x10	/* Don't drop mmap_sem and wait when retrying */
 #define FAULT_FLAG_KILLABLE	0x20	/* The fault task is in SIGKILL killable region */
+#define FAULT_FLAG_TRIED	0x40	/* second try */
 
 /*
  * This interface is used by x86 PAT code to identify a pfn mapping that is

mm/filemap.c +2 -2

Index: linux/mm/filemap.c
===================================================================
@@ -1611,13 +1611,13 @@ int filemap_fault(struct vm_area_struct
 	 * Do we have something in the page cache already?
 	 */
 	page = find_get_page(mapping, offset);
-	if (likely(page)) {
+	if (likely(page) && !(vmf->flags & FAULT_FLAG_TRIED)) {
 		/*
 		 * We found the page, so try async readahead before
 		 * waiting for the lock.
 		 */
 		do_async_mmap_readahead(vma, ra, file, page, offset);
-	} else {
+	} else if (!page) {
 		/* No page in the page cache at all */
 		do_sync_mmap_readahead(vma, ra, file, offset);
 		count_vm_event(PGMAJFAULT);

arch/arm/mm/fault.c +1 -0

Index: linux/arch/arm/mm/fault.c
===================================================================
@@ -336,6 +336,7 @@ retry:
 			/* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
 			* of starvation. */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 			goto retry;
 		}
 	}

arch/avr32/mm/fault.c +1 -0

Index: linux/arch/avr32/mm/fault.c
===================================================================
@@ -152,6 +152,7 @@ good_area:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would have

arch/cris/mm/fault.c +1 -0

Index: linux/arch/cris/mm/fault.c
===================================================================
@@ -186,6 +186,7 @@ retry:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would

arch/hexagon/mm/vm_fault.c +1 -0

Index: linux/arch/hexagon/mm/vm_fault.c
===================================================================
@@ -113,6 +113,7 @@ good_area:
 				current->min_flt++;
 			if (fault & VM_FAULT_RETRY) {
 				flags &= ~FAULT_FLAG_ALLOW_RETRY;
+				flags |= FAULT_FLAG_TRIED;
 				goto retry;
 			}
 		}

arch/ia64/mm/fault.c +1 -0

Index: linux/arch/ia64/mm/fault.c
===================================================================
@@ -184,6 +184,7 @@ retry:
 			current->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			 /* No need to up_read(&mm->mmap_sem) as we would
 			 * have already released it in __lock_page_or_retry

arch/m68k/mm/fault.c +1 -0

Index: linux/arch/m68k/mm/fault.c
===================================================================
@@ -170,6 +170,7 @@ good_area:
 			/* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
 			 * of starvation. */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would

arch/microblaze/mm/fault.c +1 -0

Index: linux/arch/microblaze/mm/fault.c
===================================================================
@@ -233,6 +233,7 @@ good_area:
 			current->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would

arch/mips/mm/fault.c +1 -0

Index: linux/arch/mips/mm/fault.c
===================================================================
@@ -171,6 +171,7 @@ good_area:
 		}
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would

arch/openrisc/mm/fault.c +1 -0

Index: linux/arch/openrisc/mm/fault.c
===================================================================
@@ -183,6 +183,7 @@ good_area:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			 /* No need to up_read(&mm->mmap_sem) as we would
 			 * have already released it in __lock_page_or_retry

arch/powerpc/mm/fault.c +1 -0

Index: linux/arch/powerpc/mm/fault.c
===================================================================
@@ -450,6 +450,7 @@ good_area:
 			/* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
 			 * of starvation. */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 			goto retry;
 		}
 	}

arch/s390/mm/fault.c +1 -0

Index: linux/arch/s390/mm/fault.c
===================================================================
@@ -367,6 +367,7 @@ retry:
 			/* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
 			 * of starvation. */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 			down_read(&mm->mmap_sem);
 			goto retry;
 		}

arch/sh/mm/fault.c +1 -0

Index: linux/arch/sh/mm/fault.c
===================================================================
@@ -504,6 +504,7 @@ good_area:
 		}
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/*
 			 * No need to up_read(&mm->mmap_sem) as we would

arch/sparc/mm/fault_32.c +1 -0

Index: linux/arch/sparc/mm/fault_32.c
===================================================================
@@ -265,6 +265,7 @@ good_area:
 		}
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/* No need to up_read(&mm->mmap_sem) as we would
 			 * have already released it in __lock_page_or_retry

arch/sparc/mm/fault_64.c +1 -0

Index: linux/arch/sparc/mm/fault_64.c
===================================================================
@@ -452,6 +452,7 @@ good_area:
 		}
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			/* No need to up_read(&mm->mmap_sem) as we would
 			 * have already released it in __lock_page_or_retry

arch/tile/mm/fault.c +1 -0

Index: linux/arch/tile/mm/fault.c
===================================================================
@@ -454,6 +454,7 @@ good_area:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			 /*
 			  * No need to up_read(&mm->mmap_sem) as we would

arch/um/kernel/trap.c +1 -0

Index: linux/arch/um/kernel/trap.c
===================================================================
@@ -89,6 +89,7 @@ good_area:
 				current->min_flt++;
 			if (fault & VM_FAULT_RETRY) {
 				flags &= ~FAULT_FLAG_ALLOW_RETRY;
+				flags |= FAULT_FLAG_TRIED;
 
 				goto retry;
 			}

arch/xtensa/mm/fault.c +1 -0

Index: linux/arch/xtensa/mm/fault.c
===================================================================
@@ -126,6 +126,7 @@ good_area:
 			current->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
 
 			 /* No need to up_read(&mm->mmap_sem) as we would
 			 * have already released it in __lock_page_or_retry

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

Rik van Riel

2012-08-22 15:42 UTC #1 lore raw

Quoted 13 lines · Shaohua Li

.fault now can retry. The retry can break state machine of .fault. In filemap_fault, if page is miss, ra->mmap_miss is increased. In the second try, since the page is in page cache now, ra->mmap_miss is decreased. And these are done in one fault, so we can't detect random mmap file access.
Add a new flag to indicate .fault is tried once. In the second try, skip ra->mmap_miss decreasing. The filemap_fault state machine is ok with it.
Index: linux/arch/avr32/mm/fault.c
===================================================================

arch/avr32/mm/fault.c good_area: · line 155

Quoted 4 lines

arch/avr32/mm/fault.c +1 -0

@@ -152,6 +152,7 @@ good_area:
  			tsk->min_flt++;
  		if (fault & VM_FAULT_RETRY) {
  			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;

Is there any place where you set FAULT_FLAG_TRIED
where FAULT_FLAG_ALLOW_RETRY is not cleared?

In other words, could we use the absence of the
FAULT_FLAG_ALLOW_RETRY as the test, avoiding the
need for a new bit flag?

Shaohua Li

author

2012-08-23 01:10 UTC #2 lore raw

Quoted 22 lines · Rik van Riel

Quoted 13 lines · Shaohua Li
.fault now can retry. The retry can break state machine of .fault. In filemap_fault, if page is miss, ra->mmap_miss is increased. In the second try, since the page is in page cache now, ra->mmap_miss is decreased. And these are done in one fault, so we can't detect random mmap file access.
Add a new flag to indicate .fault is tried once. In the second try, skip ra->mmap_miss decreasing. The filemap_fault state machine is ok with it.
Index: linux/arch/avr32/mm/fault.c
===================================================================
arch/avr32/mm/fault.c good_area: · line 155
Quoted 4 lines
arch/avr32/mm/fault.c +1 -0
@@ -152,6 +152,7 @@ good_area:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
Is there any place where you set FAULT_FLAG_TRIED where FAULT_FLAG_ALLOW_RETRY is not cleared?

Quoted 3 lines

In other words, could we use the absence of the FAULT_FLAG_ALLOW_RETRY as the test, avoiding the need for a new bit flag?

There are still several archs (~7) don't enable fault retry yet. For such archs, FAULT_FLAG_ALLOW_RETRY isn't set in the first try. If all archs support fault retry, the new flag is unnecessary.

Thanks,
Shaohua

Minchan Kim

2012-08-30 17:21 UTC #3 lore raw

Quoted 23 lines · Shaohua Li

Quoted 22 lines · Rik van Riel
Quoted 13 lines · Shaohua Li
.fault now can retry. The retry can break state machine of .fault. In filemap_fault, if page is miss, ra->mmap_miss is increased. In the second try, since the page is in page cache now, ra->mmap_miss is decreased. And these are done in one fault, so we can't detect random mmap file access.
Add a new flag to indicate .fault is tried once. In the second try, skip ra->mmap_miss decreasing. The filemap_fault state machine is ok with it.
Index: linux/arch/avr32/mm/fault.c
===================================================================
arch/avr32/mm/fault.c good_area: · line 155
Quoted 4 lines
arch/avr32/mm/fault.c +1 -0
@@ -152,6 +152,7 @@ good_area:
 			tsk->min_flt++;
 		if (fault & VM_FAULT_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+			flags |= FAULT_FLAG_TRIED;
Is there any place where you set FAULT_FLAG_TRIED where FAULT_FLAG_ALLOW_RETRY is not cleared?

Quoted 7 lines

Quoted 3 lines
In other words, could we use the absence of the FAULT_FLAG_ALLOW_RETRY as the test, avoiding the need for a new bit flag?
There are still several archs (~7) don't enable fault retry yet. For such archs, FAULT_FLAG_ALLOW_RETRY isn't set in the first try. If all archs support fault retry, the new flag is unnecessary.

I'm not sure it's a good idea because archs support FAULT_FLAG_ALLOW_RETRY use FAULT_FLAG_ALLOW_RETRY to avoid miscount major/minor fault accouting. It's a similar to your goal so if you introduce new flag, major/minor fault accounting should use your flag for the consistency, too. Otherwise, you could be better to use FAULT_FLAG_ALLOW_RETRY but the problem is all arch don't support it now as you mentioned. So ideal solution is that firstly you can make all archs support FAULT_FLAG_ALLOW_RETRY(I'm not sure it's easy or not), then use that bit flag instead of introducing new flag. If you don't like it, I'm not strongly against with you but at least, please write down TODO for tidy up in future.

TODO :
If all arch support FAULT_FLAG_ALLOW_RETRY in future, we can remove FAULT_FLAG_TRIED and use FAULT_FLAG_ALLOW_RETRY to prevent misaccounting major/minor fault and readahead mmap_miss.

Quoted 9 lines

Thanks,
Shaohua
--
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>