2010-01-11 02:44:45

by Minchan Kim

Subject: [PATCH -mmotm-2010-01-06-14-34] Fix fault count of task in GUP

get_user_pages calls handle_mm_fault to pin the arguemented
task's page. handle_mm_fault cause major or minor fault and
get_user_pages counts it into task which is passed by argument.

But the fault happens in current task's context.
So we have to count it not argumented task's context but current
task's one.

Signed-off-by: Minchan Kim <[email protected]>
CC: Nick Piggin <[email protected]>
CC: Hugh Dickins <[email protected]>
---
mm/memory.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 521abf6..2513581 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1486,9 +1486,9 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
BUG();
}
if (ret & VM_FAULT_MAJOR)
- tsk->maj_flt++;
+ current->maj_flt++;
else
- tsk->min_flt++;
+ current->min_flt++;

/*
* The VM_FAULT_WRITE bit tells us that
--
1.5.6.3


--
Kind regards,
Minchan Kim

2010-01-11 03:43:20

by Minchan Kim

Subject: [PATCH v2 -mmotm-2010-01-06-14-34] Fix fault count of task in GUP

* V2
* Don't count in case of kernel thread

== CUT HERE ==

get_user_pagse calls handle_mm_fault to get the arguemented
task's page. handle_mm_fault cause major or minor fault and
get_user_pages counts it into task which is passed by argument.

But the fault happens in current task's context.
So we have to count it not argumented task's context but current
task's one.

Signed-off-by: Minchan Kim <[email protected]>
CC: Nick Piggin <[email protected]>
CC: Hugh Dickins <[email protected]>
---
mm/memory.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 521abf6..0eb9536 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1485,11 +1485,11 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
return i ? i : -EFAULT;
BUG();
}
- if (ret & VM_FAULT_MAJOR)
- tsk->maj_flt++;
- else
- tsk->min_flt++;
-
+ if (!(current->flags & PF_KTHREAD))
+ if (ret & VM_FAULT_MAJOR)
+ current->maj_flt++;
+ else
+ current->min_flt++;
/*
* The VM_FAULT_WRITE bit tells us that
* do_wp_page has broken COW when necessary,
--
1.5.6.3


--
Kind regards,
Minchan Kim

2010-01-11 23:29:49

by Hugh Dickins

Subject: Re: [PATCH -mmotm-2010-01-06-14-34] Fix fault count of task in GUP

On Mon, 11 Jan 2010, Minchan Kim wrote:
>
> get_user_pages calls handle_mm_fault to pin the arguemented
> task's page. handle_mm_fault cause major or minor fault and
> get_user_pages counts it into task which is passed by argument.
>
> But the fault happens in current task's context.
> So we have to count it not argumented task's context but current
> task's one.

Have to?

current simulates a fault into tsk's address space.
It is not a fault into current's address space.

I can see that this could be argued either way, or even
that such a "fault" should not be counted at all; but I do not
see a reason to change the way we have been counting it for years.

Sorry, but NAK (to this and to the v2) -
unless you have a stronger argument.

Hugh

>
> Signed-off-by: Minchan Kim <[email protected]>
> CC: Nick Piggin <[email protected]>
> CC: Hugh Dickins <[email protected]>
> ---
> mm/memory.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 521abf6..2513581 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1486,9 +1486,9 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> BUG();
> }
> if (ret & VM_FAULT_MAJOR)
> - tsk->maj_flt++;
> + current->maj_flt++;
> else
> - tsk->min_flt++;
> + current->min_flt++;
>
> /*
> * The VM_FAULT_WRITE bit tells us that
> --
> 1.5.6.3
>
>
> --
> Kind regards,
> Minchan Kim

2010-01-12 00:05:43

by Minchan Kim

Subject: Re: [PATCH -mmotm-2010-01-06-14-34] Fix fault count of task in GUP

On Tue, Jan 12, 2010 at 8:29 AM, Hugh Dickins
<[email protected]> wrote:
> On Mon, 11 Jan 2010, Minchan Kim wrote:
>>
>> get_user_pages calls handle_mm_fault to pin the arguemented
>> task's page. handle_mm_fault cause major or minor fault and
>> get_user_pages counts it into task which is passed by argument.
>>
>> But the fault happens in current task's context.
>> So we have to count it not argumented task's context but current
>> task's one.
>
> Have to?
>
> current simulates a fault into tsk's address space.
> It is not a fault into current's address space.
>
> I can see that this could be argued either way, or even
> that such a "fault" should not be counted at all; but I do not
> see a reason to change the way we have been counting it for years.
>
> Sorry, but NAK (to this and to the v2) -
> unless you have a stronger argument.

Okay. The I/O to get a page happen current's context.
So I thought we have to count it with current.
But now that I think about it, yes. It's not current's _fault_.
I agree with your opinion.

Thanks for correcting me. Hugh.



--
Kind regards,
Minchan Kim