[PATCH 15/19 v2] tile/mm/fault.c: Port OOM changes to handle_page_fault

[PATCH 15/19 v2] tile/mm/fault.c: Port OOM changes to handle_page_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 tile.

Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 arch/tile/mm/fault.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/tile/mm/fault.c b/arch/tile/mm/fault.c
index cba30e9..bba4cd9 100644
--- a/arch/tile/mm/fault.c
+++ b/arch/tile/mm/fault.c
@@ -266,6 +266,8 @@ static int handle_page_fault(struct pt_regs *regs,
 	int si_code;
 	int is_kernel_mode;
 	pgd_t *pgd;
+	unsigned int flags = (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE |
+			      (write ? FAULT_FLAG_WRITE : 0));
 
 	/* on TILE, protection faults are always writes */
 	if (!is_page_fault)
@@ -372,6 +374,8 @@ static int handle_page_fault(struct pt_regs *regs,
 			vma = NULL;  /* happy compiler */
 			goto bad_area_nosemaphore;
 		}
+
+retry:
 		down_read(&mm->mmap_sem);
 	}
 
@@ -419,7 +423,11 @@ good_area:
 	 * make sure we exit gracefully rather than endlessly redo
 	 * the fault.
 	 */
-	fault = handle_mm_fault(mm, vma, address, write);
+	fault = handle_mm_fault(mm, vma, address, flags);
+
+	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+		return 0;
+
 	if (unlikely(fault & VM_FAULT_ERROR)) {
 		if (fault & VM_FAULT_OOM)
 			goto out_of_memory;
@@ -427,10 +435,22 @@ good_area:
 			goto do_sigbus;
 		BUG();
 	}
-	if (fault & VM_FAULT_MAJOR)
-		tsk->maj_flt++;
-	else
-		tsk->min_flt++;
+	if (flags & FAULT_FLAG_ALLOW_RETRY) {
+		if (fault & VM_FAULT_MAJOR)
+			tsk->maj_flt++;
+		else
+			tsk->min_flt++;
+		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;
+		}
+	}
 
 #if CHIP_HAS_TILE_DMA() || CHIP_HAS_SN_PROC()
 	/*
-- 
1.7.5.4

Re: [PATCH 15/19 v2] tile/mm/fault.c: Port OOM changes to handle_page_fault

[Chris Metcalf]

On 3/31/2012 8:05 AM, 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 tile.
>
> Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
> [...]
> +			 /* No need to up_read(&mm->mmap_sem) as we would
> +			 * have already released it in __lock_page_or_retry
> +			 * in mm/filemap.c.
> +			 */
>

Taken into the tile tree.  I adjusted the block comment whitespacing to
normal Linux standard.  Thanks!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

CodingStyle vs checkpatch for block comments

[Chris Metcalf]

(Adding linux-kernel to the cc's and restating the question...)

I accepted a patch from Kautuk Consul and adjusted his comment style, but
he pointed out that he had changed his comment style based on a warning
from checkpatch.

The Documentation/CodingStyle file says:

    The preferred style for long (multi-line) comments is:

       /*
        * This is the preferred style for multi-line
        * comments in the Linux kernel source code.
        * Please use it consistently.
        *
        * Description:  A column of asterisks on the left side,
        * with beginning and ending almost-blank lines.
        */

However, a new change to "checkpatch --strict" by Joe Perches, commit
aad4f61498, causes this construct to be flagged as a warning if (and only
if) it is preceded by a blank line.  Joe said the change was to support
David Miller's preferred style, but that he didn't much care one way or
another.

The relevant code in checkpatch.pl is:

                if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
                    $prevrawline =~ /^\+[ \t]*$/) {
                        CHK("BLOCK_COMMENT_STYLE",
                            "Don't begin block comments with only a /*
line, use /* comment...\n" . $hereprev);
                }

So, my questions -

1. I'm not sure what the regexps are really trying to avoid.  Presumably a
blank line followed by a block comment is OK?  Certainly the kernel sources
are full of this construct.

2. The actual warning message emitted seems to directly contradict the
CodingStyle document, so presumably we should either clarify the message,
or update CodingStyle if we're really trying to change the style.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: CodingStyle vs checkpatch for block comments

[Joe Perches]

On Tue, 2012-04-03 at 13:25 -0400, Chris Metcalf wrote:
> (Adding linux-kernel to the cc's and restating the question...)
> 
> I accepted a patch from Kautuk Consul and adjusted his comment style, but
> he pointed out that he had changed his comment style based on a warning

It's a check not a warning.

> from checkpatch.
> 
> The Documentation/CodingStyle file says:
> 
>     The preferred style for long (multi-line) comments is:
> 
>        /*
>         * This is the preferred style for multi-line
>         * comments in the Linux kernel source code.
>         * Please use it consistently.
>         *
>         * Description:  A column of asterisks on the left side,
>         * with beginning and ending almost-blank lines.
>         */
> 
> However, a new change to "checkpatch --strict" by Joe Perches, commit
> aad4f61498, causes this construct to be flagged as a warning

checkpatch flags it with a "check" only when --strict
is on the checkpatch command line.

>  if (and only
> if) it is preceded by a blank line.  Joe said the change was to support
> David Miller's preferred style, but that he didn't much care one way or
> another.

I think the block comment style is less important than
the actual content of the block comment.  The actual
comment content is rather difficult for checkpatch to
parse though.

> The relevant code in checkpatch.pl is:
> 
>                 if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>                     $prevrawline =~ /^\+[ \t]*$/) {
>                         CHK("BLOCK_COMMENT_STYLE",
>                             "Don't begin block comments with only a /*
> line, use /* comment...\n" . $hereprev);
>                 }
> 
> So, my questions -
> 
> 1. I'm not sure what the regexps are really trying to avoid.  Presumably a
> blank line followed by a block comment is OK?  Certainly the kernel sources
> are full of this construct.

It emits a check message on
<blank line>
	/*

but not
<blank line>
	/* some actual comment

> 2. The actual warning message emitted seems to directly contradict the
> CodingStyle document, so presumably we should either clarify the message,
> or update CodingStyle if we're really trying to change the style.

Or just remove it or add a test for the patched file
to be in net/... or drivers/net... or something.

Re: CodingStyle vs checkpatch for block comments

[Chris Metcalf]

On 4/3/2012 1:41 PM, Joe Perches wrote:
> On Tue, 2012-04-03 at 13:25 -0400, Chris Metcalf wrote:
>> The relevant code in checkpatch.pl is:
>>
>>                 if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>>                     $prevrawline =~ /^\+[ \t]*$/) {
>>                         CHK("BLOCK_COMMENT_STYLE",
>>                             "Don't begin block comments with only a /*
>> line, use /* comment...\n" . $hereprev);
>>                 }
>>
>> So, my questions -
>>
>> 1. I'm not sure what the regexps are really trying to avoid.  Presumably a
>> blank line followed by a block comment is OK?  Certainly the kernel sources
>> are full of this construct.
> It emits a check message on
> <blank line>
> 	/*
>
> but not
> <blank line>
> 	/* some actual comment

Right, I understand what the regexps do, I'm just not clear on what the
rationale is.  Is it trying to ensure that multi-line block comments are
never preceded by a blank line?  Is it trying to change the format of block
comments such that they either are preceded by a blank line, or a
standalone "/*", but not both?  Confusing.

>> 2. The actual warning message emitted seems to directly contradict the
>> CodingStyle document, so presumably we should either clarify the message,
>> or update CodingStyle if we're really trying to change the style.
> Or just remove it or add a test for the patched file
> to be in net/... or drivers/net... or something.

Obviously removing it would be an easy fix. :-)  I don't know if it makes
sense to advocate for different kernel comment styles in different subtrees.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: CodingStyle vs checkpatch for block comments

[Joe Perches]

On Tue, 2012-04-03 at 14:08 -0400, Chris Metcalf wrote:
> On 4/3/2012 1:41 PM, Joe Perches wrote:
> > On Tue, 2012-04-03 at 13:25 -0400, Chris Metcalf wrote:
> >> The relevant code in checkpatch.pl is:
> >>
> >>                 if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
> >>                     $prevrawline =~ /^\+[ \t]*$/) {
> >>                         CHK("BLOCK_COMMENT_STYLE",
> >>                             "Don't begin block comments with only a /*
> >> line, use /* comment...\n" . $hereprev);
> >>                 }
> >>
> >> So, my questions -
> >>
> >> 1. I'm not sure what the regexps are really trying to avoid.  Presumably a
> >> blank line followed by a block comment is OK?  Certainly the kernel sources
> >> are full of this construct.
> > It emits a check message on
> > <blank line>
> > 	/*
> >
> > but not
> > <blank line>
> > 	/* some actual comment
> 
> Right, I understand what the regexps do, I'm just not clear on what the
> rationale is.

Fewer vertical lines for block comments.

This block comment uses a lot of lines:

		some_code();
	}

	/*
	 * Some multiline
	 * block comment
	 */

	some_more_code();

This is 1 fewer line

		some_code();
	}

	/* Some multline
	 * block comment
	 */

	some_more_code();

>   Is it trying to ensure that multi-line block comments are
> never preceded by a blank line?

No.

> Is it trying to change the format of block
> comments such that they either are preceded by a blank line, or a
> standalone "/*", but not both?  Confusing.

It's suggesting that a multi line comment block
starting with only a /* wastes space on vertically
challenged terminals.

Some of my friends are also vertically challenged,
so I understand the desire to be efficient.

> Obviously removing it would be an easy fix. :-)  I don't know if it makes
> sense to advocate for different kernel comment styles in different subtrees.

<shrug>  I don't care much either way.

The content of the comment is more important.

Re: CodingStyle vs checkpatch for block comments

[Chris Metcalf]

On 4/3/2012 2:16 PM, Joe Perches wrote:
> On Tue, 2012-04-03 at 14:08 -0400, Chris Metcalf wrote:
>> On 4/3/2012 1:41 PM, Joe Perches wrote:
>>> On Tue, 2012-04-03 at 13:25 -0400, Chris Metcalf wrote:
>>>> The relevant code in checkpatch.pl is:
>>>>
>>>>                 if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>>>>                     $prevrawline =~ /^\+[ \t]*$/) {
>>>>                         CHK("BLOCK_COMMENT_STYLE",
>>>>                             "Don't begin block comments with only a /*
>>>> line, use /* comment...\n" . $hereprev);
>>>>                 }
>>>>
>>>> So, my questions -
>>>>
>>>> 1. I'm not sure what the regexps are really trying to avoid.  Presumably a
>>>> blank line followed by a block comment is OK?  Certainly the kernel sources
>>>> are full of this construct.
>>> It emits a check message on
>>> <blank line>
>>> 	/*
>>>
>>> but not
>>> <blank line>
>>> 	/* some actual comment
>> Right, I understand what the regexps do, I'm just not clear on what the
>> rationale is.
> Fewer vertical lines for block comments.
>
> This block comment uses a lot of lines:
>
> 		some_code();
> 	}
>
> 	/*
> 	 * Some multiline
> 	 * block comment
> 	 */
>
> 	some_more_code();
>
> This is 1 fewer line
>
> 		some_code();
> 	}
>
> 	/* Some multline
> 	 * block comment
> 	 */
>
> 	some_more_code();
>
> [....]
> It's suggesting that a multi line comment block
> starting with only a /* wastes space on vertically
> challenged terminals.

So I suggest we drop the check from checkpatch, then, since it appears to
conflict with Documentation/CodingStyle.
-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com