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

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

[Minchan Kim]

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 <minchan.kim@gmail.com>
CC: Nick Piggin <npiggin@suse.de>
CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Minchan Kim]

 * 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 <minchan.kim@gmail.com>
CC: Nick Piggin <npiggin@suse.de>
CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hugh Dickins]

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 <minchan.kim@gmail.com>
> CC: Nick Piggin <npiggin@suse.de>
> CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> ---
>  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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Minchan Kim]

On Tue, Jan 12, 2010 at 8:29 AM, Hugh Dickins
<hugh.dickins@tiscali.co.uk> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>