[PATCH -mmotm-2010-01-06-14-34] Count minor fault in break_ksm

[PATCH -mmotm-2010-01-06-14-34] Count minor fault in break_ksm

[Minchan Kim]

We have counted task's maj/min fault after handle_mm_fault.
break_ksm misses that.

I wanted to check by VM_FAULT_ERROR. 
But now break_ksm doesn't handle HWPOISON error. 

Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
CC: Izik Eidus <ieidus@redhat.com>
---
 mm/ksm.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 56a0da1..3a1fda4 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -367,9 +367,13 @@ static int break_ksm(struct vm_area_struct *vma, unsigned long addr)
 		page = follow_page(vma, addr, FOLL_GET);
 		if (!page)
 			break;
-		if (PageKsm(page))
+		if (PageKsm(page)) {
 			ret = handle_mm_fault(vma->vm_mm, vma, addr,
 							FAULT_FLAG_WRITE);
+			if (!(ret & (VM_FAULT_SIGBUS | VM_FAULT_OOM)
+					|| current->flags & PF_KTHREAD))
+				current->min_flt++;
+		}
 		else
 			ret = VM_FAULT_WRITE;
 		put_page(page);
-- 
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] Count minor fault in break_ksm

[Hugh Dickins]

On Mon, 11 Jan 2010, Minchan Kim wrote:

> We have counted task's maj/min fault after handle_mm_fault.
> break_ksm misses that.
> 
> I wanted to check by VM_FAULT_ERROR. 
> But now break_ksm doesn't handle HWPOISON error. 

Sorry, no, I just don't see a good reason to add this.
Imagine it this way: these aren't really faults, KSM simply
happens to be using "handle_mm_fault" to achieve what it needs.

(And, of course, if we did add something like this, I'd be
disagreeing with you about which tsk's min_flt to increment.)

Hugh

> 
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> CC: Izik Eidus <ieidus@redhat.com>
> ---
>  mm/ksm.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 56a0da1..3a1fda4 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -367,9 +367,13 @@ static int break_ksm(struct vm_area_struct *vma, unsigned long addr)
>  		page = follow_page(vma, addr, FOLL_GET);
>  		if (!page)
>  			break;
> -		if (PageKsm(page))
> +		if (PageKsm(page)) {
>  			ret = handle_mm_fault(vma->vm_mm, vma, addr,
>  							FAULT_FLAG_WRITE);
> +			if (!(ret & (VM_FAULT_SIGBUS | VM_FAULT_OOM)
> +					|| current->flags & PF_KTHREAD))
> +				current->min_flt++;
> +		}
>  		else
>  			ret = VM_FAULT_WRITE;
>  		put_page(page);
> -- 
> 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] Count minor fault in break_ksm

[Minchan Kim]

Hi, Hugh.

On Tue, Jan 12, 2010 at 8:40 AM, Hugh Dickins
<hugh.dickins@tiscali.co.uk> wrote:
> On Mon, 11 Jan 2010, Minchan Kim wrote:
>
>> We have counted task's maj/min fault after handle_mm_fault.
>> break_ksm misses that.
>>
>> I wanted to check by VM_FAULT_ERROR.
>> But now break_ksm doesn't handle HWPOISON error.
>
> Sorry, no, I just don't see a good reason to add this.
> Imagine it this way: these aren't really faults, KSM simply
> happens to be using "handle_mm_fault" to achieve what it needs.

Why I suggest is handle_mm_fault counts PGFAULT in system.
If we want to get minor fault count in system, we have to calculate
(PGFAULT - PGMAJFAULT).

But we don't count it as either major or minor in this case and GUP case
I doubt it, then I see GUP already handled it tsk->[maj|min]flt.
Although it isn't my point, I thought break_ksm also have to count it
like GUP at least.

Okay. It's not real problem. I found just while I review the code.
I have no objection in your opinion.
But I think it would be better to have a consistency PGFAULT and PGMAJFAULT.


>
> (And, of course, if we did add something like this, I'd be
> disagreeing with you about which tsk's min_flt to increment.)
>
> Hugh
>
>>
>> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>> CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
>> CC: Izik Eidus <ieidus@redhat.com>
>> ---
>>  mm/ksm.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 56a0da1..3a1fda4 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -367,9 +367,13 @@ static int break_ksm(struct vm_area_struct *vma, unsigned long addr)
>>               page = follow_page(vma, addr, FOLL_GET);
>>               if (!page)
>>                       break;
>> -             if (PageKsm(page))
>> +             if (PageKsm(page)) {
>>                       ret = handle_mm_fault(vma->vm_mm, vma, addr,
>>                                                       FAULT_FLAG_WRITE);
>> +                     if (!(ret & (VM_FAULT_SIGBUS | VM_FAULT_OOM)
>> +                                     || current->flags & PF_KTHREAD))
>> +                             current->min_flt++;
>> +             }
>>               else
>>                       ret = VM_FAULT_WRITE;
>>               put_page(page);
>> --
>> 1.5.6.3
>>
>>
>>
>> --
>> Kind regards,
>> Minchan Kim
>



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