2009-04-07 19:58:40

by Ying Han

[permalink] [raw]
Subject: Re: [PATCH 02/14] mm: fix major/minor fault accounting on retried fault

On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
> VM_FAULT_RETRY does make major/minor faults accounting a bit twisted..
>
> Cc: Ying Han <[email protected]>
> Signed-off-by: Wu Fengguang <[email protected]>
> ---
> arch/x86/mm/fault.c | 2 ++
> mm/memory.c | 22 ++++++++++++++--------
> 2 files changed, 16 insertions(+), 8 deletions(-)
>
> --- mm.orig/arch/x86/mm/fault.c
> +++ mm/arch/x86/mm/fault.c
> @@ -1160,6 +1160,8 @@ good_area:
> if (fault & VM_FAULT_RETRY) {
> if (retry_flag) {
> retry_flag = 0;
> + tsk->maj_flt++;
> + tsk->min_flt--;
> goto retry;
> }
> BUG();
sorry, little bit confuse here. are we assuming the retry path will
return min_flt as always?


> --- mm.orig/mm/memory.c
> +++ mm/mm/memory.c
> @@ -2882,26 +2882,32 @@ int handle_mm_fault(struct mm_struct *mm
> pud_t *pud;
> pmd_t *pmd;
> pte_t *pte;
> + int ret;
>
> __set_current_state(TASK_RUNNING);
>
> - count_vm_event(PGFAULT);
> -
> - if (unlikely(is_vm_hugetlb_page(vma)))
> - return hugetlb_fault(mm, vma, address, write_access);
> + if (unlikely(is_vm_hugetlb_page(vma))) {
> + ret = hugetlb_fault(mm, vma, address, write_access);
> + goto out;
> + }
>
> + ret = VM_FAULT_OOM;
> pgd = pgd_offset(mm, address);
> pud = pud_alloc(mm, pgd, address);
> if (!pud)
> - return VM_FAULT_OOM;
> + goto out;
> pmd = pmd_alloc(mm, pud, address);
> if (!pmd)
> - return VM_FAULT_OOM;
> + goto out;
> pte = pte_alloc_map(mm, pmd, address);
> if (!pte)
> - return VM_FAULT_OOM;
> + goto out;
>
> - return handle_pte_fault(mm, vma, address, pte, pmd, write_access);
> + ret = handle_pte_fault(mm, vma, address, pte, pmd, write_access);
> +out:
> + if (!(ret & VM_FAULT_RETRY))
> + count_vm_event(PGFAULT);
> + return ret;
> }
>
> #ifndef __PAGETABLE_PUD_FOLDED
>
> --
>
>


2009-04-07 22:46:21

by Fengguang Wu

[permalink] [raw]
Subject: Re: [PATCH 02/14] mm: fix major/minor fault accounting on retried fault

On Wed, Apr 08, 2009 at 03:58:16AM +0800, Ying Han wrote:
> On Tue, Apr 7, 2009 at 12:17 AM, Wu Fengguang <[email protected]> wrote:
> > VM_FAULT_RETRY does make major/minor faults accounting a bit twisted..
> >
> > Cc: Ying Han <[email protected]>
> > Signed-off-by: Wu Fengguang <[email protected]>
> > ---
> > arch/x86/mm/fault.c | 2 ++
> > mm/memory.c | 22 ++++++++++++++--------
> > 2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > --- mm.orig/arch/x86/mm/fault.c
> > +++ mm/arch/x86/mm/fault.c
> > @@ -1160,6 +1160,8 @@ good_area:
> > if (fault & VM_FAULT_RETRY) {
> > if (retry_flag) {
> > retry_flag = 0;
> > + tsk->maj_flt++;
> > + tsk->min_flt--;
> > goto retry;
> > }
> > BUG();
> sorry, little bit confuse here. are we assuming the retry path will
> return min_flt as always?

Sure - except for some really exceptional ftruncate cases.
The page was there ready, and we'll retry immediately.

maj_flt/min_flt are not _exact_ numbers by their nature, so 99.9%
accuracy shall be fine.

Thanks,
Fengguang

> > --- mm.orig/mm/memory.c
> > +++ mm/mm/memory.c
> > @@ -2882,26 +2882,32 @@ int handle_mm_fault(struct mm_struct *mm
> > pud_t *pud;
> > pmd_t *pmd;
> > pte_t *pte;
> > + int ret;
> >
> > __set_current_state(TASK_RUNNING);
> >
> > - count_vm_event(PGFAULT);
> > -
> > - if (unlikely(is_vm_hugetlb_page(vma)))
> > - return hugetlb_fault(mm, vma, address, write_access);
> > + if (unlikely(is_vm_hugetlb_page(vma))) {
> > + ret = hugetlb_fault(mm, vma, address, write_access);
> > + goto out;
> > + }
> >
> > + ret = VM_FAULT_OOM;
> > pgd = pgd_offset(mm, address);
> > pud = pud_alloc(mm, pgd, address);
> > if (!pud)
> > - return VM_FAULT_OOM;
> > + goto out;
> > pmd = pmd_alloc(mm, pud, address);
> > if (!pmd)
> > - return VM_FAULT_OOM;
> > + goto out;
> > pte = pte_alloc_map(mm, pmd, address);
> > if (!pte)
> > - return VM_FAULT_OOM;
> > + goto out;
> >
> > - return handle_pte_fault(mm, vma, address, pte, pmd, write_access);
> > + ret = handle_pte_fault(mm, vma, address, pte, pmd, write_access);
> > +out:
> > + if (!(ret & VM_FAULT_RETRY))
> > + count_vm_event(PGFAULT);
> > + return ret;
> > }
> >
> > #ifndef __PAGETABLE_PUD_FOLDED
> >
> > --
> >
> >