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

[PATCH 19/20 v2] 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 v2] sparc/mm/fault_32.c: Port OOM changes to do_sparc_fault

[David Miller]

From: Kautuk Consul <consul.kautuk@gmail.com>
Date: Sun, 25 Mar 2012 13:58:35 -0400

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

Still indented wrong, it should be:

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

The second line must have the openning parenthesis line up with
the column after the openning parenthesis on the first line.  Do
not use only tabs to indent this stuff, line it up properly.

You need to use tabs and spaces to line it up.  Use something like
emacs's C-mode to help you if you can't do it right by hand.

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

Still wrong, for the same exact reason.

I went out of my way to reply to your private email (which I usually
ignore, you should never email developers privately) and you didn't
even listen to the feedback I gave you.

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

[Kautuk Consul]

On Sun, Mar 25, 2012 at 3:04 PM, David Miller <davem@davemloft.net> wrote:
> From: Kautuk Consul <consul.kautuk@gmail.com>
> Date: Sun, 25 Mar 2012 13:58:35 -0400
>
>> +     unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
>> +                                     (write ? FAULT_FLAG_WRITE : 0));
>
> Still indented wrong, it should be:
>
>        unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
>                              (write ? FAULT_FLAG_WRITE : 0));
>
> The second line must have the openning parenthesis line up with
> the column after the openning parenthesis on the first line.  Do
> not use only tabs to indent this stuff, line it up properly.
>
> You need to use tabs and spaces to line it up.  Use something like
> emacs's C-mode to help you if you can't do it right by hand.
>
>> +                     perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ,
>> +                                     1, regs, address);
>
> Still wrong, for the same exact reason.
>
> I went out of my way to reply to your private email (which I usually
> ignore, you should never email developers privately) and you didn't
> even listen to the feedback I gave you.


I use gmail which did not show me the indentation which you wanted.
I understand what you said now and have sent a v3 for this.

Re: [PATCH 19/20 v2] 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:35:56 -0400

> I use gmail which did not show me the indentation which you wanted.
> I understand what you said now and have sent a v3 for this.

No, you don't understand, it's still messed up, in fact you've
made it increasingly worse.