* [PATCH v3 0/3] checkpatch: handling of memory barriers @ 2016-01-10 19:30 Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 1/3] checkpatch.pl: add missing " Michael S. Tsirkin ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw) To: linux-kernel Cc: Andy Whitcroft, Joe Perches, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, linux-arm-kernel, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/3] checkpatch.pl: add missing memory barriers 2016-01-10 19:30 [PATCH v3 0/3] checkpatch: handling of memory barriers Michael S. Tsirkin @ 2016-01-10 19:30 ` Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 2/3] checkpatch: check for __smp outside barrier.h Michael S. Tsirkin 2016-01-10 19:31 ` [PATCH v3 3/3] checkpatch: add virt barriers Michael S. Tsirkin 2 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw) To: linux-kernel Cc: Andy Whitcroft, Joe Perches, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, linux-arm-kernel, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] checkpatch: check for __smp outside barrier.h 2016-01-10 19:30 [PATCH v3 0/3] checkpatch: handling of memory barriers Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 1/3] checkpatch.pl: add missing " Michael S. Tsirkin @ 2016-01-10 19:30 ` Michael S. Tsirkin 2016-01-10 19:31 ` [PATCH v3 3/3] checkpatch: add virt barriers Michael S. Tsirkin 2 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw) To: linux-kernel Cc: Andy Whitcroft, Joe Perches, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, linux-arm-kernel, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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@arm.linux.org.uk> 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-10 19:30 [PATCH v3 0/3] checkpatch: handling of memory barriers Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 1/3] checkpatch.pl: add missing " Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 2/3] checkpatch: check for __smp outside barrier.h Michael S. Tsirkin @ 2016-01-10 19:31 ` Michael S. Tsirkin 2016-01-10 22:13 ` Julian Calaby 2 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-10 19:31 UTC (permalink / raw) To: linux-kernel Cc: Andy Whitcroft, Joe Perches, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, linux-arm-kernel, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-10 19:31 ` [PATCH v3 3/3] checkpatch: add virt barriers Michael S. Tsirkin @ 2016-01-10 22:13 ` Julian Calaby 2016-01-10 22:52 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Julian Calaby @ 2016-01-10 22:13 UTC (permalink / raw) To: Michael S. Tsirkin Cc: linux-kernel, Andy Whitcroft, Joe Perches, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, Mailing List, Arm, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux Hi Michael, On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > 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) 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-10 22:13 ` Julian Calaby @ 2016-01-10 22:52 ` Joe Perches 2016-01-11 10:35 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2016-01-10 22:52 UTC (permalink / raw) To: Julian Calaby, Michael S. Tsirkin Cc: linux-kernel, Andy Whitcroft, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, Mailing List, Arm, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote: > On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > Add virt_ barriers to list of barriers to check for > > presence of a comment. [] > > diff --git 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) 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-10 22:52 ` Joe Perches @ 2016-01-11 10:35 ` Michael S. Tsirkin 2016-01-11 10:40 ` Julian Calaby 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-11 10:35 UTC (permalink / raw) To: Joe Perches Cc: Julian Calaby, linux-kernel, Andy Whitcroft, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, Mailing List, Arm, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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: > > On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > Add virt_ barriers to list of barriers to check for > > > presence of a comment. > [] > > > diff --git 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) > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-11 10:35 ` Michael S. Tsirkin @ 2016-01-11 10:40 ` Julian Calaby 2016-01-11 10:56 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Julian Calaby @ 2016-01-11 10:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Joe Perches, linux-kernel, Andy Whitcroft, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, Mailing List, Arm, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux Hi Michael, On Mon, Jan 11, 2016 at 9:35 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > 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: >> > On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > > Add virt_ barriers to list of barriers to check for >> > > presence of a comment. >> [] >> > > diff --git 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) >> >> 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] checkpatch: add virt barriers 2016-01-11 10:40 ` Julian Calaby @ 2016-01-11 10:56 ` Michael S. Tsirkin 0 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2016-01-11 10:56 UTC (permalink / raw) To: Julian Calaby Cc: Joe Perches, linux-kernel, Andy Whitcroft, Peter Zijlstra, Arnd Bergmann, linux-arch, Andrew Cooper, virtualization, Stefano Stabellini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, David Miller, linux-ia64, linuxppc-dev, linux-s390, sparclinux, Mailing List, Arm, linux-metag, linux-mips, x86, user-mode-linux-devel, adi-buildroot-devel, linux-sh, linux-xtensa, xen-devel, Ingo Molnar, Tony Lindgren, Andrey Konovalov, Russell King - ARM Linux 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 <mst@redhat.com> wrote: > > 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: > >> > On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > > Add virt_ barriers to list of barriers to check for > >> > > presence of a comment. > >> [] > >> > > diff --git 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) > >> > >> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-11 10:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-10 19:30 [PATCH v3 0/3] checkpatch: handling of memory barriers Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 1/3] checkpatch.pl: add missing " Michael S. Tsirkin 2016-01-10 19:30 ` [PATCH v3 2/3] checkpatch: check for __smp outside barrier.h Michael S. Tsirkin 2016-01-10 19:31 ` [PATCH v3 3/3] checkpatch: add virt barriers Michael S. Tsirkin 2016-01-10 22:13 ` Julian Calaby 2016-01-10 22:52 ` Joe Perches 2016-01-11 10:35 ` Michael S. Tsirkin 2016-01-11 10:40 ` Julian Calaby 2016-01-11 10:56 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).