# \[PATCH v2 0/3\] checkpatch: handling of memory barriers - Subsystems: CHECKPATCH, THE REST - Maintainers: Andy Whitcroft, Joe Perches, Linus Torvalds - Series: 3 patches touching 1 file, +31/-2 - Status: SUPERSEDED (Superseded by v4 posted 2016-01-11) - Superseded by v4 (posted 2016-01-11). - Revision v2 (4 revisions archived). - List: linuxppc-dev - 9 messages, 2 authors - Page: https://ratatoskr.run/linuxppc-dev/2016/01/4656776/t ## \[PATCH v2 0/3\] checkpatch: handling of memory barriers - From: Michael S. Tsirkin \ - Date: 2016-01-10 11:57:03 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656776.md ```text 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. Changes from v2: 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 | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) -- MST ``` ## \[PATCH v2 1/3\] checkpatch.pl: add missing memory barriers - From: Michael S. Tsirkin \ - Date: 2016-01-10 11:57:12 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656777.md ```text SMP-only barriers were missing in checkpatch.pl Refactor code slightly to make adding more variants easier. Signed-off-by: Michael S. Tsirkin --- scripts/checkpatch.pl | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2b3c228..97b8b62 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5116,7 +5116,25 @@ 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_barriers = qr{ + store_release| + load_acquire| + store_mb| + ($barriers) + }x; + my $all_barriers = qr{ + $barriers| + smp_($smp_barriers) + }x; + + if ($line =~ /\b($all_barriers)\s*\(/) { if (!ctx_has_comment($first_line, $linenr)) { WARN("MEMORY_BARRIER", "memory barrier without comment\n" . $herecurr); -- MST ``` ## \[PATCH v2 2/3\] checkpatch: check for \_\_smp outside barrier.h - From: Michael S. Tsirkin \ - Date: 2016-01-10 11:57:22 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656778.md ```text 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 Signed-off-by: Michael S. Tsirkin --- scripts/checkpatch.pl | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 97b8b62..a96adcb 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5141,6 +5141,16 @@ sub process { } } + my $underscore_smp_barriers = qr{__smp_($smp_barriers)}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 ``` ## \[PATCH v2 3/3\] checkpatch: add virt barriers - From: Michael S. Tsirkin \ - Date: 2016-01-10 11:57:29 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656779.md ```text Add virt_ barriers to list of barriers to check for presence of a comment. Signed-off-by: Michael S. Tsirkin --- scripts/checkpatch.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a96adcb..5ca272b 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5131,7 +5131,8 @@ sub process { }x; my $all_barriers = qr{ $barriers| - smp_($smp_barriers) + smp_($smp_barriers)| + virt_($smp_barriers) }x; if ($line =~ /\b($all_barriers)\s*\(/) { -- MST ``` ## Re: \[PATCH v2 1/3\] checkpatch.pl: add missing memory barriers - From: Joe Perches \ - Date: 2016-01-10 15:07:14 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656843.md ```text On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > SMP-only barriers were missing in checkpatch.pl > > Refactor code slightly to make adding more variants easier. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -5116,7 +5116,25 @@ 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_barriers = qr{ > + store_release| > + load_acquire| > + store_mb| > + ($barriers) > + }x; If I use a variable called $smp_barriers, I'd expect it to actually be the smp_barriers, not to have to prefix it with smp_ before using it. my $smp_barriers = qr{ smp_store_release| smp_load_acquire| smp_store_mb| smp_read_barrier_depends }x; or my $smp_barriers = qr{ smp_(?:store_release|load_acquire|store_mb|read_barrier_depends) }x;   > + my $all_barriers = qr{ > + $barriers| > + smp_($smp_barriers) > + }x; And this shouldn't have a capture group. my $all_barriers = qr{ $barriers| $smp_barriers }x; > + > + if ($line =~ /\b($all_barriers)\s*\(/) { This doesn't need the capture group either (?:all_barriers) >   if (!ctx_has_comment($first_line, $linenr)) > { >   WARN("MEMORY_BARRIER", >        "memory barrier without > comment\n" . $herecurr); ``` ## Re: \[PATCH v2 2/3\] checkpatch: check for \_\_smp outside barrier.h - From: Joe Perches \ - Date: 2016-01-10 15:08:47 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656845.md ```text On Sun, 2016-01-10 at 13:57 +0200, Michael S. Tsirkin wrote: > 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. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -5141,6 +5141,16 @@ sub process { >   } >   } >   > + my $underscore_smp_barriers = qr{__smp_($smp_barriers)}x; another unnecessary capture group > + > + 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)) { ``` ## Re: \[PATCH v2 1/3\] checkpatch.pl: add missing memory barriers - From: Joe Perches \ - Date: 2016-01-10 15:17:45 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656850.md ```text On Sun, 2016-01-10 at 07:07 -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > SMP-only barriers were missing in checkpatch.pl > > > > Refactor code slightly to make adding more variants easier. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > If I use a variable called $smp_barriers, I'd expect > it to actually be the smp_barriers, not to have to > prefix it with smp_ before using it. > > my $smp_barriers = qr{ > smp_store_release| > smp_load_acquire| > smp_store_mb| > smp_read_barrier_depends That's missing (?:barriers) too. btw: shouldn't this also have smp_mb__(?:before|after)_atomic ? ``` ## Re: \[PATCH v2 1/3\] checkpatch.pl: add missing memory barriers - From: Michael S. Tsirkin \ - Date: 2016-01-10 19:13:54 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656886.md ```text On Sun, Jan 10, 2016 at 07:07:05AM -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > SMP-only barriers were missing in checkpatch.pl > > > > Refactor code slightly to make adding more variants easier. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -5116,7 +5116,25 @@ 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_barriers = qr{ > > + store_release| > > + load_acquire| > > + store_mb| > > + ($barriers) > > + }x; > > If I use a variable called $smp_barriers, I'd expect > it to actually be the smp_barriers, not to have to > prefix it with smp_ before using it. > > my $smp_barriers = qr{ > smp_store_release| > smp_load_acquire| > smp_store_mb| > smp_read_barrier_depends > }x; > > or > > my $smp_barriers = qr{ > smp_(?:store_release|load_acquire|store_mb|read_barrier_depends) > }x; > Yes but virt barriers (added in patch 3) are same things but prefixed with virt_. So we need the stems without smp_ prefix. If smp_barriers is too confusing we'll just need to give them some other name. How about: my $smp_barrier_stems ? > > + my $all_barriers = qr{ > > + $barriers| > > + smp_($smp_barriers) > > + }x; > > And this shouldn't have a capture group. > > my $all_barriers = qr{ > $barriers| > $smp_barriers > }x; > > + > > + if ($line =~ /\b($all_barriers)\s*\(/) { > > This doesn't need the capture group either (?:all_barriers) > > >   if (!ctx_has_comment($first_line, $linenr)) > > { > >   WARN("MEMORY_BARRIER", > >        "memory barrier without > > comment\n" . $herecurr); ``` ## Re: \[PATCH v2 1/3\] checkpatch.pl: add missing memory barriers - From: Michael S. Tsirkin \ - Date: 2016-01-10 19:29:22 - Message: https://ratatoskr.run/linuxppc-dev/2016/01/4656888.md ```text On Sun, Jan 10, 2016 at 07:17:31AM -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 07:07 -0800, Joe Perches wrote: > > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > > SMP-only barriers were missing in checkpatch.pl > > > > > > Refactor code slightly to make adding more variants easier. > > [] > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > If I use a variable called $smp_barriers, I'd expect > > it to actually be the smp_barriers, not to have to > > prefix it with smp_ before using it. > > > > my $smp_barriers = qr{ > > smp_store_release| > > smp_load_acquire| > > smp_store_mb| > > smp_read_barrier_depends > > That's missing (?:barriers) too. My version has it but need to add ?: to avoid a capture group. > btw: shouldn't this also have > smp_mb__(?:before|after)_atomic > ? Good catch, included in the next version. -- MST ```