Cc: Ying Han <[email protected]>
Signed-off-by: Wu Fengguang <[email protected]>
---
mm/memory.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
--- mm.orig/mm/memory.c
+++ mm/mm/memory.c
@@ -2766,10 +2766,8 @@ static int do_linear_fault(struct mm_str
{
pgoff_t pgoff = (((address & PAGE_MASK)
- vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
- int write = write_access & ~FAULT_FLAG_RETRY;
- unsigned int flags = (write ? FAULT_FLAG_WRITE : 0);
+ unsigned int flags = (write_access ? FAULT_FLAG_WRITE : 0);
- flags |= (write_access & FAULT_FLAG_RETRY);
pte_unmap(page_table);
return __do_fault(mm, vma, address, pmd, pgoff, flags, orig_pte);
}
--
On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
> Cc: Ying Han <[email protected]>
> Signed-off-by: Wu Fengguang <[email protected]>
> ---
> mm/memory.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> --- mm.orig/mm/memory.c
> +++ mm/mm/memory.c
> @@ -2766,10 +2766,8 @@ static int do_linear_fault(struct mm_str
> {
> pgoff_t pgoff = (((address & PAGE_MASK)
> - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> - int write = write_access & ~FAULT_FLAG_RETRY;
> - unsigned int flags = (write ? FAULT_FLAG_WRITE : 0);
> + unsigned int flags = (write_access ? FAULT_FLAG_WRITE : 0);
>
> - flags |= (write_access & FAULT_FLAG_RETRY);
> pte_unmap(page_table);
> return __do_fault(mm, vma, address, pmd, pgoff, flags, orig_pte);
> }
So, we got rid of FAULT_FLAG_RETRY flag?
> --
>
>
On Wed, Apr 08, 2009 at 04:03:36AM +0800, Ying Han wrote:
> On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
> > Cc: Ying Han <[email protected]>
> > Signed-off-by: Wu Fengguang <[email protected]>
> > ---
> > mm/memory.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > --- mm.orig/mm/memory.c
> > +++ mm/mm/memory.c
> > @@ -2766,10 +2766,8 @@ static int do_linear_fault(struct mm_str
> > {
> > pgoff_t pgoff = (((address & PAGE_MASK)
> > - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> > - int write = write_access & ~FAULT_FLAG_RETRY;
> > - unsigned int flags = (write ? FAULT_FLAG_WRITE : 0);
> > + unsigned int flags = (write_access ? FAULT_FLAG_WRITE : 0);
> >
> > - flags |= (write_access & FAULT_FLAG_RETRY);
> > pte_unmap(page_table);
> > return __do_fault(mm, vma, address, pmd, pgoff, flags, orig_pte);
> > }
> So, we got rid of FAULT_FLAG_RETRY flag?
Seems yes for the current mm tree, see the following two commits.
I did this patch on seeing 761fe7bc8193b7. But a closer look
indicates that the following two patches disable the filemap
VM_FAULT_RETRY part totally...
Anyway, if these two patches are to be reverted somehow(I guess yes),
this patch shall be _ignored_.
btw, do you have any test case and performance numbers for
FAULT_FLAG_RETRY? And possible overheads for (the worst case)
sparse random mmap reads on a sparse file? I cannot find any
in your changelogs..
Thanks,
Fengguang
commit 761fe7bc8193b7858b7dc7eb4a026dc66e49fe1f
Author: Andrew Morton <[email protected]>
Date: Mon Feb 9 21:08:50 2009 +0100
A shot in the dark :(
Cc: Mike Waychison <[email protected]>
Cc: Ying Han <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index bac7d7a..1c6736d 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1139,8 +1139,6 @@ good_area:
return;
}
- write |= retry_flag;
-
/*
* If for any reason at all we couldn't handle the fault,
* make sure we exit gracefully rather than endlessly redo
commit f01ca7a68c37680a4eee22a8722a713c5102b3bb
Author: Andrew Morton <[email protected]>
Date: Mon Feb 9 21:08:50 2009 +0100
Untangle the `write' boolean from the FAULT_FLAG_foo non-boolean field.
Cc: "H. Peter Anvin" <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Lee Schermerhorn <[email protected]>
Cc: Mike Waychison <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rohit Seth <[email protected]>
Cc: T<F6>r<F6>k Edwin <[email protected]>
Cc: [email protected]
Cc: Ying Han <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index b2cc88f..bac7d7a 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -978,7 +978,7 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
struct mm_struct *mm;
int write;
int fault;
- unsigned int retry_flag = FAULT_FLAG_RETRY;
+ int retry_flag = 1;
tsk = current;
mm = tsk->mm;
@@ -1140,6 +1140,7 @@ good_area:
}
write |= retry_flag;
+
/*
* If for any reason at all we couldn't handle the fault,
* make sure we exit gracefully rather than endlessly redo
@@ -1159,8 +1160,8 @@ good_area:
* be removed or changed after the retry.
*/
if (fault & VM_FAULT_RETRY) {
- if (write & FAULT_FLAG_RETRY) {
- retry_flag &= ~FAULT_FLAG_RETRY;
+ if (retry_flag) {
+ retry_flag = 0;
goto retry;
}
On Tue, Apr 7, 2009 at 4:27 PM, Wu Fengguang <[email protected]> wrote:
> On Wed, Apr 08, 2009 at 04:03:36AM +0800, Ying Han wrote:
>> On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
>> > Cc: Ying Han <[email protected]>
>> > Signed-off-by: Wu Fengguang <[email protected]>
>> > ---
>> > mm/memory.c | 4 +---
>> > 1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > --- mm.orig/mm/memory.c
>> > +++ mm/mm/memory.c
>> > @@ -2766,10 +2766,8 @@ static int do_linear_fault(struct mm_str
>> > {
>> > pgoff_t pgoff = (((address & PAGE_MASK)
>> > - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
>> > - int write = write_access & ~FAULT_FLAG_RETRY;
>> > - unsigned int flags = (write ? FAULT_FLAG_WRITE : 0);
>> > + unsigned int flags = (write_access ? FAULT_FLAG_WRITE : 0);
>> >
>> > - flags |= (write_access & FAULT_FLAG_RETRY);
>> > pte_unmap(page_table);
>> > return __do_fault(mm, vma, address, pmd, pgoff, flags, orig_pte);
>> > }
>> So, we got rid of FAULT_FLAG_RETRY flag?
>
> Seems yes for the current mm tree, see the following two commits.
>
> I did this patch on seeing 761fe7bc8193b7. But a closer look
> indicates that the following two patches disable the filemap
> VM_FAULT_RETRY part totally...
>
> Anyway, if these two patches are to be reverted somehow(I guess yes),
> this patch shall be _ignored_.
>
> btw, do you have any test case and performance numbers for
> FAULT_FLAG_RETRY? And possible overheads for (the worst case)
> sparse random mmap reads on a sparse file? I cannot find any
> in your changelogs..
here is the benchmark i posted on [V1] but somehow missed in [V2] describtion
Benchmarks:
case 1. one application has a high count of threads each faulting in
different pages of a hugefile. Benchmark indicate that this double data
structure walking in case of major fault results in << 1% performance hit.
case 2. add another thread in the above application which in a tight loop of
mmap()/munmap(). Here we measure loop count in the new thread while other
threads doing the same amount of work as case one. we got << 3% performance
hit on the Complete Time(benchmark value for case one) and 10% performance
improvement on the mmap()/munmap() counter.
This patch helps a lot in cases we have writer which is waitting behind all
readers, so it could execute much faster.
--Ying
>
> Thanks,
> Fengguang
>
>
> commit 761fe7bc8193b7858b7dc7eb4a026dc66e49fe1f
> Author: Andrew Morton <[email protected]>
> Date: Mon Feb 9 21:08:50 2009 +0100
>
> A shot in the dark :(
>
> Cc: Mike Waychison <[email protected]>
> Cc: Ying Han <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index bac7d7a..1c6736d 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1139,8 +1139,6 @@ good_area:
> return;
> }
>
> - write |= retry_flag;
> -
> /*
> * If for any reason at all we couldn't handle the fault,
> * make sure we exit gracefully rather than endlessly redo
>
>
> commit f01ca7a68c37680a4eee22a8722a713c5102b3bb
> Author: Andrew Morton <[email protected]>
> Date: Mon Feb 9 21:08:50 2009 +0100
>
> Untangle the `write' boolean from the FAULT_FLAG_foo non-boolean field.
>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Hugh Dickins <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Lee Schermerhorn <[email protected]>
> Cc: Mike Waychison <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Rohit Seth <[email protected]>
> Cc: T<F6>r<F6>k Edwin <[email protected]>
> Cc: [email protected]
> Cc: Ying Han <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index b2cc88f..bac7d7a 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -978,7 +978,7 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
> struct mm_struct *mm;
> int write;
> int fault;
> - unsigned int retry_flag = FAULT_FLAG_RETRY;
> + int retry_flag = 1;
>
> tsk = current;
> mm = tsk->mm;
> @@ -1140,6 +1140,7 @@ good_area:
> }
>
> write |= retry_flag;
> +
> /*
> * If for any reason at all we couldn't handle the fault,
> * make sure we exit gracefully rather than endlessly redo
> @@ -1159,8 +1160,8 @@ good_area:
> * be removed or changed after the retry.
> */
> if (fault & VM_FAULT_RETRY) {
> - if (write & FAULT_FLAG_RETRY) {
> - retry_flag &= ~FAULT_FLAG_RETRY;
> + if (retry_flag) {
> + retry_flag = 0;
> goto retry;
> }
>
>
Hi Ying Han,
On Wed, Apr 08, 2009 at 09:17:26AM +0800, Ying Han wrote:
> On Tue, Apr 7, 2009 at 4:27 PM, Wu Fengguang <[email protected]> wrote:
> > On Wed, Apr 08, 2009 at 04:03:36AM +0800, Ying Han wrote:
> >> On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
> >> > Cc: Ying Han <[email protected]>
> >> > Signed-off-by: Wu Fengguang <[email protected]>
> >> > ---
> >> > mm/memory.c | 4 +---
> >> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >> >
> >> > --- mm.orig/mm/memory.c
> >> > +++ mm/mm/memory.c
> >> > @@ -2766,10 +2766,8 @@ static int do_linear_fault(struct mm_str
> >> > {
> >> > pgoff_t pgoff = (((address & PAGE_MASK)
> >> > - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> >> > - int write = write_access & ~FAULT_FLAG_RETRY;
> >> > - unsigned int flags = (write ? FAULT_FLAG_WRITE : 0);
> >> > + unsigned int flags = (write_access ? FAULT_FLAG_WRITE : 0);
> >> >
> >> > - flags |= (write_access & FAULT_FLAG_RETRY);
> >> > pte_unmap(page_table);
> >> > return __do_fault(mm, vma, address, pmd, pgoff, flags, orig_pte);
> >> > }
> >> So, we got rid of FAULT_FLAG_RETRY flag?
> >
> > Seems yes for the current mm tree, see the following two commits.
> >
> > I did this patch on seeing 761fe7bc8193b7. But a closer look
> > indicates that the following two patches disable the filemap
> > VM_FAULT_RETRY part totally...
> >
> > Anyway, if these two patches are to be reverted somehow(I guess yes),
> > this patch shall be _ignored_.
> >
> > btw, do you have any test case and performance numbers for
> > FAULT_FLAG_RETRY? And possible overheads for (the worst case)
> > sparse random mmap reads on a sparse file? I cannot find any
> > in your changelogs..
>
> here is the benchmark i posted on [V1] but somehow missed in [V2] describtion
>
> Benchmarks:
> case 1. one application has a high count of threads each faulting in
> different pages of a hugefile. Benchmark indicate that this double data
> structure walking in case of major fault results in << 1% performance hit.
>
> case 2. add another thread in the above application which in a tight loop of
> mmap()/munmap(). Here we measure loop count in the new thread while other
> threads doing the same amount of work as case one. we got << 3% performance
> hit on the Complete Time(benchmark value for case one) and 10% performance
> improvement on the mmap()/munmap() counter.
>
> This patch helps a lot in cases we have writer which is waitting behind all
> readers, so it could execute much faster.
>
Just tested the sparse-random-read-on-sparse-file case, and found the
performance impact to be 0.4% (8.706s vs 8.744s). Kind of acceptable.
without FAULT_FLAG_RETRY:
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.28s user 5.39s system 99% cpu 8.692 total
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.17s user 5.54s system 99% cpu 8.742 total
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.18s user 5.48s system 99% cpu 8.684 total
FAULT_FLAG_RETRY:
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.18s user 5.63s system 99% cpu 8.825 total
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.22s user 5.47s system 99% cpu 8.718 total
iotrace.rb --load stride-100 --mplay /mnt/btrfs-ram/sparse 3.13s user 5.55s system 99% cpu 8.690 total
In the above faked workload, the mmap read page offsets are loaded from
stride-100 and performed on /mnt/btrfs-ram/sparse, which are created by:
seq 0 100 1000000 > stride-100
dd if=/dev/zero of=/mnt/btrfs-ram/sparse bs=1M count=1 seek=1024000
Thanks,
Fengguang