* Re: [PATCH v2 06/32] s390: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-04 13:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Heiko Carstens, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Davidlohr Bueso, Arnd Bergmann, x86, Christian Borntraeger,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper
In-Reply-To: <1451572003-2440-7-git-send-email-mst@redhat.com>
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 <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 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.
^ permalink raw reply
* Re: [PATCH v2 34/34] xen/io: use virt_xxx barriers
From: Stefano Stabellini @ 2016-01-04 12:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, 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,
linux-kernel, David Vrabel, linuxp
In-Reply-To: <1451572003-2440-35-git-send-email-mst@redhat.com>
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 <mst@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.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
* Re: [PATCH v2 33/34] xenbus: use virt_xxx barriers
From: Stefano Stabellini @ 2016-01-04 12:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, 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,
linux-kernel, David Vrabel, linuxp
In-Reply-To: <1451572003-2440-34-git-send-email-mst@redhat.com>
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 <mst@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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
>
^ permalink raw reply
* Re: [PATCH 32/34] xen/io: use __smp_XXX barriers
From: Stefano Stabellini @ 2016-01-04 11:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, Arnd Bergmann, Stefano Stabellini, Peter Zijlstra,
Andrew Cooper, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, David Vrabel, xen-devel, Boris Ostrovsky
In-Reply-To: <1451473761-30019-33-git-send-email-mst@redhat.com>
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 <mst@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.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..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
>
^ permalink raw reply
* Re: [PATCH 31/34] xenbus: use __smp_XXX barriers
From: Stefano Stabellini @ 2016-01-04 11:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, Arnd Bergmann, Stefano Stabellini, Peter Zijlstra,
Andrew Cooper, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, David Vrabel, xen-devel, Boris Ostrovsky
In-Reply-To: <1451473761-30019-32-git-send-email-mst@redhat.com>
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 <mst@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 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
>
^ permalink raw reply
* [PATCH 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-04 11:37 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,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1451907395-15978-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 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
^ permalink raw reply related
* [PATCH 2/3] checkpatch: check for __smp outside barrier.h
From: Michael S. Tsirkin @ 2016-01-04 11:37 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,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1451907395-15978-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 | 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
^ permalink raw reply related
* [PATCH 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-04 11:36 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,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1451907395-15978-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 | 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
^ permalink raw reply related
* [PATCH 0/3] checkpatch: handling of memory barriers
From: Michael S. Tsirkin @ 2016-01-04 11:36 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,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
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
^ permalink raw reply
* Re: [Xen-devel] [PATCH v2 34/34] xen/io: use virt_xxx barriers
From: David Vrabel @ 2016-01-04 11:32 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, 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, David Vrabel, linuxppc-dev,
David Miller
In-Reply-To: <1451572003-2440-35-git-send-email-mst@redhat.com>
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.vrabel@citrix.com>
David
^ permalink raw reply
* Re: [Xen-devel] [PATCH v2 33/34] xenbus: use virt_xxx barriers
From: David Vrabel @ 2016-01-04 11:32 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Boris Ostrovsky, linux-arch,
linux-s390, 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, David Vrabel, linuxppc-dev,
David Miller
In-Reply-To: <1451572003-2440-34-git-send-email-mst@redhat.com>
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 <david.vrabel@citrix.com>
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
^ permalink raw reply
* Re: [PATCH 01/34] Documentation/memory-barriers.txt: document __smb_mb()
From: Michael S. Tsirkin @ 2016-01-04 11:27 UTC (permalink / raw)
To: Stefano Stabellini
Cc: linux-arch, Arnd Bergmann, Jonathan Corbet, Peter Zijlstra,
Andrew Cooper, linux-doc, linux-kernel, virtualization
In-Reply-To: <alpine.DEB.2.02.1601041104290.19710@kaball.uk.xensource.com>
On Mon, Jan 04, 2016 at 11:08:19AM +0000, Stefano Stabellini wrote:
> On Wed, 30 Dec 2015, Michael S. Tsirkin wrote:
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 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
> >
^ permalink raw reply
* Re: [PATCH 01/34] Documentation/memory-barriers.txt: document __smb_mb()
From: Stefano Stabellini @ 2016-01-04 11:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, Arnd Bergmann, Stefano Stabellini, Peter Zijlstra,
Andrew Cooper, Jonathan Corbet, linux-doc, linux-kernel,
virtualization
In-Reply-To: <1451473761-30019-2-git-send-email-mst@redhat.com>
On Wed, 30 Dec 2015, Michael S. Tsirkin wrote:
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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
>
^ permalink raw reply
* Re: [RFC v4 0/5] Add virtio transport for AF_VSOCK
From: Jason Wang @ 2016-01-04 7:55 UTC (permalink / raw)
To: Stefan Hajnoczi, kvm
Cc: Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Christoffer Dall, matt.ma
In-Reply-To: <1450775258-18287-1-git-send-email-stefanha@redhat.com>
On 12/22/2015 05:07 PM, Stefan Hajnoczi wrote:
> This series is based on v4.4-rc2 and the "virtio: make find_vqs()
> checkpatch.pl-friendly" patch I recently submitted.
>
> v4:
> * Addressed code review comments from Alex Bennee
> * MAINTAINERS file entries for new files
> * Trace events instead of pr_debug()
> * RST packet is sent when there is no listen socket
> * Allow guest->host connections again (began discussing netfilter support with
> Matt Benjamin instead of hard-coding security policy in virtio-vsock code)
> * Many checkpatch.pl cleanups (will be 100% clean in v5)
>
> v3:
> * Remove unnecessary 3-way handshake, just do REQUEST/RESPONSE instead
> of REQUEST/RESPONSE/ACK
> * Remove SOCK_DGRAM support and focus on SOCK_STREAM first
> (also drop v2 Patch 1, it's only needed for SOCK_DGRAM)
> * Only allow host->guest connections (same security model as latest
> VMware)
> * Don't put vhost vsock driver into staging
> * Add missing Kconfig dependencies (Arnd Bergmann <arnd@arndb.de>)
> * Remove unneeded variable used to store return value
> (Fengguang Wu <fengguang.wu@intel.com> and Julia Lawall
> <julia.lawall@lip6.fr>)
>
> v2:
> * Rebased onto Linux v4.4-rc2
> * vhost: Refuse to assign reserved CIDs
> * vhost: Refuse guest CID if already in use
> * vhost: Only accept correctly addressed packets (no spoofing!)
> * vhost: Support flexible rx/tx descriptor layout
> * vhost: Add missing total_tx_buf decrement
> * virtio_transport: Fix total_tx_buf accounting
> * virtio_transport: Add virtio_transport global mutex to prevent races
> * common: Notify other side of SOCK_STREAM disconnect (fixes shutdown
> semantics)
> * common: Avoid recursive mutex_lock(tx_lock) for write_space (fixes deadlock)
> * common: Define VIRTIO_VSOCK_TYPE_STREAM/DGRAM hardware interface constants
> * common: Define VIRTIO_VSOCK_SHUTDOWN_RCV/SEND hardware interface constants
> * common: Fix peer_buf_alloc inheritance on child socket
>
> This patch series adds a virtio transport for AF_VSOCK (net/vmw_vsock/).
> AF_VSOCK is designed for communication between virtual machines and
> hypervisors. It is currently only implemented for VMware's VMCI transport.
>
> This series implements the proposed virtio-vsock device specification from
> here:
> http://permalink.gmane.org/gmane.comp.emulators.virtio.devel/980
>
> Most of the work was done by Asias He and Gerd Hoffmann a while back. I have
> picked up the series again.
>
> The QEMU userspace changes are here:
> https://github.com/stefanha/qemu/commits/vsock
>
> Why virtio-vsock?
> -----------------
> Guest<->host communication is currently done over the virtio-serial device.
> This makes it hard to port sockets API-based applications and is limited to
> static ports.
>
> virtio-vsock uses the sockets API so that applications can rely on familiar
> SOCK_STREAM semantics. Applications on the host can easily connect to guest
> agents because the sockets API allows multiple connections to a listen socket
> (unlike virtio-serial). This simplifies the guest<->host communication and
> eliminates the need for extra processes on the host to arbitrate virtio-serial
> ports.
>
> Overview
> --------
> This series adds 3 pieces:
>
> 1. virtio_transport_common.ko - core virtio vsock code that uses vsock.ko
>
> 2. virtio_transport.ko - guest driver
>
> 3. drivers/vhost/vsock.ko - host driver
Have a (dumb maybe) question after a quick glance at the codes:
Is there any chance to reuse existed virtio-net/vhost-net codes? For
example, using virito-net instead of a new device as a transport in
guest and using vhost-net (especially consider it uses a socket as
backend) in host. Maybe just a new virtio-net header type for vsock. I'm
asking since I don't see any blocker for doing this.
Thanks
> Howto
> -----
> The following kernel options are needed:
> CONFIG_VSOCKETS=y
> CONFIG_VIRTIO_VSOCKETS=y
> CONFIG_VIRTIO_VSOCKETS_COMMON=y
> CONFIG_VHOST_VSOCK=m
>
> Launch QEMU as follows:
> # qemu ... -device vhost-vsock-pci,id=vhost-vsock-pci0,guest-cid=3
>
> Guest and host can communicate via AF_VSOCK sockets. The host's CID (address)
> is 2 and the guest must be assigned a CID (3 in the example above).
>
> Status
> ------
> This patch series implements the latest draft specification. Please review.
>
> Asias He (4):
> VSOCK: Introduce virtio_vsock_common.ko
> VSOCK: Introduce virtio_transport.ko
> VSOCK: Introduce vhost_vsock.ko
> VSOCK: Add Makefile and Kconfig
>
> Stefan Hajnoczi (1):
> VSOCK: transport-specific vsock_transport functions
>
> MAINTAINERS | 13 +
> drivers/vhost/Kconfig | 15 +
> drivers/vhost/Makefile | 4 +
> drivers/vhost/vsock.c | 607 +++++++++++++++
> drivers/vhost/vsock.h | 4 +
> include/linux/virtio_vsock.h | 167 +++++
> include/net/af_vsock.h | 3 +
> .../trace/events/vsock_virtio_transport_common.h | 144 ++++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_vsock.h | 87 +++
> net/vmw_vsock/Kconfig | 19 +
> net/vmw_vsock/Makefile | 2 +
> net/vmw_vsock/af_vsock.c | 9 +
> net/vmw_vsock/virtio_transport.c | 481 ++++++++++++
> net/vmw_vsock/virtio_transport_common.c | 834 +++++++++++++++++++++
> 15 files changed, 2390 insertions(+)
> create mode 100644 drivers/vhost/vsock.c
> create mode 100644 drivers/vhost/vsock.h
> create mode 100644 include/linux/virtio_vsock.h
> create mode 100644 include/trace/events/vsock_virtio_transport_common.h
> create mode 100644 include/uapi/linux/virtio_vsock.h
> create mode 100644 net/vmw_vsock/virtio_transport.c
> create mode 100644 net/vmw_vsock/virtio_transport_common.c
>
^ permalink raw reply
* Re: [PATCH RFC] vhost: basic device IOTLB support
From: Jason Wang @ 2016-01-04 6:22 UTC (permalink / raw)
To: Yang Zhang, mst, kvm, virtualization, netdev, linux-kernel,
linux-api
In-Reply-To: <5689CD59.6030900@gmail.com>
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.
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
^ permalink raw reply
* Re: [PATCH RFC] vhost: basic device IOTLB support
From: Jason Wang @ 2016-01-04 6:08 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20151231130835-mutt-send-email-mst@redhat.com>
On 12/31/2015 07:17 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 31, 2015 at 03:13:45PM +0800, 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.
>>
>> For simplicity, IOTLB was implemented with a simple hash array. The
>> index were calculated from IOVA page frame number which can only works
>> at PAGE_SIZE level.
>>
>> An qemu implementation (for reference) is available at:
>> git@github.com:jasowang/qemu.git iommu
>>
>> TODO & Known issues:
>>
>> - read/write permission validation was not implemented.
>> - no feature negotiation.
>> - VHOST_SET_MEM_TABLE is not reused (maybe there's a chance).
>> - working at PAGE_SIZE level, don't support large mappings.
>> - better data structure for IOTLB instead of simple hash array.
>> - better API, e.g using mmap() instead of preset userspace address.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Interesting. I'm working on a slightly different approach
> which is direct vt-d support in vhost.
I've considered this approach. May have advantages but the issues here
is vt-d emulation is even in-complete in qemu and I believe we don't
want to duplicate the code in both vhost-kernel and vhost-user?
> This one has the advantage of being more portable.
Right, the patch tries to be architecture independent.
>
>> ---
>> drivers/vhost/net.c | 2 +-
>> drivers/vhost/vhost.c | 190 ++++++++++++++++++++++++++++++++++++++++++++-
>> drivers/vhost/vhost.h | 13 ++++
>> include/uapi/linux/vhost.h | 26 +++++++
>> 4 files changed, 229 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 9eda69e..a172be9 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -1083,7 +1083,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
>> r = vhost_dev_ioctl(&n->dev, ioctl, argp);
>> if (r == -ENOIOCTLCMD)
>> r = vhost_vring_ioctl(&n->dev, ioctl, argp);
>> - else
>> + else if (ioctl != VHOST_UPDATE_IOTLB)
>> vhost_net_flush(n);
>> mutex_unlock(&n->dev.mutex);
>> return r;
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index eec2f11..729fe05 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -113,6 +113,11 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
>> }
>> #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
>>
>> +static inline int vhost_iotlb_hash(u64 iova)
>> +{
>> + return (iova >> PAGE_SHIFT) & (VHOST_IOTLB_SIZE - 1);
>> +}
>> +
>> static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
>> poll_table *pt)
>> {
>> @@ -384,8 +389,14 @@ void vhost_dev_init(struct vhost_dev *dev,
>> dev->memory = NULL;
>> dev->mm = NULL;
>> spin_lock_init(&dev->work_lock);
>> + spin_lock_init(&dev->iotlb_lock);
>> + mutex_init(&dev->iotlb_req_mutex);
>> INIT_LIST_HEAD(&dev->work_list);
>> dev->worker = NULL;
>> + dev->iotlb_request = NULL;
>> + dev->iotlb_ctx = NULL;
>> + dev->iotlb_file = NULL;
>> + dev->pending_request.flags.type = VHOST_IOTLB_INVALIDATE;
>>
>> for (i = 0; i < dev->nvqs; ++i) {
>> vq = dev->vqs[i];
>> @@ -393,12 +404,17 @@ void vhost_dev_init(struct vhost_dev *dev,
>> vq->indirect = NULL;
>> vq->heads = NULL;
>> vq->dev = dev;
>> + vq->iotlb_request = NULL;
>> mutex_init(&vq->mutex);
>> vhost_vq_reset(dev, vq);
>> if (vq->handle_kick)
>> vhost_poll_init(&vq->poll, vq->handle_kick,
>> POLLIN, dev);
>> }
>> +
>> + init_completion(&dev->iotlb_completion);
>> + for (i = 0; i < VHOST_IOTLB_SIZE; i++)
>> + dev->iotlb[i].flags.valid = VHOST_IOTLB_INVALID;
>> }
>> EXPORT_SYMBOL_GPL(vhost_dev_init);
>>
>> @@ -940,9 +956,10 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>> {
>> struct file *eventfp, *filep = NULL;
>> struct eventfd_ctx *ctx = NULL;
>> + struct vhost_iotlb_entry entry;
>> u64 p;
>> long r;
>> - int i, fd;
>> + int index, i, fd;
>>
>> /* If you are not the owner, you can become one */
>> if (ioctl == VHOST_SET_OWNER) {
>> @@ -1008,6 +1025,80 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>> if (filep)
>> fput(filep);
>> break;
>> + case VHOST_SET_IOTLB_FD:
>> + r = get_user(fd, (int __user *)argp);
>> + if (r < 0)
>> + break;
>> + eventfp = fd == -1 ? NULL : eventfd_fget(fd);
>> + if (IS_ERR(eventfp)) {
>> + r = PTR_ERR(eventfp);
>> + break;
>> + }
>> + if (eventfp != d->iotlb_file) {
>> + filep = d->iotlb_file;
>> + d->iotlb_file = eventfp;
>> + ctx = d->iotlb_ctx;
>> + d->iotlb_ctx = eventfp ?
>> + eventfd_ctx_fileget(eventfp) : NULL;
>> + } else
>> + filep = eventfp;
>> + for (i = 0; i < d->nvqs; ++i) {
>> + mutex_lock(&d->vqs[i]->mutex);
>> + d->vqs[i]->iotlb_ctx = d->iotlb_ctx;
>> + mutex_unlock(&d->vqs[i]->mutex);
>> + }
>> + if (ctx)
>> + eventfd_ctx_put(ctx);
>> + if (filep)
>> + fput(filep);
>> + break;
>> + case VHOST_SET_IOTLB_REQUEST_ENTRY:
>> + if (!access_ok(VERIFY_READ, argp, sizeof(*d->iotlb_request)))
>> + return -EFAULT;
>> + if (!access_ok(VERIFY_WRITE, argp, sizeof(*d->iotlb_request)))
>> + return -EFAULT;
>> + d->iotlb_request = argp;
>> + for (i = 0; i < d->nvqs; ++i) {
>> + mutex_lock(&d->vqs[i]->mutex);
>> + d->vqs[i]->iotlb_request = argp;
>> + mutex_unlock(&d->vqs[i]->mutex);
>> + }
>> + break;
>> + case VHOST_UPDATE_IOTLB:
>> + r = copy_from_user(&entry, argp, sizeof(entry));
>> + if (r < 0) {
>> + r = -EFAULT;
>> + goto done;
>> + }
>> +
>> + index = vhost_iotlb_hash(entry.iova);
>> +
>> + spin_lock(&d->iotlb_lock);
>> + switch (entry.flags.type) {
>> + case VHOST_IOTLB_UPDATE:
>> + d->iotlb[index] = entry;
>> + break;
>> + case VHOST_IOTLB_INVALIDATE:
>> + if (d->iotlb[index].iova == entry.iova)
>> + d->iotlb[index] = entry;
>> + break;
>> + default:
>> + r = -EINVAL;
>> + }
>> + spin_unlock(&d->iotlb_lock);
>> +
>> + if (!r && entry.flags.type != VHOST_IOTLB_INVALIDATE) {
>> + mutex_lock(&d->iotlb_req_mutex);
>> + if (entry.iova == d->pending_request.iova &&
>> + d->pending_request.flags.type ==
>> + VHOST_IOTLB_MISS) {
>> + d->pending_request = entry;
>> + complete(&d->iotlb_completion);
>> + }
>> + mutex_unlock(&d->iotlb_req_mutex);
>> + }
>> +
>> + break;
>> default:
>> r = -ENOIOCTLCMD;
>> break;
>> @@ -1177,9 +1268,104 @@ int vhost_init_used(struct vhost_virtqueue *vq)
>> }
>> EXPORT_SYMBOL_GPL(vhost_init_used);
>>
>> +static struct vhost_iotlb_entry vhost_iotlb_miss(struct vhost_virtqueue *vq,
>> + u64 iova)
>> +{
>> + struct completion *c = &vq->dev->iotlb_completion;
>> + struct vhost_iotlb_entry *pending = &vq->dev->pending_request;
>> + struct vhost_iotlb_entry entry = {
>> + .flags.valid = VHOST_IOTLB_INVALID,
>> + };
>> +
>> + mutex_lock(&vq->dev->iotlb_req_mutex);
>> +
>> + if (!vq->iotlb_ctx)
>> + goto err;
>> +
>> + if (!vq->dev->iotlb_request)
>> + goto err;
>> +
>> + if (pending->flags.type == VHOST_IOTLB_MISS)
>> + goto err;
>> +
>> + pending->iova = iova & PAGE_MASK;
>> + pending->flags.type = VHOST_IOTLB_MISS;
>> +
>> + if (copy_to_user(vq->dev->iotlb_request, pending,
>> + sizeof(struct vhost_iotlb_entry))) {
>> + goto err;
>> + }
>> +
>> + mutex_unlock(&vq->dev->iotlb_req_mutex);
>> +
>> + eventfd_signal(vq->iotlb_ctx, 1);
>> + wait_for_completion_interruptible(c);
> This can still be under vq lock, can it not?
> Looks like this can cause deadlocks.
Yes, looks like a solution here is completing the pending translation
request and make it fail if there's an ioctl other than IOTLB updating.
>
>> +
>> + mutex_lock(&vq->dev->iotlb_req_mutex);
>> + entry = vq->dev->pending_request;
>> + mutex_unlock(&vq->dev->iotlb_req_mutex);
>> +
>> + return entry;
>> +err:
>> + mutex_unlock(&vq->dev->iotlb_req_mutex);
>> + return entry;
>> +}
>> +
>> +static int translate_iotlb(struct vhost_virtqueue *vq, u64 iova, u32 len,
>> + struct iovec iov[], int iov_size)
>> +{
>> + struct vhost_iotlb_entry *entry;
>> + struct vhost_iotlb_entry miss;
>> + struct vhost_dev *dev = vq->dev;
>> + int ret = 0;
>> + u64 s = 0, size;
>> +
>> + spin_lock(&dev->iotlb_lock);
>> +
>> + while ((u64) len > s) {
>> + if (unlikely(ret >= iov_size)) {
>> + ret = -ENOBUFS;
>> + break;
>> + }
>> + entry = &vq->dev->iotlb[vhost_iotlb_hash(iova)];
>> + if ((entry->iova != (iova & PAGE_MASK)) ||
>> + (entry->flags.valid != VHOST_IOTLB_VALID)) {
>> +
>> + spin_unlock(&dev->iotlb_lock);
>> + miss = vhost_iotlb_miss(vq, iova);
>> + spin_lock(&dev->iotlb_lock);
>> +
>> + if (miss.flags.valid != VHOST_IOTLB_VALID ||
>> + miss.iova != (iova & PAGE_MASK)) {
>> + ret = -EFAULT;
>> + goto err;
>> + }
>> + entry = &miss;
>> + }
>> +
>> + if (entry->iova == (iova & PAGE_MASK)) {
>> + size = entry->userspace_addr + entry->size - iova;
>> + iov[ret].iov_base =
>> + (void __user *)(entry->userspace_addr +
>> + (iova & (PAGE_SIZE - 1)));
>> + iov[ret].iov_len = min((u64)len - s, size);
>> + s += size;
>> + iova += size;
>> + ret++;
>> + } else {
>> + BUG();
>> + }
>> + }
>> +
>> +err:
>> + spin_unlock(&dev->iotlb_lock);
>> + return ret;
>> +}
>> +
>> static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
>> struct iovec iov[], int iov_size)
>> {
>> +#if 0
>> const struct vhost_memory_region *reg;
>> struct vhost_memory *mem;
>> struct iovec *_iov;
>> @@ -1209,6 +1395,8 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
>> }
>>
>> return ret;
>> +#endif
>> + return translate_iotlb(vq, addr, len, iov, iov_size);
>> }
>>
>> /* Each buffer in the virtqueues is actually a chain of descriptors. This
>> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>> index d3f7674..d254efc 100644
>> --- a/drivers/vhost/vhost.h
>> +++ b/drivers/vhost/vhost.h
>> @@ -68,6 +68,8 @@ struct vhost_virtqueue {
>> struct eventfd_ctx *call_ctx;
>> struct eventfd_ctx *error_ctx;
>> struct eventfd_ctx *log_ctx;
>> + struct eventfd_ctx *iotlb_ctx;
>> + struct vhost_iotlb __user *iotlb_request;
>>
>> struct vhost_poll poll;
>>
>> @@ -116,6 +118,8 @@ struct vhost_virtqueue {
>> #endif
>> };
>>
>> +#define VHOST_IOTLB_SIZE 1024
>> +
>> struct vhost_dev {
>> struct vhost_memory *memory;
>> struct mm_struct *mm;
>
>
>> @@ -124,9 +128,18 @@ struct vhost_dev {
>> int nvqs;
>> struct file *log_file;
>> struct eventfd_ctx *log_ctx;
>> + struct file *iotlb_file;
>> + struct eventfd_ctx *iotlb_ctx;
>> + struct mutex iotlb_req_mutex;
>> + struct vhost_iotlb_entry __user *iotlb_request;
>> + struct vhost_iotlb_entry pending_request;
>> + struct completion iotlb_completion;
>> + struct vhost_iotlb_entry request;
>> spinlock_t work_lock;
>> struct list_head work_list;
>> struct task_struct *worker;
>> + spinlock_t iotlb_lock;
>> + struct vhost_iotlb_entry iotlb[VHOST_IOTLB_SIZE];
>> };
>>
>> void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
>> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>> index ab373191..400e513 100644
>> --- a/include/uapi/linux/vhost.h
>> +++ b/include/uapi/linux/vhost.h
>> @@ -63,6 +63,26 @@ struct vhost_memory {
>> struct vhost_memory_region regions[0];
>> };
>>
>> +struct vhost_iotlb_entry {
>> + __u64 iova;
>> + __u64 size;
>> + __u64 userspace_addr;
>> + struct {
>> +#define VHOST_IOTLB_PERM_READ 0x1
>> +#define VHOST_IOTLB_PERM_WRITE 0x10
>> + __u8 perm;
>> +#define VHOST_IOTLB_MISS 1
>> +#define VHOST_IOTLB_UPDATE 2
>> +#define VHOST_IOTLB_INVALIDATE 3
>> + __u8 type;
>> +#define VHOST_IOTLB_INVALID 0x1
>> +#define VHOST_IOTLB_VALID 0x2
>> + __u8 valid;
>> + __u8 u8_padding;
>> + __u32 padding;
>> + } flags;
>> +};
>> +
>> /* ioctls */
>>
>> #define VHOST_VIRTIO 0xAF
>> @@ -127,6 +147,12 @@ struct vhost_memory {
>> /* Set eventfd to signal an error */
>> #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>>
>> +/* IOTLB */
>> +/* Specify an eventfd file descriptor to signle on IOTLB miss */
>> +#define VHOST_SET_IOTLB_FD _IOW(VHOST_VIRTIO, 0x23, int)
>> +#define VHOST_UPDATE_IOTLB _IOW(VHOST_VIRTIO, 0x24, struct vhost_iotlb_entry)
>> +#define VHOST_SET_IOTLB_REQUEST_ENTRY _IOW(VHOST_VIRTIO, 0x25, struct vhost_iotlb_entry)
>> +
>> /* VHOST_NET specific defines */
>>
>> /* Attach virtio net ring to a raw socket, or tap device.
>> --
>> 2.5.0
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC] vhost: basic device IOTLB support
From: Yang Zhang @ 2016-01-04 1:39 UTC (permalink / raw)
To: Jason Wang, mst, kvm, virtualization, netdev, linux-kernel,
linux-api
In-Reply-To: <1451546025-15955-1-git-send-email-jasowang@redhat.com>
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
^ permalink raw reply
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Minchan Kim @ 2016-01-04 0:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rafael Aquini, linux-kernel, stable, virtualization, linux-mm,
Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160101102756-mutt-send-email-mst@redhat.com>
On Fri, Jan 01, 2016 at 11:36:13AM +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote:
> > In balloon_page_dequeue, pages_lock should cover the loop
> > (ie, list_for_each_entry_safe). Otherwise, the cursor page could
> > be isolated by compaction and then list_del by isolation could
> > poison the page->lru.{prev,next} so the loop finally could
> > access wrong address like this. This patch fixes the bug.
> >
> > general protection fault: 0000 [#1] SMP
> > Dumping ftrace buffer:
> > (ftrace buffer empty)
> > Modules linked in:
> > CPU: 2 PID: 82 Comm: vballoon Not tainted 4.4.0-rc5-mm1-access_bit+ #1906
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> > task: ffff8800a7ff0000 ti: ffff8800a7fec000 task.ti: ffff8800a7fec000
> > RIP: 0010:[<ffffffff8115e754>] [<ffffffff8115e754>] balloon_page_dequeue+0x54/0x130
> > RSP: 0018:ffff8800a7fefdc0 EFLAGS: 00010246
> > RAX: ffff88013fff9a70 RBX: ffffea000056fe00 RCX: 0000000000002b7d
> > RDX: ffff88013fff9a70 RSI: ffffea000056fe00 RDI: ffff88013fff9a68
> > RBP: ffff8800a7fefde8 R08: ffffea000056fda0 R09: 0000000000000000
> > R10: ffff8800a7fefd90 R11: 0000000000000001 R12: dead0000000000e0
> > R13: ffffea000056fe20 R14: ffff880138809070 R15: ffff880138809060
> > FS: 0000000000000000(0000) GS:ffff88013fc40000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > CR2: 00007f229c10e000 CR3: 00000000b8b53000 CR4: 00000000000006a0
> > Stack:
> > 0000000000000100 ffff880138809088 ffff880138809000 ffff880138809060
> > 0000000000000046 ffff8800a7fefe28 ffffffff812c86d3 ffff880138809020
> > ffff880138809000 fffffffffff91900 0000000000000100 ffff880138809060
> > Call Trace:
> > [<ffffffff812c86d3>] leak_balloon+0x93/0x1a0
> > [<ffffffff812c8bc7>] balloon+0x217/0x2a0
> > [<ffffffff8143739e>] ? __schedule+0x31e/0x8b0
> > [<ffffffff81078160>] ? abort_exclusive_wait+0xb0/0xb0
> > [<ffffffff812c89b0>] ? update_balloon_stats+0xf0/0xf0
> > [<ffffffff8105b6e9>] kthread+0xc9/0xe0
> > [<ffffffff8105b620>] ? kthread_park+0x60/0x60
> > [<ffffffff8143b4af>] ret_from_fork+0x3f/0x70
> > [<ffffffff8105b620>] ? kthread_park+0x60/0x60
> > Code: 8d 60 e0 0f 84 af 00 00 00 48 8b 43 20 a8 01 75 3b 48 89 d8 f0 0f ba 28 00 72 10 48 8b 03 f6 c4 08 75 2f 48 89 df e8 8c 83 f9 ff <49> 8b 44 24 20 4d 8d 6c 24 20 48 83 e8 20 4d 39 f5 74 7a 4c 89
> > RIP [<ffffffff8115e754>] balloon_page_dequeue+0x54/0x130
> > RSP <ffff8800a7fefdc0>
> > ---[ end trace 43cf28060d708d5f ]---
> > Kernel panic - not syncing: Fatal exception
> > Dumping ftrace buffer:
> > (ftrace buffer empty)
> > Kernel Offset: disabled
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > mm/balloon_compaction.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> > index d3116be5a00f..300117f1a08f 100644
> > --- a/mm/balloon_compaction.c
> > +++ b/mm/balloon_compaction.c
> > @@ -61,6 +61,7 @@ struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> > bool dequeued_page;
> >
> > dequeued_page = false;
> > + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> > list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> > /*
> > * Block others from accessing the 'page' while we get around
> > @@ -75,15 +76,14 @@ struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> > continue;
> > }
> > #endif
> > - spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> > balloon_page_delete(page);
> > __count_vm_event(BALLOON_DEFLATE);
> > - spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> > unlock_page(page);
> > dequeued_page = true;
> > break;
> > }
> > }
> > + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> >
> > if (!dequeued_page) {
> > /*
>
> 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.
>
> Did you test this with lockdep? You really should for
> locking changes, and I'd expect it to warn about this.
I did but I don't see any warning.
>
> 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?
>
> We really must take a page reference before touching
> the page.
>
> I think we need something like the below to fix this issue.
> Could you please try this out, and send Tested-by?
> I will repost as a proper patch if this works for you.
If I missed something, I am happy to retest and report the result
when I go to the office.
Thanks.
>
>
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index d3116be..66d69c5 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -56,12 +56,34 @@ EXPORT_SYMBOL_GPL(balloon_page_enqueue);
> */
> struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> {
> - struct page *page, *tmp;
> + struct page *page;
> unsigned long flags;
> bool dequeued_page;
> + LIST_HEAD(processed); /* protected by b_dev_info->pages_lock */
>
> dequeued_page = false;
> - list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> + /*
> + * We need to go over b_dev_info->pages and lock each page,
> + * but b_dev_info->pages_lock must nest within page lock.
> + *
> + * To make this safe, remove each page from b_dev_info->pages list
> + * under b_dev_info->pages_lock, then drop this lock. Once list is
> + * empty, re-add them also under b_dev_info->pages_lock.
> + */
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + while (!list_empty(&b_dev_info->pages)) {
> + page = list_first_entry(&b_dev_info->pages, typeof(*page), lru);
> + /* move to processed list to avoid going over it another time */
> + list_move(&page->lru, &processed);
> +
> + if (!get_page_unless_zero(page))
> + continue;
> + /*
> + * pages_lock nests within page lock,
> + * so drop it before trylock_page
> + */
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +
> /*
> * Block others from accessing the 'page' while we get around
> * establishing additional references and preparing the 'page'
> @@ -72,6 +94,7 @@ struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> if (!PagePrivate(page)) {
> /* raced with isolation */
> unlock_page(page);
> + put_page(page);
> continue;
> }
> #endif
> @@ -80,11 +103,18 @@ struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> __count_vm_event(BALLOON_DEFLATE);
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> unlock_page(page);
> + put_page(page);
> dequeued_page = true;
> break;
> }
> + put_page(page);
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> }
>
> + /* re-add remaining entries */
> + list_splice(&processed, &b_dev_info->pages);
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +
> if (!dequeued_page) {
> /*
> * If we are unable to dequeue a balloon page because the page
--
Kind regards,
Minchan Kim
^ permalink raw reply
* Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread
From: Tejun Heo @ 2016-01-03 13:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Petr Mladek, Jiri Kosina, linux-kernel, Petr Mladek,
virtualization, Jeff Epler
In-Reply-To: <20160102213603.GA1703@redhat.com>
Hello, Michael.
On Sat, Jan 02, 2016 at 11:36:03PM +0200, Michael S. Tsirkin wrote:
> > Why so? As long as the maximum concurrently used workers are not
> > high, 1/5 second or even a lot longer sleeps are completely fine.
>
> I always thought the right way to defer executing a work queue item
> is to queue delayed work, not sleep + queue work.
That works too and is preferable if there are gonna be a lot of work
items sleeping but it isn't different from any other blocking.
> Doing a sleep ties up one thread for 1/5 of a second, does it not?
It does.
> If so, as long as it's the only driver doing this, we'll be fine,
> but if many others copy this pattern, things will
> start to break, will they not?
The maximum concurrency on the system_wq is 256 which is pretty high,
so for most use cases, it's fine. If high concurrency is expected,
it's better to break it out to a separate workqueue.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 17/32] arm: define __smp_xxx
From: Michael S. Tsirkin @ 2016-01-03 9:12 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Arnd Bergmann, x86, Tony Lindgren, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
Andrey Konovalov, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, linux-kernel,
linuxppc-dev
In-Reply-To: <20160102112438.GU8644@n2100.arm.linux.org.uk>
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 31, 2015 at 09:07:59PM +0200, Michael S. Tsirkin wrote:
> > This defines __smp_xxx barriers for arm,
> > for use by virtualization.
> >
> > smp_xxx barriers are removed as they are
> > defined correctly by asm-generic/barriers.h
> >
> > This reduces the amount of arch-specific boiler-plate code.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> In combination with patch 14, this looks like it should result in no
> change to the resulting code.
>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
>
> 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.
> --
> 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.
^ permalink raw reply
* Re: [PATCH v2 32/32] virtio_ring: use virt_store_mb
From: Michael S. Tsirkin @ 2016-01-03 9:01 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390, 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, linux-kernel,
linuxppc-dev, David Miller
In-Reply-To: <5686B622.6070600@cogentembedded.com>
On Fri, Jan 01, 2016 at 08:23:46PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 12/31/2015 10:09 PM, Michael S. Tsirkin wrote:
>
> >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 | 12 ++++++++++++
> > drivers/virtio/virtio_ring.c | 15 +++++++++------
> > 2 files changed, 21 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> >index f3fa55b..3a74d91 100644
> >--- a/include/linux/virtio_ring.h
> >+++ b/include/linux/virtio_ring.h
> >@@ -45,6 +45,18 @@ 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
> >+ {
>
> The kernel coding style dictates:
>
> if (weak_barriers) {
> virt_store_mb(*p, v);
> } else {
>
> >+ WRITE_ONCE(*p, v);
> >+ mb();
> >+ }
> >+}
> >+
> [...]
>
> MBR, Sergei
Will fix, thanks!
^ permalink raw reply
* Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread
From: Michael S. Tsirkin @ 2016-01-02 21:36 UTC (permalink / raw)
To: Tejun Heo
Cc: Petr Mladek, Jiri Kosina, linux-kernel, Petr Mladek,
virtualization, Jeff Epler
In-Reply-To: <20160102114316.GC3660@htj.duckdns.org>
On Sat, Jan 02, 2016 at 06:43:16AM -0500, Tejun Heo wrote:
> Hello,
>
> On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > > suggested using a system one. Tejun Heo confirmed that the system
> > > workqueue has a pretty high concurrency level (256) by default.
> > > Therefore we need not be afraid of too long blocking.
> >
> > Right but fill has a 1/5 second sleep on failure - *that*
> > is problematic for a system queue.
>
> Why so? As long as the maximum concurrently used workers are not
> high, 1/5 second or even a lot longer sleeps are completely fine.
I always thought the right way to defer executing a work queue item
is to queue delayed work, not sleep + queue work.
Doing a sleep ties up one thread for 1/5 of a second, does it not?
If so, as long as it's the only driver doing this, we'll be fine,
but if many others copy this pattern, things will
start to break, will they not?
> > > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> > > struct virtio_balloon *vb = vdev->priv;
> > >
> > > unregister_oom_notifier(&vb->nb);
> > > - kthread_stop(vb->thread);
> > > + cancel_work_sync(&vb->wq_work);
> >
> > OK but since job requeues itself, cancelling like this might not be enough.
>
> As long as there's no further external queueing, cancel_work_sync() is
> guaranteed to kill a self-requeueing work item.
>
> Thanks.
I didn't realise this. Thanks!
Unfortunately in this case, there can be further requeueing
if a stats request arrives.
> --
> tejun
^ permalink raw reply
* Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread
From: Tejun Heo @ 2016-01-02 11:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Petr Mladek, Jiri Kosina, linux-kernel, Petr Mladek,
virtualization, Jeff Epler
In-Reply-To: <20160101121432-mutt-send-email-mst@redhat.com>
Hello,
On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > suggested using a system one. Tejun Heo confirmed that the system
> > workqueue has a pretty high concurrency level (256) by default.
> > Therefore we need not be afraid of too long blocking.
>
> Right but fill has a 1/5 second sleep on failure - *that*
> is problematic for a system queue.
Why so? As long as the maximum concurrently used workers are not
high, 1/5 second or even a lot longer sleeps are completely fine.
> > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> > struct virtio_balloon *vb = vdev->priv;
> >
> > unregister_oom_notifier(&vb->nb);
> > - kthread_stop(vb->thread);
> > + cancel_work_sync(&vb->wq_work);
>
> OK but since job requeues itself, cancelling like this might not be enough.
As long as there's no further external queueing, cancel_work_sync() is
guaranteed to kill a self-requeueing work item.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 17/32] arm: define __smp_xxx
From: Russell King - ARM Linux @ 2016-01-02 11:24 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,
Arnd Bergmann, x86, Tony Lindgren, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
Andrey Konovalov, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, linux-kernel,
linuxppc-dev
In-Reply-To: <1451572003-2440-18-git-send-email-mst@redhat.com>
On Thu, Dec 31, 2015 at 09:07:59PM +0200, Michael S. Tsirkin wrote:
> This defines __smp_xxx barriers for arm,
> for use by virtualization.
>
> smp_xxx barriers are removed as they are
> defined correctly by asm-generic/barriers.h
>
> This reduces the amount of arch-specific boiler-plate code.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
In combination with patch 14, this looks like it should result in no
change to the resulting code.
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
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?
--
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.
^ permalink raw reply
* Re: [PATCH v2 08/32] arm: reuse asm-generic/barrier.h
From: Russell King - ARM Linux @ 2016-01-02 11:20 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,
Arnd Bergmann, x86, Tony Lindgren, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
Andrey Konovalov, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, linux-kernel,
linuxppc-dev
In-Reply-To: <1451572003-2440-9-git-send-email-mst@redhat.com>
On Thu, Dec 31, 2015 at 09:06:46PM +0200, Michael S. Tsirkin wrote:
> On arm smp_store_mb, read_barrier_depends, smp_read_barrier_depends,
> smp_store_release, smp_load_acquire, 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 <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Thanks, the asm-generic versions looks identical to me, so this should
result in no code generation difference.
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
--
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.
^ permalink raw reply
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;
as well as URLs for NNTP newsgroup(s).