[Xen-devel] [PATCH v2 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] checkpatch.pl: add missing memory barriers

[Joe Perches]

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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] checkpatch.pl: add missing memory barriers

[Joe Perches]

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
?

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel