Re: [PATCH v2] eeprom: at25: Add DT support for EEPROMs with odd address bits
2017-12-12 21:54 GMT+01:00 Rob Herring <r...@kernel.org>: > On Fri, Dec 08, 2017 at 02:46:41PM +0100, Geert Uytterhoeven wrote: >> Certain EEPROMS have a size that is larger than the number of address >> bytes would allow, and store the MSB of the address in bit 3 of the >> instruction byte. >> >> This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or >> in DT using the obsolete legacy "at25,addr-mode" property. >> But currently there exists no non-deprecated way to describe this in DT. >> >> Hence extend the existing "address-width" DT property to allow >> specifying 9 address bits, and enable support for that in the driver. >> >> This has been tested with a Microchip 25LC040A. >> >> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be> >> --- >> v2: >> - Do not consider odd address widths of 17 or 25 bits, >> - Move handling inside the switch() statement. >> --- >> Documentation/devicetree/bindings/eeprom/at25.txt | 4 +++- >> drivers/misc/eeprom/at25.c| 3 +++ >> 2 files changed, 6 insertions(+), 1 deletion(-) > > Reviewed-by: Rob Herring <r...@kernel.org> Reviewed-by: Ivo Sieben <meltedpiano...@gmail.com>
Re: [PATCH v2] eeprom: at25: Add DT support for EEPROMs with odd address bits
2017-12-12 21:54 GMT+01:00 Rob Herring : > On Fri, Dec 08, 2017 at 02:46:41PM +0100, Geert Uytterhoeven wrote: >> Certain EEPROMS have a size that is larger than the number of address >> bytes would allow, and store the MSB of the address in bit 3 of the >> instruction byte. >> >> This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or >> in DT using the obsolete legacy "at25,addr-mode" property. >> But currently there exists no non-deprecated way to describe this in DT. >> >> Hence extend the existing "address-width" DT property to allow >> specifying 9 address bits, and enable support for that in the driver. >> >> This has been tested with a Microchip 25LC040A. >> >> Signed-off-by: Geert Uytterhoeven >> --- >> v2: >> - Do not consider odd address widths of 17 or 25 bits, >> - Move handling inside the switch() statement. >> --- >> Documentation/devicetree/bindings/eeprom/at25.txt | 4 +++- >> drivers/misc/eeprom/at25.c| 3 +++ >> 2 files changed, 6 insertions(+), 1 deletion(-) > > Reviewed-by: Rob Herring Reviewed-by: Ivo Sieben
Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits
Hi Geert,
My 2 cents:
2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven <ge...@linux-m68k.org>:
>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>> include/linux/spi/eeprom.h, really exist?
>> Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.
> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> So I tend to believe EEPROMs using 16 + 1 or 24 + 1 address bits (with the
> extra bit in the instruction byte) do not exist?
>
I think you are right. Most likely this extra address bit option is
only used for 9 bit addressable chips.
I'm not an expert, but I know only the M95040 chip for which I
originally wrote the patch.
By then I decided to make it a bit broader (so also to be used as
address 17 & 25 bit addressing) but that might
not make any sense indeed.
>> @@ -6,7 +6,9 @@ Required properties:
>> - spi-max-frequency : max spi frequency to use
>> - pagesize : size of the eeprom page
>> - size : total eeprom size in bytes
>> -- address-width : number of address bits (one of 8, 16, or 24)
>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>> + For odd values, the MSB of the address is sent as bit 3 of the instruction
>> + byte, before the address byte(s).
>
> Alternatively, we can drop the binding change, i.e. keep on using
> address-width = <8> for 512-byte '040...
>
As you also stated before: maybe it is more clear to leave only the
"9" value option documented
here, that looks to me the only valid use case for it.
>> + if (val & 1) {
>> + chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>> + val -= 1;
>> + }
>
> ... and handle it here like:
>
> if (chip->byte_len == 2U << val)
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>
> However, that would IMHO be a bit confusing, as the "address-width"
> property is no longer the real address width, but indicates how many bits
> are specified in address bytes sent after the read/write command.
> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>
> Or deprecate this whole "specify parameters using DT properties" business,
> and derive them from the compatible value. But that would mean adding a
> large and ever growing table to an old driver...
>
> Thoughts?
I'm not a DT expert but to me your first proposal makes the most sense
to me and feels the most intuitive:
I would go for the address-with value 9 option here.
Since we only expect value 9 to be a valid option, maybe you could
rewrite it a bit to explicitly check for value 9:
if (val == 9) {
chip->flags |= EE_INSTR_BIT3_IS_ADDR;
val -= 1;
}
I think this is slightly more readable.
Hope this helps,
Regards,
Ivo Sieben
Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits
Hi Geert,
My 2 cents:
2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven :
>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>> include/linux/spi/eeprom.h, really exist?
>> Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.
> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> So I tend to believe EEPROMs using 16 + 1 or 24 + 1 address bits (with the
> extra bit in the instruction byte) do not exist?
>
I think you are right. Most likely this extra address bit option is
only used for 9 bit addressable chips.
I'm not an expert, but I know only the M95040 chip for which I
originally wrote the patch.
By then I decided to make it a bit broader (so also to be used as
address 17 & 25 bit addressing) but that might
not make any sense indeed.
>> @@ -6,7 +6,9 @@ Required properties:
>> - spi-max-frequency : max spi frequency to use
>> - pagesize : size of the eeprom page
>> - size : total eeprom size in bytes
>> -- address-width : number of address bits (one of 8, 16, or 24)
>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>> + For odd values, the MSB of the address is sent as bit 3 of the instruction
>> + byte, before the address byte(s).
>
> Alternatively, we can drop the binding change, i.e. keep on using
> address-width = <8> for 512-byte '040...
>
As you also stated before: maybe it is more clear to leave only the
"9" value option documented
here, that looks to me the only valid use case for it.
>> + if (val & 1) {
>> + chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>> + val -= 1;
>> + }
>
> ... and handle it here like:
>
> if (chip->byte_len == 2U << val)
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>
> However, that would IMHO be a bit confusing, as the "address-width"
> property is no longer the real address width, but indicates how many bits
> are specified in address bytes sent after the read/write command.
> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>
> Or deprecate this whole "specify parameters using DT properties" business,
> and derive them from the compatible value. But that would mean adding a
> large and ever growing table to an old driver...
>
> Thoughts?
I'm not a DT expert but to me your first proposal makes the most sense
to me and feels the most intuitive:
I would go for the address-with value 9 option here.
Since we only expect value 9 to be a valid option, maybe you could
rewrite it a bit to explicitly check for value 9:
if (val == 9) {
chip->flags |= EE_INSTR_BIT3_IS_ADDR;
val -= 1;
}
I think this is slightly more readable.
Hope this helps,
Regards,
Ivo Sieben
Re: [PATCH] [checkpatch.pl] Suspicious indentation detection after conditional statement
Andy, Joe,
What do you think about my patchset below?
Regards,
Ivo Sieben
2014-05-15 16:43 GMT+02:00 Ivo Sieben :
> Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
> after a conditional statement. This can be used to find missing braces or
> wrong indentation in/after a conditional statement.
>
> For example the following error is caught;
>
> if (foo)
> bar();
> return;
>
> Which can be either intended by the programmer as:
>
> if (foo)
> bar();
> return;
>
> or
> if (foo) {
> bar();
> return;
> }
>
> Signed-off-by: Ivo Sieben
> ---
>
> Request for comments:
> I think this is a nice warning to have, since you can get into a buggy
> situation
> when you want to add extra statements to a one line condition, but you forget
> to add the {} around the multiline statement block.
>
> scripts/checkpatch.pl | 41 +
> 1 file changed, 41 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e7bca89..3ca3923 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2565,6 +2565,47 @@ sub process {
> WARN("SUSPECT_CODE_INDENT",
> "suspect code indent for conditional
> statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
> }
> +
> +# Also check if the next statement after the previous condition has the same
> indent
> + my ($stat_next, undef, $line_nr_next_next) =
> + ctx_statement_block($line_nr_next,
> $remain_next, $off_next);
> + my $s_next = $stat_next;
> +
> + # Remove line prefixes
> + $s_next =~ s/\n./\n/gs;
> +
> + # Remove any comments
> + $s_next =~ s/$;//g;
> +
> + # Skip this check for in case next statement starts
> with 'else'
> + if ($s_next !~ /\s*\belse\b/) {
> +
> + # Remove while that belongs to a do {} while
> + if ($stat =~ /\bdo\b/) {
> + $s_next =~
> s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
> + }
> +
> + # Remove blank lines
> + $s_next =~ s/\s*\\?\n//g;
> +
> + # Get the real next lines
> + my $next_nof_lines = $line_nr_next_next -
> $line_nr_next;
> + my $stat_next_real = raw_line($line_nr_next,
> $next_nof_lines);
> + if (!defined($stat_next_real)) {
> + $stat_next_real = "";
> + } elsif ($next_nof_lines > 1) {
> + $stat_next_real =
> "[...]\n$stat_next_real";
> + }
> + my (undef, $nindent) = line_stats('+' .
> $s_next);
> +
> + #print "stat_next<$stat_next> stat<$stat>
> indent<$indent> nindent<$nindent> s_next<$s_next>
> stat_next_real<$stat_next_real>\n";
> +
> + if ($nindent > $indent) {
> + WARN("SUSPICIOUS_CODE_INDENT",
> +"suspicious code indentation
> after conditional statements\n" .
> +$herecurr .
> "$stat_real\n$stat_next_real\n");
> + }
> + }
> }
>
> # Track the 'values' across context and added lines.
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
Andy, Joe,
What do you think about my patchset below?
Regards,
Ivo Sieben
2014-05-15 16:43 GMT+02:00 Ivo Sieben :
> When picking up a complete statement block #if/#else/#endif prepocesor
> boundaries are taken into account by pushing current level & type on a stack.
> But on an #else the level was read from stack again (without actually popping
> it
> from stack) causing the statement block to end too early on the next ';'.
> Fixed this.
>
> For example the following code:
>
> if (!test()) {
> #ifdef NEVER
> foo();
> bar();
> #else
> bar();
> foo();
> #endif
> }
>
> Results in statement block:
>
> STATEMENT<+if (!test()) {
> +#ifdef NEVER
> + foo();
> + bar();
> +#else
> + bar();>
> CONDITION<+if (!test())>
>
> While you would expect:
>
> STATEMENT<+if (!test()) {
> +#ifdef NEVER
> + foo();
> + bar();
> +#else
> + bar();
> + foo();
> +#endif
> + }>
> CONDITION<+ if (!test())>
>
> Signed-off-by: Ivo Sieben
> ---
>
> Request for comments:
> I think I fixed a problem here that I encountered while I was working on
> another
> changeset in which I check the statement block after a condition.
> Somehow the statement block did not contain everything I expected.
>
> scripts/checkpatch.pl |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 34eb216..e7bca89 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -878,7 +878,7 @@ sub ctx_statement_block {
> if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
> push(@stack, [ $type, $level ]);
> } elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
> - ($type, $level) = @{$stack[$#stack - 1]};
> + # no changes to stack: type & level remain the same
> } elsif ($remainder =~ /^#\s*endif\b/) {
> ($type, $level) = @{pop(@stack)};
> }
> @@ -1050,7 +1050,7 @@ sub ctx_block_get {
> if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
> push(@stack, $level);
> } elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
> - $level = $stack[$#stack - 1];
> + # no changes to stack: type & level remain the same
> } elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
> $level = pop(@stack);
> }
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
Andy, Joe,
What do you think about my patchset below?
Regards,
Ivo Sieben
2014-05-15 16:43 GMT+02:00 Ivo Sieben meltedpiano...@gmail.com:
When picking up a complete statement block #if/#else/#endif prepocesor
boundaries are taken into account by pushing current level type on a stack.
But on an #else the level was read from stack again (without actually popping
it
from stack) causing the statement block to end too early on the next ';'.
Fixed this.
For example the following code:
if (!test()) {
#ifdef NEVER
foo();
bar();
#else
bar();
foo();
#endif
}
Results in statement block:
STATEMENT+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();
CONDITION+if (!test())
While you would expect:
STATEMENT+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();
+ foo();
+#endif
+ }
CONDITION+ if (!test())
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
Request for comments:
I think I fixed a problem here that I encountered while I was working on
another
changeset in which I check the statement block after a condition.
Somehow the statement block did not contain everything I expected.
scripts/checkpatch.pl |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb216..e7bca89 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -878,7 +878,7 @@ sub ctx_statement_block {
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
- ($type, $level) = @{$stack[$#stack - 1]};
+ # no changes to stack: type level remain the same
} elsif ($remainder =~ /^#\s*endif\b/) {
($type, $level) = @{pop(@stack)};
}
@@ -1050,7 +1050,7 @@ sub ctx_block_get {
if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, $level);
} elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
- $level = $stack[$#stack - 1];
+ # no changes to stack: type level remain the same
} elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
$level = pop(@stack);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [checkpatch.pl] Suspicious indentation detection after conditional statement
Andy, Joe,
What do you think about my patchset below?
Regards,
Ivo Sieben
2014-05-15 16:43 GMT+02:00 Ivo Sieben meltedpiano...@gmail.com:
Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.
For example the following error is caught;
if (foo)
bar();
return;
Which can be either intended by the programmer as:
if (foo)
bar();
return;
or
if (foo) {
bar();
return;
}
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
Request for comments:
I think this is a nice warning to have, since you can get into a buggy
situation
when you want to add extra statements to a one line condition, but you forget
to add the {} around the multiline statement block.
scripts/checkpatch.pl | 41 +
1 file changed, 41 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e7bca89..3ca3923 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2565,6 +2565,47 @@ sub process {
WARN(SUSPECT_CODE_INDENT,
suspect code indent for conditional
statements ($indent, $sindent)\n . $herecurr . $stat_real\n);
}
+
+# Also check if the next statement after the previous condition has the same
indent
+ my ($stat_next, undef, $line_nr_next_next) =
+ ctx_statement_block($line_nr_next,
$remain_next, $off_next);
+ my $s_next = $stat_next;
+
+ # Remove line prefixes
+ $s_next =~ s/\n./\n/gs;
+
+ # Remove any comments
+ $s_next =~ s/$;//g;
+
+ # Skip this check for in case next statement starts
with 'else'
+ if ($s_next !~ /\s*\belse\b/) {
+
+ # Remove while that belongs to a do {} while
+ if ($stat =~ /\bdo\b/) {
+ $s_next =~
s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+ }
+
+ # Remove blank lines
+ $s_next =~ s/\s*\\?\n//g;
+
+ # Get the real next lines
+ my $next_nof_lines = $line_nr_next_next -
$line_nr_next;
+ my $stat_next_real = raw_line($line_nr_next,
$next_nof_lines);
+ if (!defined($stat_next_real)) {
+ $stat_next_real = ;
+ } elsif ($next_nof_lines 1) {
+ $stat_next_real =
[...]\n$stat_next_real;
+ }
+ my (undef, $nindent) = line_stats('+' .
$s_next);
+
+ #print stat_next$stat_next stat$stat
indent$indent nindent$nindent s_next$s_next
stat_next_real$stat_next_real\n;
+
+ if ($nindent $indent) {
+ WARN(SUSPICIOUS_CODE_INDENT,
+suspicious code indentation
after conditional statements\n .
+$herecurr .
$stat_real\n$stat_next_real\n);
+ }
+ }
}
# Track the 'values' across context and added lines.
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
When picking up a complete statement block #if/#else/#endif prepocesor
boundaries are taken into account by pushing current level & type on a stack.
But on an #else the level was read from stack again (without actually popping it
from stack) causing the statement block to end too early on the next ';'.
Fixed this.
For example the following code:
if (!test()) {
#ifdef NEVER
foo();
bar();
#else
bar();
foo();
#endif
}
Results in statement block:
STATEMENT<+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();>
CONDITION<+if (!test())>
While you would expect:
STATEMENT<+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();
+ foo();
+#endif
+ }>
CONDITION<+ if (!test())>
Signed-off-by: Ivo Sieben
---
Request for comments:
I think I fixed a problem here that I encountered while I was working on another
changeset in which I check the statement block after a condition.
Somehow the statement block did not contain everything I expected.
scripts/checkpatch.pl |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb216..e7bca89 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -878,7 +878,7 @@ sub ctx_statement_block {
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
- ($type, $level) = @{$stack[$#stack - 1]};
+ # no changes to stack: type & level remain the same
} elsif ($remainder =~ /^#\s*endif\b/) {
($type, $level) = @{pop(@stack)};
}
@@ -1050,7 +1050,7 @@ sub ctx_block_get {
if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, $level);
} elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
- $level = $stack[$#stack - 1];
+ # no changes to stack: type & level remain the same
} elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
$level = pop(@stack);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [checkpatch.pl] Suspicious indentation detection after conditional statement
Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.
For example the following error is caught;
if (foo)
bar();
return;
Which can be either intended by the programmer as:
if (foo)
bar();
return;
or
if (foo) {
bar();
return;
}
Signed-off-by: Ivo Sieben
---
Request for comments:
I think this is a nice warning to have, since you can get into a buggy situation
when you want to add extra statements to a one line condition, but you forget
to add the {} around the multiline statement block.
scripts/checkpatch.pl | 41 +
1 file changed, 41 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e7bca89..3ca3923 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2565,6 +2565,47 @@ sub process {
WARN("SUSPECT_CODE_INDENT",
"suspect code indent for conditional
statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
}
+
+# Also check if the next statement after the previous condition has the same
indent
+ my ($stat_next, undef, $line_nr_next_next) =
+ ctx_statement_block($line_nr_next,
$remain_next, $off_next);
+ my $s_next = $stat_next;
+
+ # Remove line prefixes
+ $s_next =~ s/\n./\n/gs;
+
+ # Remove any comments
+ $s_next =~ s/$;//g;
+
+ # Skip this check for in case next statement starts
with 'else'
+ if ($s_next !~ /\s*\belse\b/) {
+
+ # Remove while that belongs to a do {} while
+ if ($stat =~ /\bdo\b/) {
+ $s_next =~
s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+ }
+
+ # Remove blank lines
+ $s_next =~ s/\s*\\?\n//g;
+
+ # Get the real next lines
+ my $next_nof_lines = $line_nr_next_next -
$line_nr_next;
+ my $stat_next_real = raw_line($line_nr_next,
$next_nof_lines);
+ if (!defined($stat_next_real)) {
+ $stat_next_real = "";
+ } elsif ($next_nof_lines > 1) {
+ $stat_next_real =
"[...]\n$stat_next_real";
+ }
+ my (undef, $nindent) = line_stats('+' .
$s_next);
+
+ #print "stat_next<$stat_next> stat<$stat>
indent<$indent> nindent<$nindent> s_next<$s_next>
stat_next_real<$stat_next_real>\n";
+
+ if ($nindent > $indent) {
+ WARN("SUSPICIOUS_CODE_INDENT",
+"suspicious code indentation after
conditional statements\n" .
+$herecurr .
"$stat_real\n$stat_next_real\n");
+ }
+ }
}
# Track the 'values' across context and added lines.
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [checkpatch.pl] Suspicious indentation detection after conditional statement
Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.
For example the following error is caught;
if (foo)
bar();
return;
Which can be either intended by the programmer as:
if (foo)
bar();
return;
or
if (foo) {
bar();
return;
}
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
Request for comments:
I think this is a nice warning to have, since you can get into a buggy situation
when you want to add extra statements to a one line condition, but you forget
to add the {} around the multiline statement block.
scripts/checkpatch.pl | 41 +
1 file changed, 41 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e7bca89..3ca3923 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2565,6 +2565,47 @@ sub process {
WARN(SUSPECT_CODE_INDENT,
suspect code indent for conditional
statements ($indent, $sindent)\n . $herecurr . $stat_real\n);
}
+
+# Also check if the next statement after the previous condition has the same
indent
+ my ($stat_next, undef, $line_nr_next_next) =
+ ctx_statement_block($line_nr_next,
$remain_next, $off_next);
+ my $s_next = $stat_next;
+
+ # Remove line prefixes
+ $s_next =~ s/\n./\n/gs;
+
+ # Remove any comments
+ $s_next =~ s/$;//g;
+
+ # Skip this check for in case next statement starts
with 'else'
+ if ($s_next !~ /\s*\belse\b/) {
+
+ # Remove while that belongs to a do {} while
+ if ($stat =~ /\bdo\b/) {
+ $s_next =~
s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+ }
+
+ # Remove blank lines
+ $s_next =~ s/\s*\\?\n//g;
+
+ # Get the real next lines
+ my $next_nof_lines = $line_nr_next_next -
$line_nr_next;
+ my $stat_next_real = raw_line($line_nr_next,
$next_nof_lines);
+ if (!defined($stat_next_real)) {
+ $stat_next_real = ;
+ } elsif ($next_nof_lines 1) {
+ $stat_next_real =
[...]\n$stat_next_real;
+ }
+ my (undef, $nindent) = line_stats('+' .
$s_next);
+
+ #print stat_next$stat_next stat$stat
indent$indent nindent$nindent s_next$s_next
stat_next_real$stat_next_real\n;
+
+ if ($nindent $indent) {
+ WARN(SUSPICIOUS_CODE_INDENT,
+suspicious code indentation after
conditional statements\n .
+$herecurr .
$stat_real\n$stat_next_real\n);
+ }
+ }
}
# Track the 'values' across context and added lines.
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [checkpatch.pl] ctx_statement_block #if/#else/#endif fix
When picking up a complete statement block #if/#else/#endif prepocesor
boundaries are taken into account by pushing current level type on a stack.
But on an #else the level was read from stack again (without actually popping it
from stack) causing the statement block to end too early on the next ';'.
Fixed this.
For example the following code:
if (!test()) {
#ifdef NEVER
foo();
bar();
#else
bar();
foo();
#endif
}
Results in statement block:
STATEMENT+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();
CONDITION+if (!test())
While you would expect:
STATEMENT+if (!test()) {
+#ifdef NEVER
+ foo();
+ bar();
+#else
+ bar();
+ foo();
+#endif
+ }
CONDITION+ if (!test())
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
Request for comments:
I think I fixed a problem here that I encountered while I was working on another
changeset in which I check the statement block after a condition.
Somehow the statement block did not contain everything I expected.
scripts/checkpatch.pl |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb216..e7bca89 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -878,7 +878,7 @@ sub ctx_statement_block {
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
- ($type, $level) = @{$stack[$#stack - 1]};
+ # no changes to stack: type level remain the same
} elsif ($remainder =~ /^#\s*endif\b/) {
($type, $level) = @{pop(@stack)};
}
@@ -1050,7 +1050,7 @@ sub ctx_block_get {
if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, $level);
} elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
- $level = $stack[$#stack - 1];
+ # no changes to stack: type level remain the same
} elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
$level = pop(@stack);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq: Set the irq thread policy without checking CAP_SYS_NICE
Patch looks OK to me.. but I'm not an expert.
Regards,
Ivo Sieben
2013/10/11 Sebastian Andrzej Siewior :
> From: Thomas Pfaff
>
> In commit ee23871389 ("genirq: Set irq thread to RT priority on
> creation") we moved the assigment of the thread's priority from the
> thread's function into __setup_irq(). That function may run in user
> context for instance if the user opens an UART node and then driver
> calls requests in the ->open() callback. That user may not have
> CAP_SYS_NICE and so the irq thread won't run with the SCHED_OTHER
> policy.
>
> This patch uses sched_setscheduler_nocheck() so we omit the CAP_SYS_NICE
> check which is otherwise required for the SCHED_OTHER policy.
>
> Cc: Ivo Sieben
> Cc: sta...@vger.kernel.org
> Signed-off-by: Thomas Pfaff
> [bigeasy: rewrite the changelog]
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> kernel/irq/manage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 514bcfd..3e59f95 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -956,7 +956,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
> struct irqaction *new)
> goto out_mput;
> }
>
> - sched_setscheduler(t, SCHED_FIFO, );
> + sched_setscheduler_nocheck(t, SCHED_FIFO, );
>
> /*
> * We keep the reference to the task struct even if
> --
> 1.8.4.rc3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq: Set the irq thread policy without checking CAP_SYS_NICE
Patch looks OK to me.. but I'm not an expert. Regards, Ivo Sieben 2013/10/11 Sebastian Andrzej Siewior bige...@linutronix.de: From: Thomas Pfaff tpf...@pcs.com In commit ee23871389 (genirq: Set irq thread to RT priority on creation) we moved the assigment of the thread's priority from the thread's function into __setup_irq(). That function may run in user context for instance if the user opens an UART node and then driver calls requests in the -open() callback. That user may not have CAP_SYS_NICE and so the irq thread won't run with the SCHED_OTHER policy. This patch uses sched_setscheduler_nocheck() so we omit the CAP_SYS_NICE check which is otherwise required for the SCHED_OTHER policy. Cc: Ivo Sieben meltedpiano...@gmail.com Cc: sta...@vger.kernel.org Signed-off-by: Thomas Pfaff tpf...@pcs.com [bigeasy: rewrite the changelog] Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de --- kernel/irq/manage.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 514bcfd..3e59f95 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -956,7 +956,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) goto out_mput; } - sched_setscheduler(t, SCHED_FIFO, param); + sched_setscheduler_nocheck(t, SCHED_FIFO, param); /* * We keep the reference to the task struct even if -- 1.8.4.rc3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[tip:irq/core] genirq: Set irq thread to RT priority on creation
Commit-ID: ee23871389d51e07380d23887333622fbe7d3dd9
Gitweb: http://git.kernel.org/tip/ee23871389d51e07380d23887333622fbe7d3dd9
Author: Ivo Sieben
AuthorDate: Mon, 3 Jun 2013 12:12:02 +0200
Committer: Thomas Gleixner
CommitDate: Tue, 11 Jun 2013 16:18:50 +0200
genirq: Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially
created on normal scheduling priority. Only after the irq thread is
woken up it sets its priority to RT_FIFO MAX_USER_RT_PRIO/2 itself.
This means that interrupts that occur directly after the irq handler
is installed will be handled on a normal scheduling priority instead
of the realtime priority that one would expect.
Fix this by setting the RT priority on creation of the irq_thread.
Signed-off-by: Ivo Sieben
Cc: Sebastian Andrzej Siewior
Cc: Steven Rostedt
Link:
http://lkml.kernel.org/r/1370254322-17240-1-git-send-email-meltedpiano...@gmail.com
Signed-off-by: Thomas Gleixner
---
kernel/irq/manage.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..e16caa8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -840,9 +840,6 @@ static void irq_thread_dtor(struct callback_head *unused)
static int irq_thread(void *data)
{
struct callback_head on_exit_work;
- static const struct sched_param param = {
- .sched_priority = MAX_USER_RT_PRIO/2,
- };
struct irqaction *action = data;
struct irq_desc *desc = irq_to_desc(action->irq);
irqreturn_t (*handler_fn)(struct irq_desc *desc,
@@ -854,8 +851,6 @@ static int irq_thread(void *data)
else
handler_fn = irq_thread_fn;
- sched_setscheduler(current, SCHED_FIFO, );
-
init_task_work(_exit_work, irq_thread_dtor);
task_work_add(current, _exit_work, false);
@@ -950,6 +945,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new->thread_fn && !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
new->name);
@@ -957,6 +955,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, );
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[tip:irq/core] genirq: Set irq thread to RT priority on creation
Commit-ID: ee23871389d51e07380d23887333622fbe7d3dd9
Gitweb: http://git.kernel.org/tip/ee23871389d51e07380d23887333622fbe7d3dd9
Author: Ivo Sieben meltedpiano...@gmail.com
AuthorDate: Mon, 3 Jun 2013 12:12:02 +0200
Committer: Thomas Gleixner t...@linutronix.de
CommitDate: Tue, 11 Jun 2013 16:18:50 +0200
genirq: Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially
created on normal scheduling priority. Only after the irq thread is
woken up it sets its priority to RT_FIFO MAX_USER_RT_PRIO/2 itself.
This means that interrupts that occur directly after the irq handler
is installed will be handled on a normal scheduling priority instead
of the realtime priority that one would expect.
Fix this by setting the RT priority on creation of the irq_thread.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
Cc: Sebastian Andrzej Siewior bige...@linutronix.de
Cc: Steven Rostedt rost...@goodmis.org
Link:
http://lkml.kernel.org/r/1370254322-17240-1-git-send-email-meltedpiano...@gmail.com
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
kernel/irq/manage.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..e16caa8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -840,9 +840,6 @@ static void irq_thread_dtor(struct callback_head *unused)
static int irq_thread(void *data)
{
struct callback_head on_exit_work;
- static const struct sched_param param = {
- .sched_priority = MAX_USER_RT_PRIO/2,
- };
struct irqaction *action = data;
struct irq_desc *desc = irq_to_desc(action-irq);
irqreturn_t (*handler_fn)(struct irq_desc *desc,
@@ -854,8 +851,6 @@ static int irq_thread(void *data)
else
handler_fn = irq_thread_fn;
- sched_setscheduler(current, SCHED_FIFO, param);
-
init_task_work(on_exit_work, irq_thread_dtor);
task_work_add(current, on_exit_work, false);
@@ -950,6 +945,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new-thread_fn !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, irq/%d-%s, irq,
new-name);
@@ -957,6 +955,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, param);
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH-v2] Set irq thread to RT priority on creation
Hi Thomas, 2013/6/3 Thomas Gleixner : > > The question is why there is data present in the UART when the UART > driver has not initialized the UART. Up to the point where the UART is > opened and the interrupt handler is installed the receiver should be > disabled. Also there is the question why a flush does not kill > everything including the pending data in the UART itself. > > And I don't think, that your issue is solved completely. Assume the > following: > > Open UART > Flush Buffers (including UART fifo) > > ---> UART receives data > ---> Interrupt > Data is available in tty buffer > > Write data to UART > > Receive data from UART > > You still get data which got into the buffer before you sent stuff > out. So your application should not be surpriced at all by random data > on the receive line when it starts up. > > Thanks, > > tglx You are absolutely right, the real issue was that my UART device still received data while the device was closed. And yes, the protocol that we use should be robust to unexpected data. I solved & will solve these problems now. So you indeed the uart explanation in my commit log should be removed. The point is that we while we were debugging & tracing this problem, we didn't expect this behavior: in the trace we saw a threaded handler scheduled in on 'normal' priority which surprised us. Also I think there are situations thinkable (altough I cannot come up with a proper example) where it is normal behavior that a IRQ directly kicks in after enabling the interrupts. Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH-v2] Set irq thread to RT priority on creation
Hi Thomas, 2013/6/3 Thomas Gleixner t...@linutronix.de: The question is why there is data present in the UART when the UART driver has not initialized the UART. Up to the point where the UART is opened and the interrupt handler is installed the receiver should be disabled. Also there is the question why a flush does not kill everything including the pending data in the UART itself. And I don't think, that your issue is solved completely. Assume the following: Open UART Flush Buffers (including UART fifo) --- UART receives data --- Interrupt Data is available in tty buffer Write data to UART Receive data from UART You still get data which got into the buffer before you sent stuff out. So your application should not be surpriced at all by random data on the receive line when it starts up. Thanks, tglx You are absolutely right, the real issue was that my UART device still received data while the device was closed. And yes, the protocol that we use should be robust to unexpected data. I solved will solve these problems now. So you indeed the uart explanation in my commit log should be removed. The point is that we while we were debugging tracing this problem, we didn't expect this behavior: in the trace we saw a threaded handler scheduled in on 'normal' priority which surprised us. Also I think there are situations thinkable (altough I cannot come up with a proper example) where it is normal behavior that a IRQ directly kicks in after enabling the interrupts. Regards, Ivo Sieben -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH-v2] Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
immediately sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.
This solves the following issue with a UART device driver:
On start of the application there is already data present in the uart RX
fifo buffer. On opening the uart device the hard & threaded interrupt
handlers are installed and the interrupts are enabled. Immediately a hard
irq occurs because there is still data in the RX fifo. Because the threaded
irq handler is still on normal scheduling, my application is not immediatly
interrupted by the threaded handler and continues to run: it tries to flush
the uart input buffer and writes data to the uart device. After this the
threaded handler finally gets scheduled in and fills the buffer with the
"old" received data. When my application reads data from the uart port it
receives the "old" data and gives an error.
Signed-off-by: Ivo Sieben
---
v2:
* Removed the sched_setscheduler() call in irq_thread() function
* Updated commit log why this solves an issue for me with a UART driver
kernel/irq/manage.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..e16caa8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -840,9 +840,6 @@ static void irq_thread_dtor(struct callback_head *unused)
static int irq_thread(void *data)
{
struct callback_head on_exit_work;
- static const struct sched_param param = {
- .sched_priority = MAX_USER_RT_PRIO/2,
- };
struct irqaction *action = data;
struct irq_desc *desc = irq_to_desc(action->irq);
irqreturn_t (*handler_fn)(struct irq_desc *desc,
@@ -854,8 +851,6 @@ static int irq_thread(void *data)
else
handler_fn = irq_thread_fn;
- sched_setscheduler(current, SCHED_FIFO, );
-
init_task_work(_exit_work, irq_thread_dtor);
task_work_add(current, _exit_work, false);
@@ -950,6 +945,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new->thread_fn && !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
new->name);
@@ -957,6 +955,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, );
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH-v2] Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
immediately sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.
This solves the following issue with a UART device driver:
On start of the application there is already data present in the uart RX
fifo buffer. On opening the uart device the hard threaded interrupt
handlers are installed and the interrupts are enabled. Immediately a hard
irq occurs because there is still data in the RX fifo. Because the threaded
irq handler is still on normal scheduling, my application is not immediatly
interrupted by the threaded handler and continues to run: it tries to flush
the uart input buffer and writes data to the uart device. After this the
threaded handler finally gets scheduled in and fills the buffer with the
old received data. When my application reads data from the uart port it
receives the old data and gives an error.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
v2:
* Removed the sched_setscheduler() call in irq_thread() function
* Updated commit log why this solves an issue for me with a UART driver
kernel/irq/manage.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..e16caa8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -840,9 +840,6 @@ static void irq_thread_dtor(struct callback_head *unused)
static int irq_thread(void *data)
{
struct callback_head on_exit_work;
- static const struct sched_param param = {
- .sched_priority = MAX_USER_RT_PRIO/2,
- };
struct irqaction *action = data;
struct irq_desc *desc = irq_to_desc(action-irq);
irqreturn_t (*handler_fn)(struct irq_desc *desc,
@@ -854,8 +851,6 @@ static int irq_thread(void *data)
else
handler_fn = irq_thread_fn;
- sched_setscheduler(current, SCHED_FIFO, param);
-
init_task_work(on_exit_work, irq_thread_dtor);
task_work_add(current, on_exit_work, false);
@@ -950,6 +945,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new-thread_fn !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, irq/%d-%s, irq,
new-name);
@@ -957,6 +955,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, param);
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] RFC: Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.
Signed-off-by: Ivo Sieben
---
RFC:
Whas there a specific reason for the irq_thread to be created on normal
scheduling and only set to RT priority when woken up?
This patch solves an issue for me where a device driver is expected to handle an
interrupt immediatly after irq handlers are installed and interrupts enabled.
(If this does make sense: I guess the sched_setscheduler() call in the
irq_thread
function can be removed?)
kernel/irq/manage.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..0ffe37b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -950,6 +950,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new->thread_fn && !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
new->name);
@@ -957,6 +960,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, );
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] RFC: Set irq thread to RT priority on creation
When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
RFC:
Whas there a specific reason for the irq_thread to be created on normal
scheduling and only set to RT priority when woken up?
This patch solves an issue for me where a device driver is expected to handle an
interrupt immediatly after irq handlers are installed and interrupts enabled.
(If this does make sense: I guess the sched_setscheduler() call in the
irq_thread
function can be removed?)
kernel/irq/manage.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..0ffe37b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -950,6 +950,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
*/
if (new-thread_fn !nested) {
struct task_struct *t;
+ static const struct sched_param param = {
+ .sched_priority = MAX_USER_RT_PRIO/2,
+ };
t = kthread_create(irq_thread, new, irq/%d-%s, irq,
new-name);
@@ -957,6 +960,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct
irqaction *new)
ret = PTR_ERR(t);
goto out_mput;
}
+
+ sched_setscheduler(t, SCHED_FIFO, param);
+
/*
* We keep the reference to the task struct even if
* the thread dies to avoid that the interrupt code
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi, 2013/1/18 Oleg Nesterov : > > I can't understand why do you dislike Ivo's simple patch. There are > a lot of "if (waitqueue_active) wake_up" examples. Even if we add the > new helpers (personally I don't think this makes sense) , we can do > this later. Why should we delay this fix? > FYI: Greg has added my patch to his tty-next branch, so my fix has been approved. Thank you all for reviewing. Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi, 2013/1/18 Oleg Nesterov o...@redhat.com: I can't understand why do you dislike Ivo's simple patch. There are a lot of if (waitqueue_active) wake_up examples. Even if we add the new helpers (personally I don't think this makes sense) , we can do this later. Why should we delay this fix? FYI: Greg has added my patch to his tty-next branch, so my fix has been approved. Thank you all for reviewing. Regards, Ivo Sieben -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
2013/1/16 Preeti U Murthy : > > Yes.Thank you very much for the explanation :) But I dont see how the > context switching goes away with your patch.With your patch, when the > higher priority thread comes in when the lower priority thread is > running in the critical section,it will see the wait queue empty and > "continue its execution" without now wanting to enter the critical > section.So this means it will preempt the lower priority thread because > it is not waiting on a lock anyway.There is a context switch here right? > I dont see any problem in scheduling due to this,but I do think your > patch is essential. > I don't have a problem that there is a context switch to the high priority process: it has a higher priority, so it probably is more important. My problem is that even when the waitqueue is empty, the high priority thread has a risk to block on the spinlock needlessly (causing context switches to low priority task and back to the high priority task) Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi Preeti, 2013/1/16 Preeti U Murthy : > Hi Ivo, > Can you explain how this problem could create a scheduler overhead? > I am a little confused, because as far as i know,scheduler does not come > in the picture of the wake up path right? select_task_rq() in > try_to_wake_up() is where the scheduler comes in,and this is after the > task wakes up. > Everytime the line discipline is dereferenced, the wakeup function is called. The wakeup() function contains a critical section protected by spinlocks. On a PREEMPT_RT system, a "normal" spinlock behaves just like a mutex: scheduling is not disabled and it is still possible that a new process on a higher RT priority is scheduled in. When a new - higher priority - process is scheduled in just when the put_ldisc() is in the critical section of the wakeup function, the higher priority process (that uses the same TTY instance) will finally also dereference the line discipline and try to wakeup the same waitqueue. This causes the high priority process to block on the same spinlock. Priority inheritance will solve this blocked situation by a context switch to the lower priority process, run until that process leaves the critical section, and a context switch back to the higher priority process. This is unnecessary since the waitqueue was empty after all (during normal operation the waitqueue is empty most of the time). This unnecessary context switch from/to the high priority process is what a mean with "scheduler overhead" (maybe not a good name for it, sorry for the confusion). Does this makes sense to you? Regards, ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi Preeti, 2013/1/16 Preeti U Murthy pre...@linux.vnet.ibm.com: Hi Ivo, Can you explain how this problem could create a scheduler overhead? I am a little confused, because as far as i know,scheduler does not come in the picture of the wake up path right? select_task_rq() in try_to_wake_up() is where the scheduler comes in,and this is after the task wakes up. Everytime the line discipline is dereferenced, the wakeup function is called. The wakeup() function contains a critical section protected by spinlocks. On a PREEMPT_RT system, a normal spinlock behaves just like a mutex: scheduling is not disabled and it is still possible that a new process on a higher RT priority is scheduled in. When a new - higher priority - process is scheduled in just when the put_ldisc() is in the critical section of the wakeup function, the higher priority process (that uses the same TTY instance) will finally also dereference the line discipline and try to wakeup the same waitqueue. This causes the high priority process to block on the same spinlock. Priority inheritance will solve this blocked situation by a context switch to the lower priority process, run until that process leaves the critical section, and a context switch back to the higher priority process. This is unnecessary since the waitqueue was empty after all (during normal operation the waitqueue is empty most of the time). This unnecessary context switch from/to the high priority process is what a mean with scheduler overhead (maybe not a good name for it, sorry for the confusion). Does this makes sense to you? Regards, ivo Sieben -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
2013/1/16 Preeti U Murthy pre...@linux.vnet.ibm.com: Yes.Thank you very much for the explanation :) But I dont see how the context switching goes away with your patch.With your patch, when the higher priority thread comes in when the lower priority thread is running in the critical section,it will see the wait queue empty and continue its execution without now wanting to enter the critical section.So this means it will preempt the lower priority thread because it is not waiting on a lock anyway.There is a context switch here right? I dont see any problem in scheduling due to this,but I do think your patch is essential. I don't have a problem that there is a context switch to the high priority process: it has a higher priority, so it probably is more important. My problem is that even when the waitqueue is empty, the high priority thread has a risk to block on the spinlock needlessly (causing context switches to low priority task and back to the high priority task) Regards, Ivo Sieben -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi 2013/1/3 Oleg Nesterov : >> I want to ask you 'scheduler' people for your opinion: >> >> Maybe you remember my previous patch where I suggested an extra >> 'waitqueue empty' check before entering the critical section of the >> wakeup() function (If you do not remember see >> https://lkml.org/lkml/2012/10/25/159) >> >> Finally Oleg responded that a lot of callers do >> >> if (waitqueue_active(q)) >> wake_up(...); >> >> what made my patch pointless and adds a memory barrier. > > Plus this change doesn't look 100% correct, at least in theory. > >> I then decided >> to also implement the 'waitqueue_active' approach for my problem. > > Well, if you ask me I think this is the best solution ;) > > But I won't insist. > >> But now I get a review comment by Jiri that he would like to hide this >> 'if active behavior' in a wake_up_if_active() kind of function. I >> think he is right that implementing this check in the wakeup function >> would clean things up, right? >> >> I would like to have your opinion on the following two suggestions: >> - We still can do the original patch on the wake_up() that I >> suggested. I then can do an additional code cleanup patch that removes >> the double 'waitqueue_active' call (a quick grep found about 150 of >> these waitqueue active calls) on several places in the code. > > In this case we should also fix some users of add_wait_queue(). > >> - Or - as an alternative - I could add extra _if_active() versions of >> all wake_up() functions, that implement this extra test. > > Not sure this will actually help to make the code cleaner. The last > patch you sent looks simple and clean. IMHO it doesn't make sense > to create _if_active helper for each wake_up*. > > Oleg. > The comments by Oleg point out to me that the 'if waitqueueu_active' is a common practice for checking a waitqueue to be non empty. It is applied this way on several places in the kernel code. @Jiri, Alan, Greg: Don't you agree my patch makes sense? It solves an issue for me, and I really would like this patch to be approved. Regards, Ivo Sieben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi 2013/1/3 Oleg Nesterov o...@redhat.com: I want to ask you 'scheduler' people for your opinion: Maybe you remember my previous patch where I suggested an extra 'waitqueue empty' check before entering the critical section of the wakeup() function (If you do not remember see https://lkml.org/lkml/2012/10/25/159) Finally Oleg responded that a lot of callers do if (waitqueue_active(q)) wake_up(...); what made my patch pointless and adds a memory barrier. Plus this change doesn't look 100% correct, at least in theory. I then decided to also implement the 'waitqueue_active' approach for my problem. Well, if you ask me I think this is the best solution ;) But I won't insist. But now I get a review comment by Jiri that he would like to hide this 'if active behavior' in a wake_up_if_active() kind of function. I think he is right that implementing this check in the wakeup function would clean things up, right? I would like to have your opinion on the following two suggestions: - We still can do the original patch on the wake_up() that I suggested. I then can do an additional code cleanup patch that removes the double 'waitqueue_active' call (a quick grep found about 150 of these waitqueue active calls) on several places in the code. In this case we should also fix some users of add_wait_queue(). - Or - as an alternative - I could add extra _if_active() versions of all wake_up() functions, that implement this extra test. Not sure this will actually help to make the code cleaner. The last patch you sent looks simple and clean. IMHO it doesn't make sense to create _if_active helper for each wake_up*. Oleg. The comments by Oleg point out to me that the 'if waitqueueu_active' is a common practice for checking a waitqueue to be non empty. It is applied this way on several places in the kernel code. @Jiri, Alan, Greg: Don't you agree my patch makes sense? It solves an issue for me, and I really would like this patch to be approved. Regards, Ivo Sieben -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Oleg, Peter, Ingo, Andi & Preeti, 2013/1/2 Jiri Slaby : > On 01/02/2013 04:21 PM, Ivo Sieben wrote: >> I don't understand your responses: do you suggest to implement this >> "if active" behavior in: >> * A new wake_up function called wake_up_if_active() that is part of >> the waitqueue layer? > > Sounds good. > > -- > js > suse labs I want to ask you 'scheduler' people for your opinion: Maybe you remember my previous patch where I suggested an extra 'waitqueue empty' check before entering the critical section of the wakeup() function (If you do not remember see https://lkml.org/lkml/2012/10/25/159) Finally Oleg responded that a lot of callers do if (waitqueue_active(q)) wake_up(...); what made my patch pointless and adds a memory barrier. I then decided to also implement the 'waitqueue_active' approach for my problem. But now I get a review comment by Jiri that he would like to hide this 'if active behavior' in a wake_up_if_active() kind of function. I think he is right that implementing this check in the wakeup function would clean things up, right? I would like to have your opinion on the following two suggestions: - We still can do the original patch on the wake_up() that I suggested. I then can do an additional code cleanup patch that removes the double 'waitqueue_active' call (a quick grep found about 150 of these waitqueue active calls) on several places in the code. - Or - as an alternative - I could add extra _if_active() versions of all wake_up() functions, that implement this extra test. Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Oleg, Peter, Ingo, Andi Preeti, 2013/1/2 Jiri Slaby jsl...@suse.cz: On 01/02/2013 04:21 PM, Ivo Sieben wrote: I don't understand your responses: do you suggest to implement this if active behavior in: * A new wake_up function called wake_up_if_active() that is part of the waitqueue layer? Sounds good. -- js suse labs I want to ask you 'scheduler' people for your opinion: Maybe you remember my previous patch where I suggested an extra 'waitqueue empty' check before entering the critical section of the wakeup() function (If you do not remember see https://lkml.org/lkml/2012/10/25/159) Finally Oleg responded that a lot of callers do if (waitqueue_active(q)) wake_up(...); what made my patch pointless and adds a memory barrier. I then decided to also implement the 'waitqueue_active' approach for my problem. But now I get a review comment by Jiri that he would like to hide this 'if active behavior' in a wake_up_if_active() kind of function. I think he is right that implementing this check in the wakeup function would clean things up, right? I would like to have your opinion on the following two suggestions: - We still can do the original patch on the wake_up() that I suggested. I then can do an additional code cleanup patch that removes the double 'waitqueue_active' call (a quick grep found about 150 of these waitqueue active calls) on several places in the code. - Or - as an alternative - I could add extra _if_active() versions of all wake_up() functions, that implement this extra test. Regards, Ivo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi Jiri & Alan, 2013/1/2 Alan Cox : > >> Looks good, but I would prefer the layer to provide us with >> wake_up_if_active... > > Seconded - this is a problem for the wake up logic in the RT tree. Why > would we ever do anything else ? I don't understand your responses: do you suggest to implement this "if active" behavior in: * A new wake_up function called wake_up_if_active() that is part of the waitqueue layer? * Implement this behavior in the existing wake_up() function as part of the RT patch? Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Hi Jiri Alan, 2013/1/2 Alan Cox a...@lxorguk.ukuu.org.uk: Looks good, but I would prefer the layer to provide us with wake_up_if_active... Seconded - this is a problem for the wake up logic in the RT tree. Why would we ever do anything else ? I don't understand your responses: do you suggest to implement this if active behavior in: * A new wake_up function called wake_up_if_active() that is part of the waitqueue layer? * Implement this behavior in the existing wake_up() function as part of the RT patch? Regards, Ivo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Before waking up the tty line discipline idle queue first check if the queue is active (non empty). This prevents unnecessary entering the critical section in the wake_up() function and therefore avoid needless scheduling overhead on a PREEMPT_RT system caused by two processes being in the same critical section. Signed-off-by: Ivo Sieben --- Remark: This patch has kind of a long history... I first tried to prevent the critical section in the wakeup() function itself by a change in the scheduler. But after review remarks from Oleg Nesterov it turned out that using the waitqueue_active() was a much nicer way to prevent it. See also https://lkml.org/lkml/2012/10/25/159 drivers/tty/tty_ldisc.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index c578229..e96d187 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -64,7 +64,9 @@ static void put_ldisc(struct tty_ldisc *ld) return; } raw_spin_unlock_irqrestore(_ldisc_lock, flags); - wake_up(>wq_idle); + + if (waitqueue_active(>wq_idle)) + wake_up(>wq_idle); } /** -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Only wakeup the line discipline idle queue when queue is active
Before waking up the tty line discipline idle queue first check if the queue is active (non empty). This prevents unnecessary entering the critical section in the wake_up() function and therefore avoid needless scheduling overhead on a PREEMPT_RT system caused by two processes being in the same critical section. Signed-off-by: Ivo Sieben meltedpiano...@gmail.com --- Remark: This patch has kind of a long history... I first tried to prevent the critical section in the wakeup() function itself by a change in the scheduler. But after review remarks from Oleg Nesterov it turned out that using the waitqueue_active() was a much nicer way to prevent it. See also https://lkml.org/lkml/2012/10/25/159 drivers/tty/tty_ldisc.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index c578229..e96d187 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -64,7 +64,9 @@ static void put_ldisc(struct tty_ldisc *ld) return; } raw_spin_unlock_irqrestore(tty_ldisc_lock, flags); - wake_up(ld-wq_idle); + + if (waitqueue_active(ld-wq_idle)) + wake_up(ld-wq_idle); } /** -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Hi 2012/11/19 Oleg Nesterov : > > Because on a second thought I suspect this change is wrong. > > Just for example, please look at kauditd_thread(). It does > > set_current_state(TASK_INTERRUPTIBLE); > > add_wait_queue(_wait, ); > > if (!CONDITION) // <-- LOAD > schedule(); > > And the last LOAD can leak into the critical section protected by > wait_queue_head_t->lock, and it can be reordered with list_add() > inside this critical section. In this case we can race with wake_up() > unless it takes the same lock. > > Oleg. > I agree that I should solve my problem using the waitqueue_active() function locally. I'll abandon this patch and fix it in the tty_ldisc.c. But we try to understand your fault scenario: How can the LOAD leak into the critical section? As far as we understand the spin_unlock() function also contains a memory barrier to prevent such a reordering from happening. Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Hi 2012/11/19 Oleg Nesterov o...@redhat.com: Because on a second thought I suspect this change is wrong. Just for example, please look at kauditd_thread(). It does set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(kauditd_wait, wait); if (!CONDITION) // -- LOAD schedule(); And the last LOAD can leak into the critical section protected by wait_queue_head_t-lock, and it can be reordered with list_add() inside this critical section. In this case we can race with wake_up() unless it takes the same lock. Oleg. I agree that I should solve my problem using the waitqueue_active() function locally. I'll abandon this patch and fix it in the tty_ldisc.c. But we try to understand your fault scenario: How can the LOAD leak into the critical section? As far as we understand the spin_unlock() function also contains a memory barrier to prevent such a reordering from happening. Regards, Ivo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Hi 2012/11/19 Oleg Nesterov : > > I am wondering if it makes sense unconditionally. A lot of callers do > > if (waitqueue_active(q)) > wake_up(...); > > this patch makes the optimization above pointless and adds mb(). > > > But I won't argue. > > Oleg. > This patch solved an issue for me that I had with the TTY line discipline idle handling: Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY line discipline is dereferenced the Idle handling wait queue is woken up (see function put_ldisc in /drivers/tty/tty_ldisc.c) However line discipline idle handling is not used very often so the wait queue is empty most of the time. But still the wake_up() function enters the critical section guarded by spin locks. This causes additional scheduling overhead when a lower priority thread has control of that same lock. The /drivers/tty/tty_ldisc.c did not use the waitqueue_active() call to check if the waitqueue was filled maybe I should solve this problem the other way around: and make tty_ldisc.c first do the waitqueue_active() call? Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Hi 2012/11/19 Oleg Nesterov o...@redhat.com: I am wondering if it makes sense unconditionally. A lot of callers do if (waitqueue_active(q)) wake_up(...); this patch makes the optimization above pointless and adds mb(). But I won't argue. Oleg. This patch solved an issue for me that I had with the TTY line discipline idle handling: Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY line discipline is dereferenced the Idle handling wait queue is woken up (see function put_ldisc in /drivers/tty/tty_ldisc.c) However line discipline idle handling is not used very often so the wait queue is empty most of the time. But still the wake_up() function enters the critical section guarded by spin locks. This causes additional scheduling overhead when a lower priority thread has control of that same lock. The /drivers/tty/tty_ldisc.c did not use the waitqueue_active() call to check if the waitqueue was filled maybe I should solve this problem the other way around: and make tty_ldisc.c first do the waitqueue_active() call? Regards, Ivo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben
---
a second repost of this patch v2: Can anyone respond?
Did I apply the memory barrier correct?
v2:
- We don't need the "careful" list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(>lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(>task_list)) {
+ spin_lock_irqsave(>lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(>lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
a second repost of this patch v2: Can anyone respond?
Did I apply the memory barrier correct?
v2:
- We don't need the careful list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(q-lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(q-lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(q-task_list)) {
+ spin_lock_irqsave(q-lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(q-lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben
---
repost:
Did I apply the memory barrier correct?
v2:
- We don't need the "careful" list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(>lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(>task_list)) {
+ spin_lock_irqsave(>lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(>lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[REPOST-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
repost:
Did I apply the memory barrier correct?
v2:
- We don't need the careful list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(q-lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(q-lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(q-task_list)) {
+ spin_lock_irqsave(q-lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(q-lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben
---
v2:
- We don't need the "careful" list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(>lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(>task_list)) {
+ spin_lock_irqsave(>lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(>lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH-v2] sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
v2:
- We don't need the careful list empty, a normal list empty is sufficient:
if you miss an update it was just as it happened a little later.
- Because of memory ordering problems we can observe an unupdated list
administration. This can cause an wait_event-like code to miss an event.
Adding a memory barrier befor checking the list to be empty will guarantee we
evaluate a 100% updated list adminsitration.
kernel/sched/core.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d8927f..168a9b2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,22 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(q-lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(q-lock, flags);
+ /*
+* We check for list emptiness outside the lock. This prevents the wake
+* up to enter the critical section needlessly when the task list is
+* empty.
+*
+* Placed a full memory barrier before checking list emptiness to make
+* 100% sure this function sees an up-to-date list administration.
+* Note that other code that manipulates the list uses a spin_lock and
+* therefore doesn't need additional memory barriers.
+*/
+ smp_mb();
+ if (!list_empty(q-task_list)) {
+ spin_lock_irqsave(q-lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(q-lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[REPOST] RFC: sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben
---
REPOST:
Request for comments:
- Does this make any sense?
- I assume that I can safely use the list_empty_careful() function here, but is
that correct?
Background to this patch:
Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY
line discipline is dereferenced the Idle handling wait queue is woken up (see
function put_ldisc in /drivers/tty/tty_ldisc.c)
However line discipline idle handling is not used very often so the wait queue
is empty most of the time. But still the wake_up() function enters the critical
section guarded by spin locks. This causes additional scheduling overhead when
a lower priority thread has control of that same lock.
kernel/sched/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c177472..c1667c4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,19 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(>lock, flags);
+ /*
+* We can check for list emptiness outside the lock by using the
+* "careful" check that verifies both the next and prev pointers, so
+* that there cannot be any half-pending updates in progress.
+*
+* This prevents the wake up to enter the critical section needlessly
+* when the task list is empty.
+*/
+ if (!list_empty_careful(>task_list)) {
+ spin_lock_irqsave(>lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(>lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[REPOST] RFC: sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
REPOST:
Request for comments:
- Does this make any sense?
- I assume that I can safely use the list_empty_careful() function here, but is
that correct?
Background to this patch:
Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY
line discipline is dereferenced the Idle handling wait queue is woken up (see
function put_ldisc in /drivers/tty/tty_ldisc.c)
However line discipline idle handling is not used very often so the wait queue
is empty most of the time. But still the wake_up() function enters the critical
section guarded by spin locks. This causes additional scheduling overhead when
a lower priority thread has control of that same lock.
kernel/sched/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c177472..c1667c4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3090,9 +3090,19 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(q-lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(q-lock, flags);
+ /*
+* We can check for list emptiness outside the lock by using the
+* careful check that verifies both the next and prev pointers, so
+* that there cannot be any half-pending updates in progress.
+*
+* This prevents the wake up to enter the critical section needlessly
+* when the task list is empty.
+*/
+ if (!list_empty_careful(q-task_list)) {
+ spin_lock_irqsave(q-lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(q-lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] RFC: sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben
---
Request for comments:
- Does this make any sense?
- I assume that I can safely use the list_empty_careful() function here, but is
that correct?
Background to this patch:
Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY
line discipline is dereferenced the Idle handling wait queue is woken up (see
function put_ldisc in /drivers/tty/tty_ldisc.c)
However line discipline idle handling is not used very often so the wait queue
is empty most of the time. But still the wake_up() function enters the critical
section guarded by spin locks. This causes additional scheduling overhead when
a lower priority thread has control of that same lock.
kernel/sched/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 649c9f8..6436eb8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3631,9 +3631,19 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(>lock, flags);
+ /*
+* We can check for list emptiness outside the lock by using the
+* "careful" check that verifies both the next and prev pointers, so
+* that there cannot be any half-pending updates in progress.
+*
+* This prevents the wake up to enter the critical section needlessly
+* when the task list is empty.
+*/
+ if (!list_empty_careful(>task_list)) {
+ spin_lock_irqsave(>lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(>lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] RFC: sched: Prevent wakeup to enter critical section needlessly
Check the waitqueue task list to be non empty before entering the critical
section. This prevents locking the spin lock needlessly in case the queue
was empty, and therefor also prevent scheduling overhead on a PREEMPT_RT
system.
Signed-off-by: Ivo Sieben meltedpiano...@gmail.com
---
Request for comments:
- Does this make any sense?
- I assume that I can safely use the list_empty_careful() function here, but is
that correct?
Background to this patch:
Testing on a PREEMPT_RT system with TTY serial communication. Each time the TTY
line discipline is dereferenced the Idle handling wait queue is woken up (see
function put_ldisc in /drivers/tty/tty_ldisc.c)
However line discipline idle handling is not used very often so the wait queue
is empty most of the time. But still the wake_up() function enters the critical
section guarded by spin locks. This causes additional scheduling overhead when
a lower priority thread has control of that same lock.
kernel/sched/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 649c9f8..6436eb8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3631,9 +3631,19 @@ void __wake_up(wait_queue_head_t *q, unsigned int mode,
{
unsigned long flags;
- spin_lock_irqsave(q-lock, flags);
- __wake_up_common(q, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(q-lock, flags);
+ /*
+* We can check for list emptiness outside the lock by using the
+* careful check that verifies both the next and prev pointers, so
+* that there cannot be any half-pending updates in progress.
+*
+* This prevents the wake up to enter the critical section needlessly
+* when the task list is empty.
+*/
+ if (!list_empty_careful(q-task_list)) {
+ spin_lock_irqsave(q-lock, flags);
+ __wake_up_common(q, mode, nr_exclusive, 0, key);
+ spin_unlock_irqrestore(q-lock, flags);
+ }
}
EXPORT_SYMBOL(__wake_up);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] misc/at25: Add an .id_table to at25 to facilitate driver loading and binding.
Hi,
2012/8/22 David Daney :
> From: David Daney
>
> /*-*/
> +static const struct spi_device_id at25_id[] = {
> + {"at25", 0},
> + {"m95256", 0},
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, at25_id);
I use this driver for the ST M95040, M95020 & M95010 eeprom devices.
So wouldn't it be better to use the "famliy" name for these chips
("m95" instead of "m95256").
Regards,
Ivo Sieben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] misc/at25: Add an .id_table to at25 to facilitate driver loading and binding.
Hi,
2012/8/22 David Daney ddaney.c...@gmail.com:
From: David Daney david.da...@cavium.com
/*-*/
+static const struct spi_device_id at25_id[] = {
+ {at25, 0},
+ {m95256, 0},
+ { }
+};
+MODULE_DEVICE_TABLE(spi, at25_id);
I use this driver for the ST M95040, M95020 M95010 eeprom devices.
So wouldn't it be better to use the famliy name for these chips
(m95 instead of m95256).
Regards,
Ivo Sieben
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
