* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-11 10:56 UTC (permalink / raw)
To: Julian Calaby
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, Mailing List, Arm
In-Reply-To: <CAGRGNgU7=vNXZbgSwLs+bBueX1+rKACCRMi74hM+jP8MaX+-WA@mail.gmail.com>
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
* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Julian Calaby @ 2016-01-11 10:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, Mailing List, Arm
In-Reply-To: <20160111123423-mutt-send-email-mst@redhat.com>
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
* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-11 10:35 UTC (permalink / raw)
To: Joe Perches
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
Andrey Konovalov, xen-devel, Ingo Molnar, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, Julian Calaby,
adi-buildroot-devel, Andy Whitcroft, Thomas Gleixner, linux-metag
In-Reply-To: <1452466336.7773.46.camel@perches.com>
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
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Minchan Kim @ 2016-01-10 23:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rafael Aquini, linux-kernel, stable, virtualization, linux-mm,
Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160110233310-mutt-send-email-mst@redhat.com>
On Sun, Jan 10, 2016 at 11:40:17PM +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 04, 2016 at 09:27:47AM +0900, Minchan Kim wrote:
> > > I think this will cause deadlocks.
> > >
> > > pages_lock now nests within page lock, balloon_page_putback
> > > nests them in the reverse order.
> >
> > In balloon_page_dequeu, we used trylock so I don't think it's
> > deadlock.
>
> I went over this again and I don't see the issue anymore.
> I think I was mistaken, so I dropped my patch and picked
> up yours. Sorry about the noise.
No problem. Thanks for the review.
^ permalink raw reply
* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Joe Perches @ 2016-01-10 22:52 UTC (permalink / raw)
To: Julian Calaby, Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, Mailing List, Arm
In-Reply-To: <CAGRGNgXQANbKD=VA0Qx4Wp1+MpZUVV7by8RrKxF9o=qu=vUQqA@mail.gmail.com>
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
* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Julian Calaby @ 2016-01-10 22:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, Mailing List, Arm
In-Reply-To: <1452454200-8844-4-git-send-email-mst@redhat.com>
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
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Michael S. Tsirkin @ 2016-01-10 21:40 UTC (permalink / raw)
To: Minchan Kim
Cc: Rafael Aquini, linux-kernel, stable, virtualization, linux-mm,
Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160104002747.GA31090@blaptop.local>
On Mon, Jan 04, 2016 at 09:27:47AM +0900, Minchan Kim wrote:
> > I think this will cause deadlocks.
> >
> > pages_lock now nests within page lock, balloon_page_putback
> > nests them in the reverse order.
>
> In balloon_page_dequeu, we used trylock so I don't think it's
> deadlock.
I went over this again and I don't see the issue anymore.
I think I was mistaken, so I dropped my patch and picked
up yours. Sorry about the noise.
> >
> > Also, there's another issue there I think: after isolation page could
> > also get freed before we try to lock it.
>
> If a page was isolated, the page shouldn't stay b_dev_info->pages
> list so balloon_page_dequeue cannot see the page.
> Am I missing something?
I mean without locks, as it is now. With either your or my patch in
place, it's fine.
--
MST
^ permalink raw reply
* [PATCH v3 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-10 19:31 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452454200-8844-1-git-send-email-mst@redhat.com>
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
* [PATCH v3 2/3] checkpatch: check for __smp outside barrier.h
From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452454200-8844-1-git-send-email-mst@redhat.com>
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
* [PATCH v3 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452454200-8844-1-git-send-email-mst@redhat.com>
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
* [PATCH v3 0/3] checkpatch: handling of memory barriers
From: Michael S. Tsirkin @ 2016-01-10 19:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
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
* Re: [PATCH v2 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 19:29 UTC (permalink / raw)
To: Joe Perches
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452439051.7773.27.camel@perches.com>
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
^ permalink raw reply
* Re: [PATCH v2 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 19:13 UTC (permalink / raw)
To: Joe Perches
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452438425.7773.21.camel@perches.com>
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);
^ permalink raw reply
* Re: [PATCH v2 1/3] checkpatch.pl: add missing memory barriers
From: Joe Perches @ 2016-01-10 15:17 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452438425.7773.21.camel@perches.com>
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
?
^ permalink raw reply
* Re: [PATCH v2 2/3] checkpatch: check for __smp outside barrier.h
From: Joe Perches @ 2016-01-10 15:08 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452427000-4520-3-git-send-email-mst@redhat.com>
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)) {
^ permalink raw reply
* Re: [PATCH v2 1/3] checkpatch.pl: add missing memory barriers
From: Joe Perches @ 2016-01-10 15:07 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452427000-4520-2-git-send-email-mst@redhat.com>
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);
^ permalink raw reply
* [PATCH v3 41/41] s390: more efficient smp barriers
From: Michael S. Tsirkin @ 2016-01-10 14:22 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, Heiko Carstens,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, x86, Christian Borntraeger, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
adi-buildroot-devel, Martin Schwidefsky, Thomas Gleixner,
linux-metag
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
As per: lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
atomics imply a barrier on s390, so s390 should change
smp_mb__before_atomic and smp_mb__after_atomic to barrier() instead of
smp_mb() and hence should not use the generic versions.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/s390/include/asm/barrier.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index 4d26fa4..5c8db3c 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -45,6 +45,9 @@ do { \
___p1; \
})
+#define __smp_mb__before_atomic() barrier()
+#define __smp_mb__after_atomic() barrier()
+
#include <asm-generic/barrier.h>
#endif /* __ASM_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 40/41] s390: use generic memory barriers
From: Michael S. Tsirkin @ 2016-01-10 14:22 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, Heiko Carstens,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, x86, Christian Borntraeger, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
adi-buildroot-devel, Martin Schwidefsky, Thomas Gleixner,
linux-metag
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
The s390 kernel is SMP to 99.99%, we just didn't bother with a
non-smp variant for the memory-barriers. If the generic header
is used we'd get the non-smp version for free. It will save a
small amount of text space for CONFIG_SMP=n.
Suggested-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/s390/include/asm/barrier.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index fbd25b2..4d26fa4 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -29,9 +29,6 @@
#define __smp_mb() mb()
#define __smp_rmb() rmb()
#define __smp_wmb() wmb()
-#define smp_mb() __smp_mb()
-#define smp_rmb() __smp_rmb()
-#define smp_wmb() __smp_wmb()
#define __smp_store_release(p, v) \
do { \
--
MST
^ permalink raw reply related
* [PATCH v3 39/41] xen/events: use virt_xxx barriers
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
Stefano Stabellini, H. Peter Anvin, sparclinux, Boris Ostrovsky,
linux-arch, linux-s390, Russell King - ARM Linux, Arnd Bergmann,
x86, xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Wei Liu,
Konrad Rzeszutek Wilk <konrad.wil>
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
drivers/xen/events/events_fifo.c uses rmb() to communicate with the
other side.
For guests compiled with CONFIG_SMP, smp_rmb would be sufficient, so
rmb() here is only needed if a non-SMP guest runs on an SMP host.
Switch to the virt_rmb barrier which serves this exact purpose.
Pull in asm/barrier.h here to make sure the file is self-contained.
Suggested-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/xen/events/events_fifo.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/events/events_fifo.c b/drivers/xen/events/events_fifo.c
index 96a1b8d..eff2b88 100644
--- a/drivers/xen/events/events_fifo.c
+++ b/drivers/xen/events/events_fifo.c
@@ -41,6 +41,7 @@
#include <linux/percpu.h>
#include <linux/cpu.h>
+#include <asm/barrier.h>
#include <asm/sync_bitops.h>
#include <asm/xen/hypercall.h>
#include <asm/xen/hypervisor.h>
@@ -296,7 +297,7 @@ static void consume_one_event(unsigned cpu,
* control block.
*/
if (head == 0) {
- rmb(); /* Ensure word is up-to-date before reading head. */
+ virt_rmb(); /* Ensure word is up-to-date before reading head. */
head = control_block->head[priority];
}
--
MST
^ permalink raw reply related
* [PATCH v3 38/41] xen/io: use virt_xxx barriers
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, Russell King - ARM Linux, Arnd Bergmann, x86,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel <david.vrab>
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
include/xen/interface/io/ring.h uses
full memory barriers to communicate with the other side.
For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.
Switch to virt_xxx barriers which serve this exact purpose.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
---
include/xen/interface/io/ring.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 7dc685b..21f4fbd 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -208,12 +208,12 @@ struct __name##_back_ring { \
#define RING_PUSH_REQUESTS(_r) do { \
- wmb(); /* back sees requests /before/ updated producer index */ \
+ virt_wmb(); /* back sees requests /before/ updated producer index */ \
(_r)->sring->req_prod = (_r)->req_prod_pvt; \
} while (0)
#define RING_PUSH_RESPONSES(_r) do { \
- wmb(); /* front sees responses /before/ updated producer index */ \
+ virt_wmb(); /* front sees responses /before/ updated producer index */ \
(_r)->sring->rsp_prod = (_r)->rsp_prod_pvt; \
} while (0)
@@ -250,9 +250,9 @@ struct __name##_back_ring { \
#define RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(_r, _notify) do { \
RING_IDX __old = (_r)->sring->req_prod; \
RING_IDX __new = (_r)->req_prod_pvt; \
- wmb(); /* back sees requests /before/ updated producer index */ \
+ virt_wmb(); /* back sees requests /before/ updated producer index */ \
(_r)->sring->req_prod = __new; \
- mb(); /* back sees new requests /before/ we check req_event */ \
+ virt_mb(); /* back sees new requests /before/ we check req_event */ \
(_notify) = ((RING_IDX)(__new - (_r)->sring->req_event) < \
(RING_IDX)(__new - __old)); \
} while (0)
@@ -260,9 +260,9 @@ struct __name##_back_ring { \
#define RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(_r, _notify) do { \
RING_IDX __old = (_r)->sring->rsp_prod; \
RING_IDX __new = (_r)->rsp_prod_pvt; \
- wmb(); /* front sees responses /before/ updated producer index */ \
+ virt_wmb(); /* front sees responses /before/ updated producer index */ \
(_r)->sring->rsp_prod = __new; \
- mb(); /* front sees new responses /before/ we check rsp_event */ \
+ virt_mb(); /* front sees new responses /before/ we check rsp_event */ \
(_notify) = ((RING_IDX)(__new - (_r)->sring->rsp_event) < \
(RING_IDX)(__new - __old)); \
} while (0)
@@ -271,7 +271,7 @@ struct __name##_back_ring { \
(_work_to_do) = RING_HAS_UNCONSUMED_REQUESTS(_r); \
if (_work_to_do) break; \
(_r)->sring->req_event = (_r)->req_cons + 1; \
- mb(); \
+ virt_mb(); \
(_work_to_do) = RING_HAS_UNCONSUMED_REQUESTS(_r); \
} while (0)
@@ -279,7 +279,7 @@ struct __name##_back_ring { \
(_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r); \
if (_work_to_do) break; \
(_r)->sring->rsp_event = (_r)->rsp_cons + 1; \
- mb(); \
+ virt_mb(); \
(_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r); \
} while (0)
--
MST
^ permalink raw reply related
* [PATCH v3 37/41] xenbus: use virt_xxx barriers
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, Russell King - ARM Linux, Arnd Bergmann, x86,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel <david.vrab>
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
drivers/xen/xenbus/xenbus_comms.c uses
full memory barriers to communicate with the other side.
For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.
Switch to virt_xxx barriers which serve this exact purpose.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
---
drivers/xen/xenbus/xenbus_comms.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index fdb0f33..ecdecce 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -123,14 +123,14 @@ int xb_write(const void *data, unsigned len)
avail = len;
/* Must write data /after/ reading the consumer index. */
- mb();
+ virt_mb();
memcpy(dst, data, avail);
data += avail;
len -= avail;
/* Other side must not see new producer until data is there. */
- wmb();
+ virt_wmb();
intf->req_prod += avail;
/* Implies mb(): other side will see the updated producer. */
@@ -180,14 +180,14 @@ int xb_read(void *data, unsigned len)
avail = len;
/* Must read data /after/ reading the producer index. */
- rmb();
+ virt_rmb();
memcpy(data, src, avail);
data += avail;
len -= avail;
/* Other side must not see free space until we've copied out */
- mb();
+ virt_mb();
intf->rsp_cons += avail;
pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);
--
MST
^ permalink raw reply related
* [PATCH v3 36/41] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Andy Whitcroft,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper,
Joe Perches, linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
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 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
^ permalink raw reply related
* [PATCH v3 35/41] checkpatch: check for __smp outside barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Andy Whitcroft,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper,
Joe Perches, linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
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 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
^ permalink raw reply related
* [PATCH v3 34/41] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Andy Whitcroft,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper,
Joe Perches, linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
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 | 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
^ permalink raw reply related
* [PATCH v3 33/41] virtio_ring: use virt_store_mb
From: Michael S. Tsirkin @ 2016-01-10 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, Joe Perches,
linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
We need a full barrier after writing out event index, using
virt_store_mb there seems better than open-coding. As usual, we need a
wrapper to account for strong barriers.
It's tempting to use this in vhost as well, for that, we'll
need a variant of smp_store_mb that works on __user pointers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 11 +++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index f3fa55b..a156e2b 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -45,6 +45,17 @@ static inline void virtio_wmb(bool weak_barriers)
wmb();
}
+static inline void virtio_store_mb(bool weak_barriers,
+ __virtio16 *p, __virtio16 v)
+{
+ if (weak_barriers) {
+ virt_store_mb(*p, v);
+ } else {
+ WRITE_ONCE(*p, v);
+ mb();
+ }
+}
+
struct virtio_device;
struct virtqueue;
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ee663c4..e12e385 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -517,10 +517,10 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
/* If we expect an interrupt for the next entry, tell host
* by writing event index and flush out the write before
* the read in the next get_buf call. */
- if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx);
- virtio_mb(vq->weak_barriers);
- }
+ if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
#ifdef DEBUG
vq->last_add_time_valid = false;
@@ -653,8 +653,11 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
}
/* TODO: tune this threshold */
bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs);
- virtio_mb(vq->weak_barriers);
+
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs));
+
if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
END_USE(vq);
return false;
--
MST
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox