Michael S. Tsirkin
2016-Jan-10 19:30 UTC
[PATCH v3 0/3] checkpatch: handling of memory barriers
As part of memory barrier cleanup, this patchset extends checkpatch to make it easier to stop incorrect memory barrier usage. This replaces the checkpatch patches in my series arch: barrier cleanup + barriers for virt and will be included in the next version of the series. changes from v2: address comments by Joe Perches: use (?: ... ) to avoid unnecessary capture groups rename smp_barriers to smp_barrier_stems for clarity add barriers before/after atomic Changes from v1: catch optional\s* before () in barriers rewrite using qr{} instead of map Michael S. Tsirkin (3): checkpatch.pl: add missing memory barriers checkpatch: check for __smp outside barrier.h checkpatch: add virt barriers scripts/checkpatch.pl | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) -- MST
Michael S. Tsirkin
2016-Jan-10 19:30 UTC
[PATCH v3 1/3] checkpatch.pl: add missing memory barriers
SMP-only barriers were missing in checkpatch.pl Refactor code slightly to make adding more variants easier. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- scripts/checkpatch.pl | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2b3c228..1c01b7d 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5116,7 +5116,27 @@ sub process { } } # 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 = qr{ + mb| + rmb| + wmb| + read_barrier_depends + }x; + my $smp_barrier_stems = qr{ + mb__before_atomic| + mb__after_atomic| + store_release| + load_acquire| + store_mb| + (?:$barriers) + }x; + my $all_barriers = qr{ + $barriers| + smp_(?:$smp_barrier_stems) + }x; + + if ($line =~ /\b(?:$all_barriers)\s*\(/) { if (!ctx_has_comment($first_line, $linenr)) { WARN("MEMORY_BARRIER", "memory barrier without comment\n" . $herecurr); -- MST
Michael S. Tsirkin
2016-Jan-10 19:30 UTC
[PATCH v3 2/3] checkpatch: check for __smp outside barrier.h
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 <linux at arm.linux.org.uk> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- scripts/checkpatch.pl | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1c01b7d..15cfca4 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5143,6 +5143,16 @@ sub process { } } + my $underscore_smp_barriers = qr{__smp_(?:$smp_barrier_stems)}x; + + if ($realfile !~ m@^include/asm-generic/@ && + $realfile !~ m@/barrier\.h$@ && + $line =~ m/\b(?:$underscore_smp_barriers)\s*\(/ && + $line !~ m/^.\s*\#\s*define\s+(?:$underscore_smp_barriers)\s*\(/) { + WARN("MEMORY_BARRIER", + "__smp memory barriers shouldn't be used outside barrier.h and asm-generic\n" . $herecurr); + } + # check for waitqueue_active without a comment. if ($line =~ /\bwaitqueue_active\s*\(/) { if (!ctx_has_comment($first_line, $linenr)) { -- MST
Add virt_ barriers to list of barriers to check for presence of a comment. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- scripts/checkpatch.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 15cfca4..4466579 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5133,7 +5133,8 @@ sub process { }x; my $all_barriers = qr{ $barriers| - smp_(?:$smp_barrier_stems) + smp_(?:$smp_barrier_stems)| + virt_(?:$smp_barrier_stems) }x; if ($line =~ /\b(?:$all_barriers)\s*\(/) { -- MST
Hi Michael, On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst at redhat.com> wrote:> Add virt_ barriers to list of barriers to check for > presence of a comment. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > scripts/checkpatch.pl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 15cfca4..4466579 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -5133,7 +5133,8 @@ sub process { > }x; > my $all_barriers = qr{ > $barriers| > - smp_(?:$smp_barrier_stems) > + smp_(?:$smp_barrier_stems)| > + virt_(?:$smp_barrier_stems)Sorry I'm late to the party here, but would it make sense to write this as: (?:smp|virt)_(?:$smp_barrier_stems) Thanks, -- Julian Calaby Email: julian.calaby at gmail.com Profile: google.com/profiles/julian.calaby