Linux Virtualization - Jan 2016 - [PATCH 1/3] checkpatch.pl: add missing memory barriers

On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin
wrote:
> 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 | 9 ++++++++-
> ?1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b3c228..0245bbe 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5116,7 +5116,14 @@ 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 = ('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);

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:
> > +		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.

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:
> > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > +		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.
right, thanks.

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:
> > 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 | 9 ++++++++-
> > ?1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 2b3c228..0245bbe 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5116,7 +5116,14 @@ 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 = ('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.
> > +		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.
> 
> > ?			if (!ctx_has_comment($first_line, $linenr)) {
> > ?				WARN("MEMORY_BARRIER",
> > ?				?????"memory barrier without comment\n" . $herecurr);

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:
> > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > 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 | 9 ++++++++-
> > > ?1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 2b3c228..0245bbe 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5116,7 +5116,14 @@ 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 = ('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.
> 
I think using the qr form (like 'our $Attribute' and
a bunch of others) is the general style that should
be used for checkpatch.