[PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Kautuk Consul]

Commit d065bd810b6deb67d4897a14bfe21f8eb526ba99
(mm: retry page fault when blocking on disk transfer) and
commit 37b23e0525d393d48a7d59f870b3bc061a30ccdb
(x86,mm: make pagefault killable)

The above commits introduced changes into the x86 pagefault handler
for making the page fault handler retryable as well as killable.

These changes reduce the mmap_sem hold time, which is crucial
during OOM killer invocation.

Port these changes to 32-bit sparc.

Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 arch/sparc/mm/fault_32.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index 8023fd7..6da1818 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -226,6 +226,8 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 	unsigned long g2;
 	int from_user = !(regs->psr & PSR_PS);
 	int fault, code;
+	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
+						  (write ? FAULT_FLAG_WRITE : 0));
 
 	if(text_fault)
 		address = regs->pc;
@@ -252,6 +254,7 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
+retry:
 	down_read(&mm->mmap_sem);
 
 	/*
@@ -290,7 +293,11 @@ good_area:
 	 * make sure we exit gracefully rather than endlessly redo
 	 * the fault.
 	 */
-	fault = handle_mm_fault(mm, vma, address, write ? FAULT_FLAG_WRITE : 0);
+	fault = handle_mm_fault(mm, vma, address, flags);
+
+	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+		return;
+
 	if (unlikely(fault & VM_FAULT_ERROR)) {
 		if (fault & VM_FAULT_OOM)
 			goto out_of_memory;
@@ -298,13 +305,29 @@ good_area:
 			goto do_sigbus;
 		BUG();
 	}
-	if (fault & VM_FAULT_MAJOR) {
-		current->maj_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, regs, address);
-	} else {
-		current->min_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, regs, address);
+
+	if (flags & FAULT_FLAG_ALLOW_RETRY) {
+		if (fault & VM_FAULT_MAJOR) {
+			current->maj_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ,
+						  1, regs, address);
+		} else {
+			current->min_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,
+						  1, regs, address);
+		}
+		if (fault & VM_FAULT_RETRY) {
+			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+
+			/* No need to up_read(&mm->mmap_sem) as we would
+			 * have already released it in __lock_page_or_retry
+			 * in mm/filemap.c.
+			 */
+
+			goto retry;
+		}
 	}
+
 	up_read(&mm->mmap_sem);
 	return;
 
-- 
1.7.5.4

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Joe Perches]

On Mon, 2012-03-26 at 11:32 -0400, Kautuk Consul wrote:
[]
> diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
> index 8023fd7..6da1818 100644
> --- a/arch/sparc/mm/fault_32.c
> +++ b/arch/sparc/mm/fault_32.c
> @@ -226,6 +226,8 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
>  	unsigned long g2;
>  	int from_user = !(regs->psr & PSR_PS);
>  	int fault, code;
> +	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
> +						  (write ? FAULT_FLAG_WRITE : 0));

David meant that the indentation should be aligned
immediately after the open parenthesis:

+	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
+			      (write ? FAULT_FLAG_WRITE : 0));

though perhaps this is clearer as:

	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;

	if (write)
		flags |= FAULT_FLAG_WRITE;

or even separate sets.

	if (write)
		flags = foo;
	else
		flags = bar;

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Kautuk Consul]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

> David meant that the indentation should be aligned
> immediately after the open parenthesis:
>
> +       unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
> +                             (write ? FAULT_FLAG_WRITE : 0));
>
> though perhaps this is clearer as:
>
>        unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>
>        if (write)
>                flags |= FAULT_FLAG_WRITE;
>
> or even separate sets.
>
>        if (write)
>                flags = foo;
>        else
>                flags = bar;

This is how it is in the patch file(immediately after the paramthesis).
I have now attached both sparc patches to this email.

you must be having the same problem in viewing the patch file
as me due to the email client itself inserting invalid spaces here and there.

by the way, what tab-spaces are you using when you open it in vim ?

[-- Attachment #2: 0019-sparc-mm-fault_32.c-Port-OOM-changes-to-do_page_faul.patch --]
[-- Type: text/x-patch, Size: 2869 bytes --]

From a95d00b2293d41a5afa193e2045c3d8bb81bbc4a Mon Sep 17 00:00:00 2001
From: Kautuk Consul <consul.kautuk@gmail.com>
Date: Sun, 25 Mar 2012 13:50:30 -0400
Subject: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

Commit d065bd810b6deb67d4897a14bfe21f8eb526ba99
(mm: retry page fault when blocking on disk transfer) and
commit 37b23e0525d393d48a7d59f870b3bc061a30ccdb
(x86,mm: make pagefault killable)

The above commits introduced changes into the x86 pagefault handler
for making the page fault handler retryable as well as killable.

These changes reduce the mmap_sem hold time, which is crucial
during OOM killer invocation.

Port these changes to 32-bit sparc.

Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 arch/sparc/mm/fault_32.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index 8023fd7..6da1818 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -226,6 +226,8 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 	unsigned long g2;
 	int from_user = !(regs->psr & PSR_PS);
 	int fault, code;
+	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
+						  (write ? FAULT_FLAG_WRITE : 0));
 
 	if(text_fault)
 		address = regs->pc;
@@ -252,6 +254,7 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
+retry:
 	down_read(&mm->mmap_sem);
 
 	/*
@@ -290,7 +293,11 @@ good_area:
 	 * make sure we exit gracefully rather than endlessly redo
 	 * the fault.
 	 */
-	fault = handle_mm_fault(mm, vma, address, write ? FAULT_FLAG_WRITE : 0);
+	fault = handle_mm_fault(mm, vma, address, flags);
+
+	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+		return;
+
 	if (unlikely(fault & VM_FAULT_ERROR)) {
 		if (fault & VM_FAULT_OOM)
 			goto out_of_memory;
@@ -298,13 +305,29 @@ good_area:
 			goto do_sigbus;
 		BUG();
 	}
-	if (fault & VM_FAULT_MAJOR) {
-		current->maj_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, regs, address);
-	} else {
-		current->min_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, regs, address);
+
+	if (flags & FAULT_FLAG_ALLOW_RETRY) {
+		if (fault & VM_FAULT_MAJOR) {
+			current->maj_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ,
+						  1, regs, address);
+		} else {
+			current->min_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,
+						  1, regs, address);
+		}
+		if (fault & VM_FAULT_RETRY) {
+			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+
+			/* No need to up_read(&mm->mmap_sem) as we would
+			 * have already released it in __lock_page_or_retry
+			 * in mm/filemap.c.
+			 */
+
+			goto retry;
+		}
 	}
+
 	up_read(&mm->mmap_sem);
 	return;
 
-- 
1.7.5.4


[-- Attachment #3: 0020-sparc-mm-fault_64.c-Port-OOM-changes-to-do_page_faul.patch --]
[-- Type: text/x-patch, Size: 2856 bytes --]

From 3f4a3166b5baff9d043fd39f0f0c2c8044c85cce Mon Sep 17 00:00:00 2001
From: Kautuk Consul <consul.kautuk@gmail.com>
Date: Mon, 26 Mar 2012 11:28:50 -0400
Subject: [PATCH 20/20 v3] sparc/mm/fault_64.c: Port OOM changes to
 do_sparc64_fault

Commit d065bd810b6deb67d4897a14bfe21f8eb526ba99
(mm: retry page fault when blocking on disk transfer) and
commit 37b23e0525d393d48a7d59f870b3bc061a30ccdb
(x86,mm: make pagefault killable)

The above commits introduced changes into the x86 pagefault handler
for making the page fault handler retryable as well as killable.

These changes reduce the mmap_sem hold time, which is crucial
during OOM killer invocation.

Port these changes to 64-bit sparc.

Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 arch/sparc/mm/fault_64.c |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index 504c062..8322962 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -279,6 +279,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 	unsigned int insn = 0;
 	int si_code, fault_code, fault;
 	unsigned long address, mm_rss;
+	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
 
 	fault_code = get_thread_fault_code();
 
@@ -333,6 +334,8 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 			insn = get_fault_insn(regs, insn);
 			goto handle_kernel_fault;
 		}
+
+retry:
 		down_read(&mm->mmap_sem);
 	}
 
@@ -423,7 +426,13 @@ good_area:
 			goto bad_area;
 	}
 
-	fault = handle_mm_fault(mm, vma, address, (fault_code & FAULT_CODE_WRITE) ? FAULT_FLAG_WRITE : 0);
+	fault = handle_mm_fault(mm, vma, address,
+							flags | ((fault_code & FAULT_CODE_WRITE) ?
+									 FAULT_FLAG_WRITE : 0);
+
+	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+		return;
+
 	if (unlikely(fault & VM_FAULT_ERROR)) {
 		if (fault & VM_FAULT_OOM)
 			goto out_of_memory;
@@ -431,12 +440,27 @@ good_area:
 			goto do_sigbus;
 		BUG();
 	}
-	if (fault & VM_FAULT_MAJOR) {
-		current->maj_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, regs, address);
-	} else {
-		current->min_flt++;
-		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, regs, address);
+
+	if (flags & FAULT_FLAG_ALLOW_RETRY) {
+		if (fault & VM_FAULT_MAJOR) {
+			current->maj_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ,
+						  1, regs, address);
+		} else {
+			current->min_flt++;
+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,
+						  1, regs, address);
+		}
+		if (fault & VM_FAULT_RETRY) {
+			flags &= ~FAULT_FLAG_ALLOW_RETRY;
+
+			/* No need to up_read(&mm->mmap_sem) as we would
+			 * have already released it in __lock_page_or_retry
+			 * in mm/filemap.c.
+			 */
+
+			goto retry;
+		}
 	}
 	up_read(&mm->mmap_sem);
 
-- 
1.7.5.4

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Joe Perches]

On Mon, 2012-03-26 at 12:20 -0400, Kautuk Consul wrote:
> > David meant that the indentation should be aligned
> > immediately after the open parenthesis:
> >
> > +       unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
> > +                             (write ? FAULT_FLAG_WRITE : 0));
> >
> > though perhaps this is clearer as:
> >
> >        unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> >
> >        if (write)
> >                flags |= FAULT_FLAG_WRITE;
> >
> > or even separate sets.
> >
> >        if (write)
> >                flags = foo;
> >        else
> >                flags = bar;
> 
> This is how it is in the patch file(immediately after the paramthesis).
> I have now attached both sparc patches to this email.
> 
> you must be having the same problem in viewing the patch file
> as me due to the email client itself inserting invalid spaces here and there.

Nope.  That's not it.

> by the way, what tab-spaces are you using when you open it in vim ?

I use vim as an editor as infrequently as possible.

Your original patch used 6 tabs and 2 spaces.
Tab size in kernel style is always 8.

Again, make sure your tab indentation is set to 8.

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Sam Ravnborg]

On Mon, Mar 26, 2012 at 11:32:26AM -0400, Kautuk Consul wrote:
> Commit d065bd810b6deb67d4897a14bfe21f8eb526ba99
> (mm: retry page fault when blocking on disk transfer) and
> commit 37b23e0525d393d48a7d59f870b3bc061a30ccdb
> (x86,mm: make pagefault killable)
> 
> The above commits introduced changes into the x86 pagefault handler
> for making the page fault handler retryable as well as killable.
> 
> These changes reduce the mmap_sem hold time, which is crucial
> during OOM killer invocation.
> 
> Port these changes to 32-bit sparc.
> 
> Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
> ---
>  arch/sparc/mm/fault_32.c |   37 ++++++++++++++++++++++++++++++-------
>  1 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
> index 8023fd7..6da1818 100644
> --- a/arch/sparc/mm/fault_32.c
> +++ b/arch/sparc/mm/fault_32.c
> @@ -226,6 +226,8 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
>  	unsigned long g2;
>  	int from_user = !(regs->psr & PSR_PS);
>  	int fault, code;
> +	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
> +						  (write ? FAULT_FLAG_WRITE : 0));
Hi Kautuk.

The indention still look like shit.
It looks like this (tabs converted to spaces):
        unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
                                          (write ? FAULT_FLAG_WRITE : 0));


	Sam

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Kautuk Consul]

>>
>> you must be having the same problem in viewing the patch file
>> as me due to the email client itself inserting invalid spaces here and there.
>
> Nope.  That's not it.
>
>> by the way, what tab-spaces are you using when you open it in vim ?
>
> I use vim as an editor as infrequently as possible.
>
> Your original patch used 6 tabs and 2 spaces.
> Tab size in kernel style is always 8.
>
> Again, make sure your tab indentation is set to 8.
>

I see what you are saying.
One question: Is it possible to include this kinda checking
(multiline statements with paranthesis checking assuming tab
indentation 8) in the
checkpatch.pl script ?
It would be easier to not make such mistakes in future with that kind of tool.

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Joe Perches]

On Mon, 2012-03-26 at 12:55 -0400, Kautuk Consul wrote:
> Is it possible to include this kinda checking
> (multiline statements with paranthesis checking assuming tab
> indentation 8) in the
> checkpatch.pl script ?
> It would be easier to not make such mistakes in future with that kind of tool.

Funny you should ask...

Use the latest version of checkpatch and
add the --strict option to the command line.

https://lkml.org/lkml/2012/3/2/15

It works for most cases.

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[Kautuk Consul]

On Mon, Mar 26, 2012 at 1:03 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2012-03-26 at 12:55 -0400, Kautuk Consul wrote:
>> Is it possible to include this kinda checking
>> (multiline statements with paranthesis checking assuming tab
>> indentation 8) in the
>> checkpatch.pl script ?
>> It would be easier to not make such mistakes in future with that kind of tool.
>
> Funny you should ask...
>
> Use the latest version of checkpatch and
> add the --strict option to the command line.
>
> https://lkml.org/lkml/2012/3/2/15
>
> It works for most cases.
>

Ah yes, I am not in the habit of using the --strict option.
I will use that in future, sorry for my ignorance.

And thanks to all you guys for helping me out here. :)

Re: [PATCH 19/20 v3] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[David Miller]

From: Kautuk Consul <consul.kautuk@gmail.com>
Date: Mon, 26 Mar 2012 11:32:26 -0400

> +	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
> +						  (write ? FAULT_FLAG_WRITE : 0));

Indentation is still wrong.  The "(write ..." on the second line must
line up with the column of the first "F" in "(FAULT_FLAG..." on the
first line.

+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ,
+						  1, regs, address);

+			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,
+						  1, regs, address);

These are still wrong too, for the same reason.

You didn't fix anything.