[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

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 | 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

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 | 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@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 [off-list ref] wrote:

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@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote:

On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin [off-list ref] wrote:

quoted

Add virt_ barriers to list of barriers to check for
presence of a comment.

[]

quoted

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

quoted

@@ -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)

Yes.  Perhaps the name might be better as barrier_stems.

Also, ideally this would be longest match first or use \b
after the matches so that $all_barriers could work
successfully without a following \s*\(

my $all_barriers = qr{
	(?:smp|virt)_(?:barrier_stems)|
	$barriers)
}x;

or maybe add separate $smp_barriers and $virt_barriers

<shrug>  it doesn't matter much in any case

On Sun, Jan 10, 2016 at 02:52:16PM -0800, Joe Perches wrote:

On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote:

quoted

On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin [off-list ref] wrote:

quoted

Add virt_ barriers to list of barriers to check for
presence of a comment.

[]

quoted

quoted

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

quoted

quoted

@@ -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)

Yes.  Perhaps the name might be better as barrier_stems.

Also, ideally this would be longest match first or use \b
after the matches so that $all_barriers could work
successfully without a following \s*\(

my $all_barriers = qr{
	(?:smp|virt)_(?:barrier_stems)|
	$barriers)
}x;

or maybe add separate $smp_barriers and $virt_barriers

<shrug>  it doesn't matter much in any case

OK just to clarify - are you OK with merging the patch as is?
Refactorings can come as patches on top if required.

-- 
MST

Hi Michael,

On Mon, Jan 11, 2016 at 9:35 PM, Michael S. Tsirkin [off-list ref] wrote:

On Sun, Jan 10, 2016 at 02:52:16PM -0800, Joe Perches wrote:

quoted

On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote:

quoted

On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin [off-list ref] wrote:

quoted

Add virt_ barriers to list of barriers to check for
presence of a comment.

[]

quoted

quoted

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

quoted

quoted

@@ -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)

Yes.  Perhaps the name might be better as barrier_stems.

Also, ideally this would be longest match first or use \b
after the matches so that $all_barriers could work
successfully without a following \s*\(

my $all_barriers = qr{
      (?:smp|virt)_(?:barrier_stems)|
      $barriers)
}x;

or maybe add separate $smp_barriers and $virt_barriers

<shrug>  it doesn't matter much in any case

OK just to clarify - are you OK with merging the patch as is?
Refactorings can come as patches on top if required.

I don't really care either way, I was just asking if it was possible.
If you don't see any value in that change, then don't make it.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

On Mon, Jan 11, 2016 at 09:40:18PM +1100, Julian Calaby wrote:

Hi Michael,

On Mon, Jan 11, 2016 at 9:35 PM, Michael S. Tsirkin [off-list ref] wrote:

quoted

On Sun, Jan 10, 2016 at 02:52:16PM -0800, Joe Perches wrote:

quoted

On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote:

quoted

On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin [off-list ref] wrote:

quoted

Add virt_ barriers to list of barriers to check for
presence of a comment.

[]

quoted

quoted

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

[]

quoted

quoted

@@ -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)

Yes.  Perhaps the name might be better as barrier_stems.

Also, ideally this would be longest match first or use \b
after the matches so that $all_barriers could work
successfully without a following \s*\(

my $all_barriers = qr{
      (?:smp|virt)_(?:barrier_stems)|
      $barriers)
}x;

or maybe add separate $smp_barriers and $virt_barriers

<shrug>  it doesn't matter much in any case

OK just to clarify - are you OK with merging the patch as is?
Refactorings can come as patches on top if required.

I don't really care either way, I was just asking if it was possible.
If you don't see any value in that change, then don't make it.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

OK, got it, thanks.

I will rename smp_barrier_stems to barrier_stems since
this doesn't need too much testing.

I'd rather keep the regex code as is since changing it requires
testing.  I might play with it some more in the future
but I'd like to merge it in the current form to help make
sure __smp barriers are not misused.

I'll post v4 now - an ack will be appreciated.
-- 
MST