From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 11:36:56
As part of memory barrier cleanup, this patchset
extends checkpatch to make it easier to stop
incorrect memory barrier usage.
This applies on top of my series
arch: barrier cleanup + barriers for virt
and will be included in the next version of the series.
Michael S. Tsirkin (3):
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
scripts/checkpatch.pl | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 11:37:09
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -5116,7 +5116,14 @@ sub process {}}#checkformemorybarrierswithoutacomment.-if($line=~/\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/){++my@barriers=('mb','rmb','wmb','read_barrier_depends');+my@smp_barriers=('smp_store_release','smp_load_acquire','smp_store_mb');++@smp_barriers=(@smp_barriers,map{"smp_".$_}@barriers);+my$all_barriers=join('|',(@barriers,@smp_barriers));++if($line=~/\b($all_barriers)\(/){if(!ctx_has_comment($first_line,$linenr)){WARN("MEMORY_BARRIER","memory barrier without comment\n".$herecurr);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 11:37:17
Introduction of __smp barriers cleans up a bunch of duplicate code, but
it gives people an additional handle onto a "new" set of barriers - just
because they're prefixed with __* unfortunately doesn't stop anyone from
using it (as happened with other arch stuff before.)
Add a checkpatch test so it will trigger a warning.
Reported-by: Russell King <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -5130,6 +5130,17 @@ sub process {}}+my@underscore_smp_barriers=map{"__".$_}@smp_barriers;+my$underscore_all_barriers=join('|',@underscore_smp_barriers);++if($realfile!~m@^include/asm-generic/@&&+$realfile!~m@/barrier\.h$@&&+$line=~m/\b($underscore_all_barriers)\(/&&+$line!~m/^.\s*\#\s*define\s+($underscore_all_barriers)\(/){+WARN("MEMORY_BARRIER",+"__smp memory barriers shouldn't be used outside barrier.h and asm-generic\n".$herecurr);+}+#checkforwaitqueue_activewithoutacomment.if($line=~/\bwaitqueue_active\s*\(/){if(!ctx_has_comment($first_line,$linenr)){
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 11:37:27
Add virt_ barriers to list of barriers to check for
presence of a comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -5121,7 +5121,8 @@ sub process {my@smp_barriers=('smp_store_release','smp_load_acquire','smp_store_mb');@smp_barriers=(@smp_barriers,map{"smp_".$_}@barriers);-my$all_barriers=join('|',(@barriers,@smp_barriers));+my@virt_barriers=map{my$l=$_;$l=~s/smp_/virt_/;$l}@smp_barriers;+my$all_barriers=join('|',(@barriers,@smp_barriers,@virt_barriers));if($line=~/\b($all_barriers)\(/){if(!ctx_has_comment($first_line,$linenr)){
From: Joe Perches <joe@perches.com> Date: 2016-01-04 16:07:52
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted hunk
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
}
}
# check for memory barriers without a comment.
- if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
+
+ my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
+ my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 'smp_store_mb');
+
+ @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
I think using map, which so far checkpatch doesn't use,
makes smp_barriers harder to understand and it'd be
better to enumerate them.
+ my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+ if ($line =~ /\b($all_barriers)\(/) {
It would be better to use /\b$all_barriers\s*\(/
as there's no reason for the capture and there
could be a space between the function and the
open parenthesis.
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
"memory barrier without comment\n" . $herecurr);
From: Russell King - ARM Linux <hidden> Date: 2016-01-04 16:11:44
On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted
+ my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+ if ($line =~ /\b($all_barriers)\(/) {
It would be better to use /\b$all_barriers\s*\(/
as there's no reason for the capture and there
could be a space between the function and the
open parenthesis.
I think you mean
/\b(?:$all_barriers)\s*\(/
as 'all_barriers' will be:
mb|wmb|rmb|smp_mb|smp_wmb|smp_rmb
and putting that into your suggestion results in:
/\bmb|wmb|rmb|smp_mb|smp_wmb|smp_rmb\s*\(/
which is clearly wrong - the \b only applies to 'mb' and the \s*\( only
applies to smp_rmb.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Joe Perches <joe@perches.com> Date: 2016-01-04 16:15:27
On Mon, 2016-01-04 at 16:11 +0000, Russell King - ARM Linux wrote:
On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
quoted
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted
+ my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+ if ($line =~ /\b($all_barriers)\(/) {
It would be better to use /\b$all_barriers\s*\(/
as there's no reason for the capture and there
could be a space between the function and the
open parenthesis.
I think you mean
/\b(?:$all_barriers)\s*\(/
as 'all_barriers' will be:
mb|wmb|rmb|smp_mb|smp_wmb|smp_rmb
and putting that into your suggestion results in:
/\bmb|wmb|rmb|smp_mb|smp_wmb|smp_rmb\s*\(/
which is clearly wrong - the \b only applies to 'mb' and the \s*\( only
applies to smp_rmb.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:45:31
On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
}
}
# check for memory barriers without a comment.
- if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
+
+ my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
+ my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 'smp_store_mb');
+
+ @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
I think using map, which so far checkpatch doesn't use,
makes smp_barriers harder to understand and it'd be
better to enumerate them.
Okay - I'll rewrite using foreach.
quoted
+ my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+ if ($line =~ /\b($all_barriers)\(/) {
It would be better to use /\b$all_barriers\s*\(/
as there's no reason for the capture and there
could be a space between the function and the
open parenthesis.
That's the way it was - space before ( will trigger other
warnings. But sure, ok.
quoted
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
"memory barrier without comment\n" . $herecurr);
From: Joe Perches <joe@perches.com> Date: 2016-01-04 22:16:02
On Mon, 2016-01-04 at 22:45 +0200, Michael S. Tsirkin wrote:
On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
quoted
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 11:42:46
On Mon, Jan 04, 2016 at 02:15:50PM -0800, Joe Perches wrote:
On Mon, 2016-01-04 at 22:45 +0200, Michael S. Tsirkin wrote:
quoted
On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
quoted
On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
quoted
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)