Re: [PATCH 01/34] Documentation/memory-barriers.txt: document __smb_mb()
On Wed, 30 Dec 2015, Michael S. Tsirkin wrote: > Signed-off-by: Michael S. Tsirkin > --- > Documentation/memory-barriers.txt | 33 - > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/Documentation/memory-barriers.txt > b/Documentation/memory-barriers.txt > index aef9487..a20f7ef 100644 > --- a/Documentation/memory-barriers.txt > +++ b/Documentation/memory-barriers.txt > @@ -1655,17 +1655,18 @@ macro is a good place to start looking. > SMP memory barriers are reduced to compiler barriers on uniprocessor compiled > systems because it is assumed that a CPU will appear to be self-consistent, > and will order overlapping accesses correctly with respect to itself. > +However, see the subsection on "Virtual Machine Guests" below. > > [!] Note that SMP memory barriers _must_ be used to control the ordering of > references to shared memory on SMP systems, though the use of locking instead > is sufficient. > > Mandatory barriers should not be used to control SMP effects, since mandatory > -barriers unnecessarily impose overhead on UP systems. They may, however, be > -used to control MMIO effects on accesses through relaxed memory I/O windows. > -These are required even on non-SMP systems as they affect the order in which > -memory operations appear to a device by prohibiting both the compiler and the > -CPU from reordering them. > +barriers impose unnecessary overhead on both SMP and UP systems. They may, > +however, be used to control MMIO effects on accesses through relaxed memory > I/O > +windows. These barriers are required even on non-SMP systems as they affect > +the order in which memory operations appear to a device by prohibiting both > the > +compiler and the CPU from reordering them. > > > There are some more advanced barrier functions: > @@ -2948,6 +2949,28 @@ The Alpha defines the Linux kernel's memory barrier > model. > > See the subsection on "Cache Coherency" above. > > +VIRTUAL MACHINE GUESTS > +--- > + > +Guests running within virtual machines might be affected by > +SMP effects even if the guest itself is compiled within ^ without > +SMP support. > + > +This is an artifact of interfacing with an SMP host while > +running an UP kernel. > + > +Using mandatory barriers for this use-case would be possible > +but is often suboptimal. > + > +To handle this case optimally, low-level __smp_mb() etc macros are available. > +These have the same effect as smp_mb() etc when SMP is enabled, but generate > +identical code for SMP and non-SMP systems. For example, virtual machine > guests > +should use __smp_mb() rather than smp_mb() when synchronizing against a > +(possibly SMP) host. > + > +These are equivalent to smp_mb() etc counterparts in all other respects, > +in particular, they do not control MMIO effects: to control > +MMIO effects, use mandatory barriers. > > > EXAMPLE USES > -- > MST > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 01/34] Documentation/memory-barriers.txt: document __smb_mb()
On Mon, Jan 04, 2016 at 11:08:19AM +, Stefano Stabellini wrote: > On Wed, 30 Dec 2015, Michael S. Tsirkin wrote: > > Signed-off-by: Michael S. Tsirkin > > --- > > Documentation/memory-barriers.txt | 33 - > > 1 file changed, 28 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/memory-barriers.txt > > b/Documentation/memory-barriers.txt > > index aef9487..a20f7ef 100644 > > --- a/Documentation/memory-barriers.txt > > +++ b/Documentation/memory-barriers.txt > > @@ -1655,17 +1655,18 @@ macro is a good place to start looking. > > SMP memory barriers are reduced to compiler barriers on uniprocessor > > compiled > > systems because it is assumed that a CPU will appear to be self-consistent, > > and will order overlapping accesses correctly with respect to itself. > > +However, see the subsection on "Virtual Machine Guests" below. > > > > [!] Note that SMP memory barriers _must_ be used to control the ordering of > > references to shared memory on SMP systems, though the use of locking > > instead > > is sufficient. > > > > Mandatory barriers should not be used to control SMP effects, since > > mandatory > > -barriers unnecessarily impose overhead on UP systems. They may, however, be > > -used to control MMIO effects on accesses through relaxed memory I/O > > windows. > > -These are required even on non-SMP systems as they affect the order in > > which > > -memory operations appear to a device by prohibiting both the compiler and > > the > > -CPU from reordering them. > > +barriers impose unnecessary overhead on both SMP and UP systems. They may, > > +however, be used to control MMIO effects on accesses through relaxed > > memory I/O > > +windows. These barriers are required even on non-SMP systems as they > > affect > > +the order in which memory operations appear to a device by prohibiting > > both the > > +compiler and the CPU from reordering them. > > > > > > There are some more advanced barrier functions: > > @@ -2948,6 +2949,28 @@ The Alpha defines the Linux kernel's memory barrier > > model. > > > > See the subsection on "Cache Coherency" above. > > > > +VIRTUAL MACHINE GUESTS > > +--- > > + > > +Guests running within virtual machines might be affected by > > +SMP effects even if the guest itself is compiled within > > ^ without Right - this is fixed in v2. Could you review that one please? > > +SMP support. > > + > > +This is an artifact of interfacing with an SMP host while > > +running an UP kernel. > > + > > +Using mandatory barriers for this use-case would be possible > > +but is often suboptimal. > > + > > +To handle this case optimally, low-level __smp_mb() etc macros are > > available. > > +These have the same effect as smp_mb() etc when SMP is enabled, but > > generate > > +identical code for SMP and non-SMP systems. For example, virtual machine > > guests > > +should use __smp_mb() rather than smp_mb() when synchronizing against a > > +(possibly SMP) host. > > + > > +These are equivalent to smp_mb() etc counterparts in all other respects, > > +in particular, they do not control MMIO effects: to control > > +MMIO effects, use mandatory barriers. > > > > > > EXAMPLE USES > > -- > > MST > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v2 33/34] xenbus: use virt_xxx barriers
On 31/12/15 19:10, Michael S. Tsirkin wrote: > 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. Acked-by: David Vrabel If you're feeling particularly keen there's a rmb() consume_one_event() in drivers/xen/events/events_fifo.c that can be converted to virt_rmb() as well. David ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH v2 34/34] xen/io: use virt_xxx barriers
On 31/12/15 19:10, Michael S. Tsirkin wrote: > 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. Acked-by: David Vrabel David ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/3] checkpatch.pl: add missing memory barriers
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 | 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);
+ my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+ if ($line =~ /\b($all_barriers)\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
"memory barrier without comment\n" .
$herecurr);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/3] checkpatch: check for __smp outside barrier.h
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
Signed-off-by: Michael S. Tsirkin
---
scripts/checkpatch.pl | 11 +++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0245bbe..e3f9ad9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5130,6 +5130,17 @@ sub process {
}
}
+ my @underscore_smp_barriers = map {"__" . $_} @smp_barriers;
+ my $underscore_all_barriers = join('|',
@underscore_smp_barriers);
+
+ if ($realfile !~ m@^include/asm-generic/@ &&
+ $realfile !~ m@/barrier\.h$@ &&
+ $line =~ m/\b($underscore_all_barriers)\(/ &&
+ $line !~
m/^.\s*\#\s*define\s+($underscore_all_barriers)\(/) {
+ 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 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 applies on top of my series arch: barrier cleanup + barriers for virt and will be included in the next version of the series. Michael S. Tsirkin (3): checkpatch.pl: add missing memory barriers checkpatch: check for __smp outside barrier.h checkpatch: add virt barriers scripts/checkpatch.pl | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/3] checkpatch: add virt barriers
Add virt_ barriers to list of barriers to check for
presence of a comment.
Signed-off-by: Michael S. Tsirkin
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3f9ad9..5fb6ef7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5121,7 +5121,8 @@ sub process {
my @smp_barriers = ('smp_store_release', 'smp_load_acquire',
'smp_store_mb');
@smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
- my $all_barriers = join('|', (@barriers, @smp_barriers));
+ my @virt_barriers = map {my $l = $_; $l =~ s/smp_/virt_/; $l}
@smp_barriers;
+ my $all_barriers = join('|', (@barriers, @smp_barriers,
@virt_barriers));
if ($line =~ /\b($all_barriers)\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 31/34] xenbus: use __smp_XXX barriers
On Wed, 30 Dec 2015, Michael S. Tsirkin wrote:
> 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 __smp_XXX barriers which serve this exact purpose.
>
> Signed-off-by: Michael S. Tsirkin
Reviewed-by: Stefano Stabellini
> This is straight-forward, but untested.
> I can either merge this patchset through my tree if this is
> acked, or defer this and merge the patchset first,
> and xen bits through xen tree afterwards.
>
> Pls let me know.
I think it can go via your tree. However you have missed a few Xen
source files which need the same mb/__smb_mb conversions:
drivers/xen/grant-table.c
drivers/xen/evtchn.c
drivers/xen/events/events_fifo.c
drivers/xen/xen-scsiback.c
drivers/xen/tmem.c
drivers/xen/xen-pciback/pci_stub.c
drivers/xen/xen-pciback/pciback_ops.c
> 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..09b17c7 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();
> + __smp_mb();
>
> memcpy(dst, data, avail);
> data += avail;
> len -= avail;
>
> /* Other side must not see new producer until data is there. */
> - wmb();
> + __smp_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();
> + __smp_rmb();
>
> memcpy(data, src, avail);
> data += avail;
> len -= avail;
>
> /* Other side must not see free space until we've copied out */
> - mb();
> + __smp_mb();
> intf->rsp_cons += avail;
>
> pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);
> --
> MST
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 32/34] xen/io: use __smp_XXX barriers
On Wed, 30 Dec 2015, Michael S. Tsirkin wrote:
> 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 __smp_XXX barriers which serve this exact purpose.
>
> Signed-off-by: Michael S. Tsirkin
Reviewed-by: Stefano Stabellini
> 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..46dfc65 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 */ \
> +__smp_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 */
> \
> +__smp_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 */ \
> +__smp_wmb(); /* back sees requests /before/ updated producer index */
> \
> (_r)->sring->req_prod = __new; \
> -mb(); /* back sees new requests /before/ we check req_event */ \
> +__smp_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 */
> \
> +__smp_wmb(); /* front sees responses /before/ updated producer index */
> \
> (_r)->sring->rsp_prod = __new; \
> -mb(); /* front sees new responses /before/ we check rsp_event */ \
> +__smp_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();\
> +__smp_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();\
> +__smp_mb();
> \
> (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);
> \
> } while (0)
>
> --
> MST
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 33/34] xenbus: use virt_xxx barriers
On Thu, 31 Dec 2015, Michael S. Tsirkin wrote:
> 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
Reviewed-by: Stefano Stabellini
Are you also going to take care of
drivers/xen/grant-table.c
drivers/xen/evtchn.c
drivers/xen/events/events_fifo.c
drivers/xen/xen-scsiback.c
drivers/xen/tmem.c
drivers/xen/xen-pciback/pci_stub.c
drivers/xen/xen-pciback/pciback_ops.c
?
> 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
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 34/34] xen/io: use virt_xxx barriers
On Thu, 31 Dec 2015, Michael S. Tsirkin wrote:
> 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
Reviewed-by: Stefano Stabellini
> 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
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 06/32] s390: reuse asm-generic/barrier.h
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
> On s390 read_barrier_depends, smp_read_barrier_depends
> smp_store_mb(), smp_mb__before_atomic and smp_mb__after_atomic match the
> asm-generic variants exactly. Drop the local definitions and pull in
> asm-generic/barrier.h instead.
>
> This is in preparation to refactoring this code area.
>
> Signed-off-by: Michael S. Tsirkin
> Acked-by: Arnd Bergmann
> ---
> arch/s390/include/asm/barrier.h | 10 ++
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
> index 7ffd0b1..c358c31 100644
> --- a/arch/s390/include/asm/barrier.h
> +++ b/arch/s390/include/asm/barrier.h
> @@ -30,14 +30,6 @@
> #define smp_rmb()rmb()
> #define smp_wmb()wmb()
>
> -#define read_barrier_depends() do { } while (0)
> -#define smp_read_barrier_depends() do { } while (0)
> -
> -#define smp_mb__before_atomic() smp_mb()
> -#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 11/32] mips: reuse asm-generic/barrier.h
On Thu, Dec 31, 2015 at 09:07:10PM +0200, Michael S. Tsirkin wrote:
> -#define smp_store_release(p, v)
> \
> -do { \
> - compiletime_assert_atomic_type(*p); \
> - smp_mb(); \
> - WRITE_ONCE(*p, v); \
> -} while (0)
> -
> -#define smp_load_acquire(p) \
> -({ \
> - typeof(*p) ___p1 = READ_ONCE(*p); \
> - compiletime_assert_atomic_type(*p); \
> - smp_mb(); \
> - ___p1; \
> -})
David Daney wanted to use fancy new MIPS barriers to provide better
implementations of this.
This patch isn't in the way of that, just a FYI.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 17/32] arm: define __smp_xxx
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote: > On Sat, Jan 02, 2016 at 11:24:38AM +, Russell King - ARM Linux wrote: > > My only concern is that 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 (been there with > > other arch stuff before.) > > > > I wonder whether we should consider making the smp memory barriers > > inline functions, so these __smp_xxx() variants can be undef'd > > afterwards, thereby preventing drivers getting their hands on these > > new macros? > > That'd be tricky to do cleanly since asm-generic depends on > ifndef to add generic variants where needed. > > But it would be possible to add a checkpatch test for this. Wasn't the whole purpose of these things for 'drivers' (namely virtio/xen hypervisor interaction) to use these? And I suppose most of virtio would actually be modules, so you cannot do what I did with preempt_enable_no_resched() either. But yes, it would be good to limit the use of these things. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 20/32] metag: define __smp_xxx
On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
> +#ifdef CONFIG_SMP
> +#define fence() metag_fence()
> +#else
> +#define fence() do { } while (0)
> #endif
James, it strikes me as odd that fence() is a no-op instead of a
barrier() for UP, can you verify/explain?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 22/32] s390: define __smp_xxx
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote: > This defines __smp_xxx barriers for s390, > for use by virtualization. > > Some smp_xxx barriers are removed as they are > defined correctly by asm-generic/barriers.h > > Note: smp_mb, smp_rmb and smp_wmb are defined as full barriers > unconditionally on this architecture. > > Signed-off-by: Michael S. Tsirkin > Acked-by: Arnd Bergmann > --- > arch/s390/include/asm/barrier.h | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h > index c358c31..fbd25b2 100644 > --- a/arch/s390/include/asm/barrier.h > +++ b/arch/s390/include/asm/barrier.h > @@ -26,18 +26,21 @@ > #define wmb()barrier() > #define dma_rmb()mb() > #define dma_wmb()mb() > -#define smp_mb() mb() > -#define smp_rmb()rmb() > -#define smp_wmb()wmb() > - > -#define smp_store_release(p, v) > \ > +#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() Why define the smp_*mb() primitives here? Would not the inclusion of asm-generic/barrier.h do this? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 17/32] arm: define __smp_xxx
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote: > On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote: > > On Sat, Jan 02, 2016 at 11:24:38AM +, Russell King - ARM Linux wrote: > > > > My only concern is that 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 (been there with > > > other arch stuff before.) > > > > > > I wonder whether we should consider making the smp memory barriers > > > inline functions, so these __smp_xxx() variants can be undef'd > > > afterwards, thereby preventing drivers getting their hands on these > > > new macros? > > > > That'd be tricky to do cleanly since asm-generic depends on > > ifndef to add generic variants where needed. > > > > But it would be possible to add a checkpatch test for this. > > Wasn't the whole purpose of these things for 'drivers' (namely > virtio/xen hypervisor interaction) to use these? Ah, I see, you add virt_*mb() stuff later on for that use case. So, assuming everybody does include asm-generic/barrier.h, you could simply #undef the __smp version at the end of that, once we've generated all the regular primitives from it, no? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 17/32] arm: define __smp_xxx
On Mon, Jan 04, 2016 at 02:54:20PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote:
> > On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
> > > On Sat, Jan 02, 2016 at 11:24:38AM +, Russell King - ARM Linux wrote:
> >
> > > > My only concern is that 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 (been there with
> > > > other arch stuff before.)
> > > >
> > > > I wonder whether we should consider making the smp memory barriers
> > > > inline functions, so these __smp_xxx() variants can be undef'd
> > > > afterwards, thereby preventing drivers getting their hands on these
> > > > new macros?
> > >
> > > That'd be tricky to do cleanly since asm-generic depends on
> > > ifndef to add generic variants where needed.
> > >
> > > But it would be possible to add a checkpatch test for this.
> >
> > Wasn't the whole purpose of these things for 'drivers' (namely
> > virtio/xen hypervisor interaction) to use these?
>
> Ah, I see, you add virt_*mb() stuff later on for that use case.
>
> So, assuming everybody does include asm-generic/barrier.h, you could
> simply #undef the __smp version at the end of that, once we've generated
> all the regular primitives from it, no?
Not so simple - that's why I mentioned using inline functions.
The new smp_* _macros_ are:
+#define smp_mb() __smp_mb()
which means if we simply #undef __smp_mb(), smp_mb() then points at
something which is no longer available, and we'll end up with errors
saying that __smp_mb() doesn't exist.
My suggestion was to change:
#ifndef smp_mb
#define smp_mb()__smp_mb()
#endif
to:
#ifndef smp_mb
static inline void smp_mb(void)
{
__smp_mb();
}
#endif
which then means __smp_mb() and friends can be #undef'd afterwards.
--
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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb
On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
> At the moment, xchg on sh only supports 4 and 1 byte values, so using it
> from smp_store_mb means attempts to store a 2 byte value using this
> macro fail.
>
> And happens to be exactly what virtio drivers want to do.
>
> Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
>
> Signed-off-by: Michael S. Tsirkin
> ---
> arch/sh/include/asm/barrier.h | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/sh/include/asm/barrier.h b/arch/sh/include/asm/barrier.h
> index f887c64..0cc5735 100644
> --- a/arch/sh/include/asm/barrier.h
> +++ b/arch/sh/include/asm/barrier.h
> @@ -32,7 +32,15 @@
> #define ctrl_barrier() __asm__ __volatile__
> ("nop;nop;nop;nop;nop;nop;nop;nop")
> #endif
>
> -#define __smp_store_mb(var, value) do { (void)xchg(&var, value); } while (0)
> +#define __smp_store_mb(var, value) do { \
> + if (sizeof(var) != 4 && sizeof(var) != 1) { \
> + WRITE_ONCE(var, value); \
> + __smp_mb(); \
> + } else { \
> + (void)xchg(&var, value); \
> + } \
> +} while (0)
So SH is an orphaned arch, which is also why I did not comment on using
xchg() for the UP smp_store_mb() thing.
But I really think we should try fixing the xchg() implementation
instead of this duct-tape.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 33/34] xenbus: use virt_xxx barriers
On Thu, Dec 31, 2015 at 09:10:01PM +0200, Michael S. Tsirkin wrote: > 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 > --- > 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(); > So its possible to remove this barrier entirely, see the "CONTROL DEPENDNCIES" chunk of memory-barrier.txt. But do that in a separate patch series and only if you really really need the performance. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 06/32] s390: reuse asm-generic/barrier.h
On Mon, 4 Jan 2016 14:20:42 +0100
Peter Zijlstra wrote:
> On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
> > On s390 read_barrier_depends, smp_read_barrier_depends
> > smp_store_mb(), smp_mb__before_atomic and smp_mb__after_atomic match the
> > asm-generic variants exactly. Drop the local definitions and pull in
> > asm-generic/barrier.h instead.
> >
> > This is in preparation to refactoring this code area.
> >
> > Signed-off-by: Michael S. Tsirkin
> > Acked-by: Arnd Bergmann
> > ---
> > arch/s390/include/asm/barrier.h | 10 ++
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/barrier.h
> > b/arch/s390/include/asm/barrier.h
> > index 7ffd0b1..c358c31 100644
> > --- a/arch/s390/include/asm/barrier.h
> > +++ b/arch/s390/include/asm/barrier.h
> > @@ -30,14 +30,6 @@
> > #define smp_rmb() rmb()
> > #define smp_wmb() wmb()
> >
> > -#define read_barrier_depends() do { } while (0)
> > -#define smp_read_barrier_depends() do { } while (0)
> > -
> > -#define smp_mb__before_atomic()smp_mb()
> > -#define smp_mb__after_atomic() smp_mb()
>
> As per:
>
> lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
>
> s390 should change this to barrier() instead of smp_mb() and hence
> should not use the generic versions.
Yes, we wanted to simplify this. Thanks for the reminder, I'll queue
a patch.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 20/32] metag: define __smp_xxx
On Mon, Jan 04, 2016 at 03:25:58PM +, James Hogan wrote: > It is used along with the metag specific __global_lock1() (global > voluntary lock between hw threads) whenever a write is performed, and by > smp_mb/smp_rmb to try to catch other cases, but I've never been > confident this fixes every single corner case, since there could be > other places where multiple CPUs perform unsynchronised writes to the > same memory location, and expect cache not to become incoherent at that > location. Ah, yuck, I thought blackfin was the only one attempting !coherent SMP. And yes, this is bound to break in lots of places in subtle ways. We very much assume cache coherency for SMP in generic code. > It seemed to be sufficient to achieve stability however, and SMP on Meta > Linux never made it into a product anyway, since the other hw thread > tended to be used for RTOS stuff, so it didn't seem worth extending the > generic barrier API for it. *phew*, should we take it out then, just to be sure nobody accidentally tries to use it then? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 20/32] metag: define __smp_xxx
Hi Peter,
On Mon, Jan 04, 2016 at 02:41:28PM +0100, Peter Zijlstra wrote:
> On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
> > +#ifdef CONFIG_SMP
> > +#define fence() metag_fence()
> > +#else
> > +#define fence()do { } while (0)
> > #endif
>
> James, it strikes me as odd that fence() is a no-op instead of a
> barrier() for UP, can you verify/explain?
fence() is an unfortunate workaround for a specific issue on a certain
SoC, where writes from different hw threads get reordered outside of the
core, resulting in incoherency between RAM and cache. It has slightly
different semantics to the normal SMP barriers, since I was assured it
is required before a write rather than after it.
Here's the comment:
> This is needed before a write to shared memory in a critical section,
> to prevent external reordering of writes before the fence on other
> threads with writes after the fence on this thread (and to prevent the
> ensuing cache-memory incoherence). It is therefore ineffective if used
> after and on the same thread as a write.
It is used along with the metag specific __global_lock1() (global
voluntary lock between hw threads) whenever a write is performed, and by
smp_mb/smp_rmb to try to catch other cases, but I've never been
confident this fixes every single corner case, since there could be
other places where multiple CPUs perform unsynchronised writes to the
same memory location, and expect cache not to become incoherent at that
location.
It seemed to be sufficient to achieve stability however, and SMP on Meta
Linux never made it into a product anyway, since the other hw thread
tended to be used for RTOS stuff, so it didn't seem worth extending the
generic barrier API for it.
Cheers
James
signature.asc
Description: Digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] vhost: basic device IOTLB support
On 2015/12/31 15:13, Jason Wang wrote: This patch tries to implement an device IOTLB for vhost. This could be used with for co-operation with userspace(qemu) implementation of iommu for a secure DMA environment in guest. The idea is simple. When vhost meets an IOTLB miss, it will request the assistance of userspace to do the translation, this is done through: - Fill the translation request in a preset userspace address (This address is set through ioctl VHOST_SET_IOTLB_REQUEST_ENTRY). - Notify userspace through eventfd (This eventfd was set through ioctl VHOST_SET_IOTLB_FD). When userspace finishes the translation, it will update the vhost IOTLB through VHOST_UPDATE_IOTLB ioctl. Userspace is also in charge of snooping the IOTLB invalidation of IOMMU IOTLB and use VHOST_UPDATE_IOTLB to invalidate the possible entry in vhost. Is there any performance data shows the difference with IOTLB supporting? I doubt we may see performance decrease since the flush code path is longer than before. -- best regards yang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 20/32] metag: define __smp_xxx
On Mon, Jan 04, 2016 at 04:30:36PM +0100, Peter Zijlstra wrote: > On Mon, Jan 04, 2016 at 03:25:58PM +, James Hogan wrote: > > It is used along with the metag specific __global_lock1() (global > > voluntary lock between hw threads) whenever a write is performed, and by > > smp_mb/smp_rmb to try to catch other cases, but I've never been > > confident this fixes every single corner case, since there could be > > other places where multiple CPUs perform unsynchronised writes to the > > same memory location, and expect cache not to become incoherent at that > > location. > > Ah, yuck, I thought blackfin was the only one attempting !coherent SMP. > And yes, this is bound to break in lots of places in subtle ways. We > very much assume cache coherency for SMP in generic code. Well, its usually completely coherent, its just a bit dodgy in a particular hardware corner case, which was pretty hard to hit, even without these workarounds. > > > It seemed to be sufficient to achieve stability however, and SMP on Meta > > Linux never made it into a product anyway, since the other hw thread > > tended to be used for RTOS stuff, so it didn't seem worth extending the > > generic barrier API for it. > > *phew*, should we take it out then, just to be sure nobody accidentally > tries to use it then? SMP support on this SoC you mean? I doubt it'll be a problem tbh, and it'd work fine in QEMU when emulating this SoC, so I'd prefer to keep it in. Cheers James signature.asc Description: Digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers
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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] checkpatch: add virt barriers
On Mon, 2016-01-04 at 13:37 +0200, Michael S. Tsirkin wrote: > Add virt_ barriers to list of barriers to check for > presence of a comment. Are these virt_ barriers used anywhere? I see some virtio_ barrier like uses. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 17/32] arm: define __smp_xxx
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote: > On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote: > > On Sat, Jan 02, 2016 at 11:24:38AM +, Russell King - ARM Linux wrote: > > > > My only concern is that 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 (been there with > > > other arch stuff before.) > > > > > > I wonder whether we should consider making the smp memory barriers > > > inline functions, so these __smp_xxx() variants can be undef'd > > > afterwards, thereby preventing drivers getting their hands on these > > > new macros? > > > > That'd be tricky to do cleanly since asm-generic depends on > > ifndef to add generic variants where needed. > > > > But it would be possible to add a checkpatch test for this. > > Wasn't the whole purpose of these things for 'drivers' (namely > virtio/xen hypervisor interaction) to use these? My take out from discussion with you was that virtualization is probably the only valid use-case. So at David Miller's suggestion there's a patch later in the series that adds virt_ wrappers and these are then used by virtio xen and later maybe others. > And I suppose most of virtio would actually be modules, so you cannot do > what I did with preempt_enable_no_resched() either. > > But yes, it would be good to limit the use of these things. Right so the trick is checkpatch warns about use of __smp_xxx and hopefully people are not crazy enough to use virt_xxx variants for non-virtual drivers. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 22/32] s390: define __smp_xxx
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote: > On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote: > > This defines __smp_xxx barriers for s390, > > for use by virtualization. > > > > Some smp_xxx barriers are removed as they are > > defined correctly by asm-generic/barriers.h > > > > Note: smp_mb, smp_rmb and smp_wmb are defined as full barriers > > unconditionally on this architecture. > > > > Signed-off-by: Michael S. Tsirkin > > Acked-by: Arnd Bergmann > > --- > > arch/s390/include/asm/barrier.h | 15 +-- > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/arch/s390/include/asm/barrier.h > > b/arch/s390/include/asm/barrier.h > > index c358c31..fbd25b2 100644 > > --- a/arch/s390/include/asm/barrier.h > > +++ b/arch/s390/include/asm/barrier.h > > @@ -26,18 +26,21 @@ > > #define wmb() barrier() > > #define dma_rmb() mb() > > #define dma_wmb() mb() > > -#define smp_mb() mb() > > -#define smp_rmb() rmb() > > -#define smp_wmb() wmb() > > - > > -#define smp_store_release(p, v) > > \ > > +#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() > > Why define the smp_*mb() primitives here? Would not the inclusion of > asm-generic/barrier.h do this? No because the generic one is a nop on !SMP, this one isn't. Pls note this patch is just reordering code without making functional changes. And at the moment, on s390 smp_xxx barriers are always non empty. Some of this could be sub-optimal, but since on s390 Linux always runs on a hypervisor, I am not sure it's safe to use the generic version - in other words, it just might be that for s390 smp_ and virt_ barriers must be equivalent. If in fact this turns out to be wrong, I can pick up a patch to change this, but I'd rather make this a patch on top so that my patches are testable just by compiling and comparing the binary. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 06/32] s390: reuse asm-generic/barrier.h
On Mon, Jan 04, 2016 at 02:20:42PM +0100, Peter Zijlstra wrote:
> On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
> > On s390 read_barrier_depends, smp_read_barrier_depends
> > smp_store_mb(), smp_mb__before_atomic and smp_mb__after_atomic match the
> > asm-generic variants exactly. Drop the local definitions and pull in
> > asm-generic/barrier.h instead.
> >
> > This is in preparation to refactoring this code area.
> >
> > Signed-off-by: Michael S. Tsirkin
> > Acked-by: Arnd Bergmann
> > ---
> > arch/s390/include/asm/barrier.h | 10 ++
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/barrier.h
> > b/arch/s390/include/asm/barrier.h
> > index 7ffd0b1..c358c31 100644
> > --- a/arch/s390/include/asm/barrier.h
> > +++ b/arch/s390/include/asm/barrier.h
> > @@ -30,14 +30,6 @@
> > #define smp_rmb() rmb()
> > #define smp_wmb() wmb()
> >
> > -#define read_barrier_depends() do { } while (0)
> > -#define smp_read_barrier_depends() do { } while (0)
> > -
> > -#define smp_mb__before_atomic()smp_mb()
> > -#define smp_mb__after_atomic() smp_mb()
>
> As per:
>
> lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
>
> s390 should change this to barrier() instead of smp_mb() and hence
> should not use the generic versions.
Thanks Peter!
OK so I will just rename this to __smp_mb__before_atomic and
__smp_mb__after_atomic but keep them around.
I'm not changing these - that's best left to s390 maintainers.
Should I add a TODO comment to change them to barrier so
we don't forget?
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 17/32] arm: define __smp_xxx
On Mon, Jan 04, 2016 at 02:54:20PM +0100, Peter Zijlstra wrote: > On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote: > > On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote: > > > On Sat, Jan 02, 2016 at 11:24:38AM +, Russell King - ARM Linux wrote: > > > > > > My only concern is that 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 (been there with > > > > other arch stuff before.) > > > > > > > > I wonder whether we should consider making the smp memory barriers > > > > inline functions, so these __smp_xxx() variants can be undef'd > > > > afterwards, thereby preventing drivers getting their hands on these > > > > new macros? > > > > > > That'd be tricky to do cleanly since asm-generic depends on > > > ifndef to add generic variants where needed. > > > > > > But it would be possible to add a checkpatch test for this. > > > > Wasn't the whole purpose of these things for 'drivers' (namely > > virtio/xen hypervisor interaction) to use these? > > Ah, I see, you add virt_*mb() stuff later on for that use case. > > So, assuming everybody does include asm-generic/barrier.h, you could > simply #undef the __smp version at the end of that, once we've generated > all the regular primitives from it, no? Maybe I misunderstand, but I don't think so: --> #define __smp_xxx FOO #define smp_xxx __smp_xxx #undef __smp_xxx smp_xxx <-- resolves to __smp_xxx, not FOO. That's why I went the checkpatch way. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 06/32] s390: reuse asm-generic/barrier.h
On Mon, Jan 04, 2016 at 04:03:39PM +0100, Martin Schwidefsky wrote:
> On Mon, 4 Jan 2016 14:20:42 +0100
> Peter Zijlstra wrote:
>
> > On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
> > > On s390 read_barrier_depends, smp_read_barrier_depends
> > > smp_store_mb(), smp_mb__before_atomic and smp_mb__after_atomic match the
> > > asm-generic variants exactly. Drop the local definitions and pull in
> > > asm-generic/barrier.h instead.
> > >
> > > This is in preparation to refactoring this code area.
> > >
> > > Signed-off-by: Michael S. Tsirkin
> > > Acked-by: Arnd Bergmann
> > > ---
> > > arch/s390/include/asm/barrier.h | 10 ++
> > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/s390/include/asm/barrier.h
> > > b/arch/s390/include/asm/barrier.h
> > > index 7ffd0b1..c358c31 100644
> > > --- a/arch/s390/include/asm/barrier.h
> > > +++ b/arch/s390/include/asm/barrier.h
> > > @@ -30,14 +30,6 @@
> > > #define smp_rmb()rmb()
> > > #define smp_wmb()wmb()
> > >
> > > -#define read_barrier_depends() do { } while (0)
> > > -#define smp_read_barrier_depends() do { } while (0)
> > > -
> > > -#define smp_mb__before_atomic() smp_mb()
> > > -#define smp_mb__after_atomic() smp_mb()
> >
> > As per:
> >
> > lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
> >
> > s390 should change this to barrier() instead of smp_mb() and hence
> > should not use the generic versions.
>
> Yes, we wanted to simplify this. Thanks for the reminder, I'll queue
> a patch.
Could you base on my patchset maybe, to avoid conflicts,
and I'll merge it?
Or if it's just replacing these 2 with barrier() I can do this
myself easily.
> --
> blue skies,
>Martin.
>
> "Reality continues to ruin my life." - Calvin.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers
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
> > ---
> > 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);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [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
> ---
> 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);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] checkpatch: add virt barriers
On Mon, Jan 04, 2016 at 08:47:53AM -0800, Joe Perches wrote: > On Mon, 2016-01-04 at 13:37 +0200, Michael S. Tsirkin wrote: > > Add virt_ barriers to list of barriers to check for > > presence of a comment. > > Are these virt_ barriers used anywhere? > > I see some virtio_ barrier like uses. They will be :) They are added and used by patchset arch: barrier cleanup + barriers for virt See http://article.gmane.org/gmane.linux.kernel.virtualization/26555 -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] checkpatch: add virt barriers
On Mon, 2016-01-04 at 23:07 +0200, Michael S. Tsirkin wrote: > On Mon, Jan 04, 2016 at 08:47:53AM -0800, Joe Perches wrote: > > On Mon, 2016-01-04 at 13:37 +0200, Michael S. Tsirkin wrote: > > > Add virt_ barriers to list of barriers to check for > > > presence of a comment. > > > > Are these virt_ barriers used anywhere? > > > > I see some virtio_ barrier like uses. > > They will be :) They are added and used by patchset > arch: barrier cleanup + barriers for virt > > See > http://article.gmane.org/gmane.linux.kernel.virtualization/26555 Ah, OK, thanks. Are the virtio_ barriers going away? If not, maybe those should be added too. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 10/32] metag: reuse asm-generic/barrier.h
On Thu, Dec 31, 2015 at 09:07:02PM +0200, Michael S. Tsirkin wrote:
> On metag dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
> smp_read_barrier_depends, smp_store_release and smp_load_acquire match
> the asm-generic variants exactly. Drop the local definitions and pull in
> asm-generic/barrier.h instead.
>
> This is in preparation to refactoring this code area.
>
> Signed-off-by: Michael S. Tsirkin
> Acked-by: Arnd Bergmann
Looks good, and confirmed no text change (once patch 1 is applied that
is).
Acked-by: James Hogan
Thanks
James
> ---
> arch/metag/include/asm/barrier.h | 25 ++---
> 1 file changed, 2 insertions(+), 23 deletions(-)
>
> diff --git a/arch/metag/include/asm/barrier.h
> b/arch/metag/include/asm/barrier.h
> index 172b7e5..b5b778b 100644
> --- a/arch/metag/include/asm/barrier.h
> +++ b/arch/metag/include/asm/barrier.h
> @@ -44,9 +44,6 @@ static inline void wr_fence(void)
> #define rmb()barrier()
> #define wmb()mb()
>
> -#define dma_rmb()rmb()
> -#define dma_wmb()wmb()
> -
> #ifndef CONFIG_SMP
> #define fence() do { } while (0)
> #define smp_mb()barrier()
> @@ -81,27 +78,9 @@ static inline void fence(void)
> #endif
> #endif
>
> -#define read_barrier_depends() do { } while (0)
> -#define smp_read_barrier_depends() do { } while (0)
> -
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); }
> while (0)
> -
> -#define smp_store_release(p, v)
> \
> -do { \
> - compiletime_assert_atomic_type(*p); \
> - smp_mb(); \
> - WRITE_ONCE(*p, v); \
> -} while (0)
> -
> -#define smp_load_acquire(p) \
> -({ \
> - typeof(*p) ___p1 = READ_ONCE(*p); \
> - compiletime_assert_atomic_type(*p); \
> - smp_mb(); \
> - ___p1; \
> -})
> -
> #define smp_mb__before_atomic() barrier()
> #define smp_mb__after_atomic() barrier()
>
> +#include
> +
> #endif /* _ASM_METAG_BARRIER_H */
> --
> MST
>
signature.asc
Description: Digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers
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
> > > ---
> > > 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 20/32] metag: define __smp_xxx
Hi Michael,
On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
> This defines __smp_xxx barriers for metag,
> for use by virtualization.
>
> smp_xxx barriers are removed as they are
> defined correctly by asm-generic/barriers.h
>
> Note: as __smp_XX macros should not depend on CONFIG_SMP, they can not
> use the existing fence() macro since that is defined differently between
> SMP and !SMP. For this reason, this patch introduces a wrapper
> metag_fence() that doesn't depend on CONFIG_SMP.
> fence() is then defined using that, depending on CONFIG_SMP.
I'm not a fan of the inconsistent commit message wrapping. I wrap to 72
columns (although I now notice SubmittingPatches says to use 75...).
>
> Signed-off-by: Michael S. Tsirkin
> Acked-by: Arnd Bergmann
> ---
> arch/metag/include/asm/barrier.h | 32 +++-
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/arch/metag/include/asm/barrier.h
> b/arch/metag/include/asm/barrier.h
> index b5b778b..84880c9 100644
> --- a/arch/metag/include/asm/barrier.h
> +++ b/arch/metag/include/asm/barrier.h
> @@ -44,13 +44,6 @@ static inline void wr_fence(void)
> #define rmb()barrier()
> #define wmb()mb()
>
> -#ifndef CONFIG_SMP
> -#define fence() do { } while (0)
> -#define smp_mb()barrier()
> -#define smp_rmb() barrier()
> -#define smp_wmb() barrier()
> -#else
!SMP kernel text differs, but only because of new presence of unused
metag_fence() inline function. If I #if 0 that out, then it matches, so
thats fine.
> -
> #ifdef CONFIG_METAG_SMP_WRITE_REORDERING
> /*
> * Write to the atomic memory unlock system event register (command 0). This
> is
> @@ -60,26 +53,31 @@ static inline void wr_fence(void)
> * incoherence). It is therefore ineffective if used after and on the same
> * thread as a write.
> */
> -static inline void fence(void)
> +static inline void metag_fence(void)
> {
> volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK;
> barrier();
> *flushptr = 0;
> barrier();
> }
> -#define smp_mb()fence()
> -#define smp_rmb() fence()
> -#define smp_wmb() barrier()
> +#define __smp_mb()metag_fence()
> +#define __smp_rmb() metag_fence()
> +#define __smp_wmb() barrier()
> #else
> -#define fence() do { } while (0)
> -#define smp_mb()barrier()
> -#define smp_rmb() barrier()
> -#define smp_wmb() barrier()
> +#define metag_fence()do { } while (0)
> +#define __smp_mb()barrier()
> +#define __smp_rmb() barrier()
> +#define __smp_wmb() barrier()
Whitespace is now messed up. Admitedly its already inconsistent
tabs/spaces, but it'd be nice if the definitions at least still all
lined up. You're touching all the definitions which use spaces anyway,
so feel free to convert them to tabs while you're at it.
Other than those niggles, it looks sensible to me:
Acked-by: James Hogan
Cheers
James
> #endif
> +
> +#ifdef CONFIG_SMP
> +#define fence() metag_fence()
> +#else
> +#define fence() do { } while (0)
> #endif
>
> -#define smp_mb__before_atomic() barrier()
> -#define smp_mb__after_atomic() barrier()
> +#define __smp_mb__before_atomic()barrier()
> +#define __smp_mb__after_atomic() barrier()
>
> #include
>
> --
> MST
>
signature.asc
Description: Digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers
On Mon, 2016-01-04 at 16:11 +, 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] vhost: basic device IOTLB support
On 2016/1/4 14:22, Jason Wang wrote: On 01/04/2016 09:39 AM, Yang Zhang wrote: On 2015/12/31 15:13, Jason Wang wrote: This patch tries to implement an device IOTLB for vhost. This could be used with for co-operation with userspace(qemu) implementation of iommu for a secure DMA environment in guest. The idea is simple. When vhost meets an IOTLB miss, it will request the assistance of userspace to do the translation, this is done through: - Fill the translation request in a preset userspace address (This address is set through ioctl VHOST_SET_IOTLB_REQUEST_ENTRY). - Notify userspace through eventfd (This eventfd was set through ioctl VHOST_SET_IOTLB_FD). When userspace finishes the translation, it will update the vhost IOTLB through VHOST_UPDATE_IOTLB ioctl. Userspace is also in charge of snooping the IOTLB invalidation of IOMMU IOTLB and use VHOST_UPDATE_IOTLB to invalidate the possible entry in vhost. Is there any performance data shows the difference with IOTLB supporting? Basic testing show it was slower than without IOTLB. I doubt we may see performance decrease since the flush code path is longer than before. Yes, it also depend on the TLB hit rate. If lots of dynamic mappings and unmappings are used in guest (e.g normal Linux driver). This method should be much more slower since: - lots of invalidation and its path is slow. - the hit rate is low and the high price of userspace assisted address translation. - limitation of userspace IOMMU/IOTLB implementation (qemu's vtd emulation simply empty all entries when it's full). Another method is to implement kernel IOMMU (e.g vtd). But I'm not sure vhost is the best place to do this, since vhost should be architecture independent. Maybe we'd better to do it in kvm or have a pv IOMMU implementation in vhost. Actually, i have the kernel IOMMU(virtual vtd) patch which can pass though the physical device to L2 guest on hand. But it is just a draft patch which was written several years ago. If there is real requirement for it, I can rebase it and send out it for review. Another side, if fixed mappings were used in guest, (e.g dpdk in guest). We have the possibility to have 100% hit rate with almost no invalidation, the performance penalty should be ignorable, this should be the main use case for this patch. The patch is just a prototype for discussion. Any other ideas are welcomed. Thanks -- best regards yang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
