* [PATCH v3 09/41] arm64: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, Catalin Marinas,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
Arnd Bergmann, x86, xen-devel, Ingo Molnar, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, Andre Przywara,
adi-buildroot-devel, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
On arm64 nop, 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/arm64/include/asm/barrier.h | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
index 9622eb4..91a43f4 100644
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -91,14 +91,7 @@ do { \
__u.__val; \
})
-#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 nop() asm volatile("nop");
-
-#define smp_mb__before_atomic() smp_mb()
-#define smp_mb__after_atomic() smp_mb()
+#include <asm-generic/barrier.h>
#endif /* __ASSEMBLY__ */
--
MST
^ permalink raw reply related
* [PATCH v3 08/41] arm: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 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, xen-devel,
Ingo Molnar, linux-xtensa, Richard Woodruff,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
Russell King, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper, Jo
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
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>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
arch/arm/include/asm/barrier.h | 23 +----------------------
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
index 3ff5642..31152e8 100644
--- a/arch/arm/include/asm/barrier.h
+++ b/arch/arm/include/asm/barrier.h
@@ -70,28 +70,7 @@ extern void arm_heavy_mb(void);
#define smp_wmb() dmb(ishst)
#endif
-#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 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_mb__before_atomic() smp_mb()
-#define smp_mb__after_atomic() smp_mb()
+#include <asm-generic/barrier.h>
#endif /* !__ASSEMBLY__ */
#endif /* __ASM_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 07/41] sparc: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 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, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, Joe Perches,
linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
On sparc 64 bit dma_rmb, dma_wmb, smp_store_mb, smp_mb, smp_rmb,
smp_wmb, read_barrier_depends and smp_read_barrier_depends match the
asm-generic variants exactly. Drop the local definitions and pull in
asm-generic/barrier.h instead.
nop uses __asm__ __volatile but is otherwise identical to
the generic version, drop that as well.
This is in preparation to refactoring this code area.
Note: nop() was in processor.h and not in barrier.h as on other
architectures. Nothing seems to depend on it being there though.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/include/asm/barrier_32.h | 1 -
arch/sparc/include/asm/barrier_64.h | 21 ++-------------------
arch/sparc/include/asm/processor.h | 3 ---
3 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/arch/sparc/include/asm/barrier_32.h b/arch/sparc/include/asm/barrier_32.h
index ae69eda..8059130 100644
--- a/arch/sparc/include/asm/barrier_32.h
+++ b/arch/sparc/include/asm/barrier_32.h
@@ -1,7 +1,6 @@
#ifndef __SPARC_BARRIER_H
#define __SPARC_BARRIER_H
-#include <asm/processor.h> /* for nop() */
#include <asm-generic/barrier.h>
#endif /* !(__SPARC_BARRIER_H) */
diff --git a/arch/sparc/include/asm/barrier_64.h b/arch/sparc/include/asm/barrier_64.h
index 14a9286..26c3f72 100644
--- a/arch/sparc/include/asm/barrier_64.h
+++ b/arch/sparc/include/asm/barrier_64.h
@@ -37,25 +37,6 @@ do { __asm__ __volatile__("ba,pt %%xcc, 1f\n\t" \
#define rmb() __asm__ __volatile__("":::"memory")
#define wmb() __asm__ __volatile__("":::"memory")
-#define dma_rmb() rmb()
-#define dma_wmb() wmb()
-
-#define smp_store_mb(__var, __value) \
- do { WRITE_ONCE(__var, __value); membar_safe("#StoreLoad"); } while(0)
-
-#ifdef CONFIG_SMP
-#define smp_mb() mb()
-#define smp_rmb() rmb()
-#define smp_wmb() wmb()
-#else
-#define smp_mb() __asm__ __volatile__("":::"memory")
-#define smp_rmb() __asm__ __volatile__("":::"memory")
-#define smp_wmb() __asm__ __volatile__("":::"memory")
-#endif
-
-#define read_barrier_depends() do { } while (0)
-#define smp_read_barrier_depends() do { } while (0)
-
#define smp_store_release(p, v) \
do { \
compiletime_assert_atomic_type(*p); \
@@ -74,4 +55,6 @@ do { \
#define smp_mb__before_atomic() barrier()
#define smp_mb__after_atomic() barrier()
+#include <asm-generic/barrier.h>
+
#endif /* !(__SPARC64_BARRIER_H) */
diff --git a/arch/sparc/include/asm/processor.h b/arch/sparc/include/asm/processor.h
index 2fe99e6..9da9646 100644
--- a/arch/sparc/include/asm/processor.h
+++ b/arch/sparc/include/asm/processor.h
@@ -5,7 +5,4 @@
#else
#include <asm/processor_32.h>
#endif
-
-#define nop() __asm__ __volatile__ ("nop")
-
#endif
--
MST
^ permalink raw reply related
* [PATCH v3 06/41] s390: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, Heiko Carstens,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, x86, Christian Borntraeger, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
adi-buildroot-devel, Martin Schwidefsky, Thomas Gleixner,
linux-metag
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
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()
-
-#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); \
@@ -53,4 +45,6 @@ do { \
___p1; \
})
+#include <asm-generic/barrier.h>
+
#endif /* __ASM_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 05/41] powerpc: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, virtualization, Paul Mackerras,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Davidlohr Bueso, Russell King - ARM Linux, Arnd Bergmann,
Michael Ellerman, x86, xen-devel, Ingo Molnar, Paul E. McKenney,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
adi-buildroot-devel, Thomas Gleixner
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
On powerpc 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/powerpc/include/asm/barrier.h | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index a7af5fb..980ad0c 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -34,8 +34,6 @@
#define rmb() __asm__ __volatile__ ("sync" : : : "memory")
#define wmb() __asm__ __volatile__ ("sync" : : : "memory")
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
-
#ifdef __SUBARCH_HAS_LWSYNC
# define SMPWMB LWSYNC
#else
@@ -60,9 +58,6 @@
#define smp_wmb() barrier()
#endif /* CONFIG_SMP */
-#define read_barrier_depends() do { } while (0)
-#define smp_read_barrier_depends() do { } while (0)
-
/*
* This is a barrier which prevents following instructions from being
* started until the value of the argument x is known. For example, if
@@ -87,8 +82,8 @@ do { \
___p1; \
})
-#define smp_mb__before_atomic() smp_mb()
-#define smp_mb__after_atomic() smp_mb()
#define smp_mb__before_spinlock() smp_mb()
+#include <asm-generic/barrier.h>
+
#endif /* _ASM_POWERPC_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 04/41] ia64: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-10 14:17 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,
Davidlohr Bueso, Russell King - ARM Linux, Arnd Bergmann, x86,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Tony Luck, Andrew Cooper,
Fenghua Yu <fengh>
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
On ia64 smp_rmb, smp_wmb, read_barrier_depends, smp_read_barrier_depends
and smp_store_mb() 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: Tony Luck <tony.luck@intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/include/asm/barrier.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
index 209c4b8..2f93348 100644
--- a/arch/ia64/include/asm/barrier.h
+++ b/arch/ia64/include/asm/barrier.h
@@ -48,12 +48,6 @@
# define smp_mb() barrier()
#endif
-#define smp_rmb() smp_mb()
-#define smp_wmb() smp_mb()
-
-#define read_barrier_depends() do { } while (0)
-#define smp_read_barrier_depends() do { } while (0)
-
#define smp_mb__before_atomic() barrier()
#define smp_mb__after_atomic() barrier()
@@ -77,12 +71,12 @@ do { \
___p1; \
})
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
-
/*
* The group barrier in front of the rsm & ssm are necessary to ensure
* that none of the previous instructions in the same group are
* affected by the rsm/ssm.
*/
+#include <asm-generic/barrier.h>
+
#endif /* _ASM_IA64_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 03/41] ia64: rename nop->iosapic_nop
From: Michael S. Tsirkin @ 2016-01-10 14:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Tony Luck, Andrew Cooper,
Fenghua Yu, Jiang Liu, Joe
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
asm-generic/barrier.h defines a nop() macro.
To be able to use this header on ia64, we shouldn't
call local functions/variables nop().
There's one instance where this breaks on ia64:
rename the function to iosapic_nop to avoid the conflict.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Tony Luck <tony.luck@intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/kernel/iosapic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/ia64/kernel/iosapic.c b/arch/ia64/kernel/iosapic.c
index d2fae05..90fde5b 100644
--- a/arch/ia64/kernel/iosapic.c
+++ b/arch/ia64/kernel/iosapic.c
@@ -256,7 +256,7 @@ set_rte (unsigned int gsi, unsigned int irq, unsigned int dest, int mask)
}
static void
-nop (struct irq_data *data)
+iosapic_nop (struct irq_data *data)
{
/* do nothing... */
}
@@ -415,7 +415,7 @@ iosapic_unmask_level_irq (struct irq_data *data)
#define iosapic_shutdown_level_irq mask_irq
#define iosapic_enable_level_irq unmask_irq
#define iosapic_disable_level_irq mask_irq
-#define iosapic_ack_level_irq nop
+#define iosapic_ack_level_irq iosapic_nop
static struct irq_chip irq_type_iosapic_level = {
.name = "IO-SAPIC-level",
@@ -453,7 +453,7 @@ iosapic_ack_edge_irq (struct irq_data *data)
}
#define iosapic_enable_edge_irq unmask_irq
-#define iosapic_disable_edge_irq nop
+#define iosapic_disable_edge_irq iosapic_nop
static struct irq_chip irq_type_iosapic_edge = {
.name = "IO-SAPIC-edge",
--
MST
^ permalink raw reply related
* [PATCH v3 02/41] asm-generic: guard smp_store_release/load_acquire
From: Michael S. Tsirkin @ 2016-01-10 14:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, Joe Perches,
linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
Allow architectures to override smp_store_release
and smp_load_acquire by guarding the defines
in asm-generic/barrier.h with ifndef directives.
This is in preparation to reusing asm-generic/barrier.h
on architectures which have their own definition
of these macros.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
include/asm-generic/barrier.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index 0f45f93..987b2e0 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -104,13 +104,16 @@
#define smp_mb__after_atomic() smp_mb()
#endif
+#ifndef smp_store_release
#define smp_store_release(p, v) \
do { \
compiletime_assert_atomic_type(*p); \
smp_mb(); \
WRITE_ONCE(*p, v); \
} while (0)
+#endif
+#ifndef smp_load_acquire
#define smp_load_acquire(p) \
({ \
typeof(*p) ___p1 = READ_ONCE(*p); \
@@ -118,6 +121,7 @@ do { \
smp_mb(); \
___p1; \
})
+#endif
#endif /* !__ASSEMBLY__ */
#endif /* __ASM_GENERIC_BARRIER_H */
--
MST
^ permalink raw reply related
* [PATCH v3 01/41] lcoking/barriers, arch: Use smp barriers in smp_store_release()
From: Michael S. Tsirkin @ 2016-01-10 14:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, Heiko Carstens, virtualization,
Paul Mackerras, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, Davidlohr Bueso, Michael Ellerman, x86,
Christian Borntraeger, Linus Torvalds, xen-devel, Ingo Molnar,
Paul E . McKenney, linux-xtensa
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
From: Davidlohr Bueso <dave@stgolabs.net>
With commit b92b8b35a2e ("locking/arch: Rename set_mb() to smp_store_mb()")
it was made clear that the context of this call (and thus set_mb)
is strictly for CPU ordering, as opposed to IO. As such all archs
should use the smp variant of mb(), respecting the semantics and
saving a mandatory barrier on UP.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <linux-arch@vger.kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: dave@stgolabs.net
Link: http://lkml.kernel.org/r/1445975631-17047-3-git-send-email-dave@stgolabs.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/ia64/include/asm/barrier.h | 2 +-
arch/powerpc/include/asm/barrier.h | 2 +-
arch/s390/include/asm/barrier.h | 2 +-
include/asm-generic/barrier.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
index df896a1..209c4b8 100644
--- a/arch/ia64/include/asm/barrier.h
+++ b/arch/ia64/include/asm/barrier.h
@@ -77,7 +77,7 @@ do { \
___p1; \
})
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
/*
* The group barrier in front of the rsm & ssm are necessary to ensure
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index 0eca6ef..a7af5fb 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -34,7 +34,7 @@
#define rmb() __asm__ __volatile__ ("sync" : : : "memory")
#define wmb() __asm__ __volatile__ ("sync" : : : "memory")
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
#ifdef __SUBARCH_HAS_LWSYNC
# define SMPWMB LWSYNC
diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
index d68e11e..7ffd0b1 100644
--- a/arch/s390/include/asm/barrier.h
+++ b/arch/s390/include/asm/barrier.h
@@ -36,7 +36,7 @@
#define smp_mb__before_atomic() smp_mb()
#define smp_mb__after_atomic() smp_mb()
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
#define smp_store_release(p, v) \
do { \
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index b42afad..0f45f93 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -93,7 +93,7 @@
#endif /* CONFIG_SMP */
#ifndef smp_store_mb
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
+#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
#endif
#ifndef smp_mb__before_atomic
--
MST
^ permalink raw reply related
* [PATCH v3 00/41] arch: barrier cleanup + barriers for virt
From: Michael S. Tsirkin @ 2016-01-10 14:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, Joe Perches,
linuxppc-dev, David Miller
Changes since v2:
- extended checkpatch tests for barriers, and added patches
teaching it to warn about incorrect usage of barriers
(__smp_xxx barriers are for use by asm-generic code only),
should help prevent misuse by arch code
to address comments by Russell King
- patched more instances of xen to use virt_ barriers
as suggested by Stefano Stabellini
- implemented a 2 byte xchg on sh instead of hacking around it
as suggested by Peter Zijlstra and Rich Felker
- added a patch to drop some s390 arch-specific smp_xxx barriers - generic
versions are more efficient
as suggested by Peter Zijlstra and Martin Schwidefsky
- added a patch to replace before/after atomic barriers with barrier()
on s390 as suggested by Peter Zijlstra and Martin Schwidefsky
- included acks from multiple arch maintainers
thanks a lot for the review!
Changes since v1:
- replaced an asm-generic patch with an equivalent patch already in tip
- add wrappers with virt_ prefix for better code annotation,
as suggested by David Miller
- dropped XXX in patch names as this makes vger choke, Cc all relevant
mailing lists on all patches (not personal email, as the list becomes
too long then)
I parked this in vhost tree for now, though the inclusion of patch 1 from tip
creates a merge conflict - but one that is trivial to resolve.
So I intend to just merge it all through my tree, including the
duplicate patch, and assume conflict will be resolved.
I would really appreciate some feedback on arch bits (especially the x86 bits),
and acks for merging this through the vhost tree.
Thanks!
What really started me off is trying to cleanup some virt code, as suggested by
Peter, who said
> You could of course go fix that instead of mutilating things into
> sort-of functional state.
This work is needed for virtio, so it's probably easiest to
merge it through my tree - is this fine by everyone?
Note to arch maintainers: please don't cherry-pick patches out of this patchset
as it's been structured in this order to avoid breaking bisect.
Please send acks instead!
=====
Sometimes, virtualization is weird. For example, virtio does this (conceptually):
#ifdef CONFIG_SMP
smp_mb();
#else
mb();
#endif
Similarly, Xen calls mb() when it's not doing any MMIO at all.
Of course it's wrong in the sense that it's suboptimal. What we would really
like is to have, on UP, exactly the same barrier as on SMP. This is because a
UP guest can run on an SMP host.
But Linux doesn't provide this ability: if CONFIG_SMP is not defined is
optimizes most barriers out to a compiler barrier.
Consider for example x86: what we want is xchg (NOT mfence - there's no real IO
going on here - just switching out of the VM - more like a function call
really) but if built without CONFIG_SMP smp_store_mb does not include this.
Virt in general is probably the only use-case, because this really is an
artifact of interfacing with an SMP host while running an UP kernel,
but since we have (at least) two users, it seems to make sense to
put these APIs in a central place.
In fact, smp_ barriers are stubs on !SMP, so they can be defined as follows:
arch/XXX/include/asm/barrier.h:
#define __smp_mb() DOSOMETHING
include/asm-generic/barrier.h:
#ifdef CONFIG_SMP
#define smp_mb() __smp_mb()
#else
#define smp_mb() barrier()
#endif
This has the benefit of cleaning out a bunch of duplicated
ifdefs on a bunch of architectures - this patchset brings
about a net reduction in LOC, more than compensated for
later by performance enhancements, extra documentation and tools :)
Then virt can use __smp_XXX when talking to an SMP host.
To make those users explicit, this patchset adds virt_xxx wrappers
for them.
Touching all archs is a tad tedious, but its fairly straight forward.
The patchset is structured as follows:
-. Patch 1 fixes a bug in asm-generic.
It is already in tip, included here for completeness.
-. Patches 2-12 make sure barrier.h on all remaining
architectures includes asm-generic/barrier.h:
after the change in Patch 1, code there matches
asm-generic/barrier.h almost verbatim.
Minor code tweaks were required in a couple of places.
Macros duplicated from asm-generic/barrier.h are dropped
in the process.
After all that preparatory work, we are getting to the actual change.
-. Patch 13 adds generic smp_XXX wrappers in asm-generic:
these select __smp_XXX or barrier() depending on CONFIG_SMP
-. Patches 14-27 change all architectures to
define __smp_XXX macros; the generic code in asm-generic/barrier.h
then defines smp_XXX macros
I compiled the affected arches before and after the changes,
dumped the .text section (using objdump -O binary) and
made sure that the object code is exactly identical
before and after the change.
Note: the changes were intentionally done in a way
that avoids generated code changes.
When I got feedback from arch maintainers that the
barriers can be improved, I made this in a separate
patch on top, to allow this testing by binary comparisons.
Unfortunately, I don't have a metag cross-build toolset ready.
Hoping for some acks on this architecture.
Next, the following patches put the __smp_xxx APIs to work for virt:
-. Patch 28 adds virt_ wrappers for __smp_, and documents them.
After all this work, this requires very few lines of code in
the generic header.
-. Patches 29,30 convert virtio drivers to use the virt_xxx APIs
tested on x86
-. Patches 31-33 teach virtio to use virt_store_mb
sh architecture was missing a 2-byte xchg,
needed for 2 byte smp_store_mb,
so I had to add this support for sh
-. Patches 34-36 teach checkpatch to warn about
misuse of the new barriers
-. Patches 37-39 convert xen drivers to use the virt_xxx APIs
compiled only (by intel 0-day infrastructure)
-. Patch 40 makes some smp barriers on s390 more efficient
included here to avoid merge conflicts, at maintainer's request
tested on x86
Davidlohr Bueso (1):
lcoking/barriers, arch: Use smp barriers in smp_store_release()
Michael S. Tsirkin (40):
asm-generic: guard smp_store_release/load_acquire
ia64: rename nop->iosapic_nop
ia64: reuse asm-generic/barrier.h
powerpc: reuse asm-generic/barrier.h
s390: reuse asm-generic/barrier.h
sparc: reuse asm-generic/barrier.h
arm: reuse asm-generic/barrier.h
arm64: reuse asm-generic/barrier.h
metag: reuse asm-generic/barrier.h
mips: reuse asm-generic/barrier.h
x86/um: reuse asm-generic/barrier.h
x86: reuse asm-generic/barrier.h
asm-generic: add __smp_xxx wrappers
powerpc: define __smp_xxx
arm64: define __smp_xxx
arm: define __smp_xxx
blackfin: define __smp_xxx
ia64: define __smp_xxx
metag: define __smp_xxx
mips: define __smp_xxx
s390: define __smp_xxx
sh: define __smp_xxx, fix smp_store_mb for !SMP
sparc: define __smp_xxx
tile: define __smp_xxx
xtensa: define __smp_xxx
x86: define __smp_xxx
asm-generic: implement virt_xxx memory barriers
Revert "virtio_ring: Update weak barriers to use dma_wmb/rmb"
virtio_ring: update weak barriers to use virt_xxx
sh: support 1 and 2 byte xchg
sh: move xchg_cmpxchg to a header by itself
virtio_ring: use virt_store_mb
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
xenbus: use virt_xxx barriers
xen/io: use virt_xxx barriers
xen/events: use virt_xxx barriers
s390: use generic memory barriers
s390: more efficient smp barriers
arch/arm/include/asm/barrier.h | 35 ++----------
arch/arm64/include/asm/barrier.h | 19 ++-----
arch/blackfin/include/asm/barrier.h | 4 +-
arch/ia64/include/asm/barrier.h | 24 +++-----
arch/metag/include/asm/barrier.h | 55 ++++++-------------
arch/mips/include/asm/barrier.h | 51 ++++++-----------
arch/powerpc/include/asm/barrier.h | 33 ++++-------
arch/s390/include/asm/barrier.h | 23 ++++----
arch/sh/include/asm/barrier.h | 3 +-
arch/sh/include/asm/cmpxchg-grb.h | 22 ++++++++
arch/sh/include/asm/cmpxchg-irq.h | 11 ++++
arch/sh/include/asm/cmpxchg-llsc.h | 25 +--------
arch/sh/include/asm/cmpxchg-xchg.h | 51 +++++++++++++++++
arch/sh/include/asm/cmpxchg.h | 3 +
arch/sparc/include/asm/barrier_32.h | 1 -
arch/sparc/include/asm/barrier_64.h | 29 ++--------
arch/sparc/include/asm/processor.h | 3 -
arch/tile/include/asm/barrier.h | 9 +--
arch/x86/include/asm/barrier.h | 36 +++++-------
arch/x86/um/asm/barrier.h | 9 +--
arch/xtensa/include/asm/barrier.h | 4 +-
include/asm-generic/barrier.h | 106 +++++++++++++++++++++++++++++++++---
include/linux/virtio_ring.h | 21 +++++--
include/xen/interface/io/ring.h | 16 +++---
arch/ia64/kernel/iosapic.c | 6 +-
drivers/virtio/virtio_ring.c | 15 +++--
drivers/xen/events/events_fifo.c | 3 +-
drivers/xen/xenbus/xenbus_comms.c | 8 +--
Documentation/memory-barriers.txt | 28 ++++++++--
scripts/checkpatch.pl | 31 ++++++++++-
30 files changed, 382 insertions(+), 302 deletions(-)
create mode 100644 arch/sh/include/asm/cmpxchg-xchg.h
--
MST
^ permalink raw reply
* [PATCH v2 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-10 11:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452427000-4520-1-git-send-email-mst@redhat.com>
Add virt_ barriers to list of barriers to check for
presence of a comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a96adcb..5ca272b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5131,7 +5131,8 @@ sub process {
}x;
my $all_barriers = qr{
$barriers|
- smp_($smp_barriers)
+ smp_($smp_barriers)|
+ virt_($smp_barriers)
}x;
if ($line =~ /\b($all_barriers)\s*\(/) {
--
MST
^ permalink raw reply related
* [PATCH v2 2/3] checkpatch: check for __smp outside barrier.h
From: Michael S. Tsirkin @ 2016-01-10 11:57 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452427000-4520-1-git-send-email-mst@redhat.com>
Introduction of __smp barriers cleans up a bunch of duplicate code, but
it gives people an additional handle onto a "new" set of barriers - just
because they're prefixed with __* unfortunately doesn't stop anyone from
using it (as happened with other arch stuff before.)
Add a checkpatch test so it will trigger a warning.
Reported-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 97b8b62..a96adcb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5141,6 +5141,16 @@ sub process {
}
}
+ my $underscore_smp_barriers = qr{__smp_($smp_barriers)}x;
+
+ if ($realfile !~ m@^include/asm-generic/@ &&
+ $realfile !~ m@/barrier\.h$@ &&
+ $line =~ m/\b($underscore_smp_barriers)\s*\(/ &&
+ $line !~ m/^.\s*\#\s*define\s+($underscore_smp_barriers)\s*\(/) {
+ WARN("MEMORY_BARRIER",
+ "__smp memory barriers shouldn't be used outside barrier.h and asm-generic\n" . $herecurr);
+ }
+
# check for waitqueue_active without a comment.
if ($line =~ /\bwaitqueue_active\s*\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
--
MST
^ permalink raw reply related
* [PATCH v2 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 11:56 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <1452427000-4520-1-git-send-email-mst@redhat.com>
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b3c228..97b8b62 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5116,7 +5116,25 @@ sub process {
}
}
# check for memory barriers without a comment.
- if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
+
+ my $barriers = qr{
+ mb|
+ rmb|
+ wmb|
+ read_barrier_depends
+ }x;
+ my $smp_barriers = qr{
+ store_release|
+ load_acquire|
+ store_mb|
+ ($barriers)
+ }x;
+ my $all_barriers = qr{
+ $barriers|
+ smp_($smp_barriers)
+ }x;
+
+ if ($line =~ /\b($all_barriers)\s*\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
"memory barrier without comment\n" . $herecurr);
--
MST
^ permalink raw reply related
* [PATCH v2 0/3] checkpatch: handling of memory barriers
From: Michael S. Tsirkin @ 2016-01-10 11:56 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel
As part of memory barrier cleanup, this patchset
extends checkpatch to make it easier to stop
incorrect memory barrier usage.
This applies on top of my series
arch: barrier cleanup + barriers for virt
and will be included in the next version of the series.
Changes from v2:
catch optional\s* before () in barriers
rewrite using qr{} instead of map
Michael S. Tsirkin (3):
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
scripts/checkpatch.pl | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
--
MST
^ permalink raw reply
* Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-10 11:42 UTC (permalink / raw)
To: Joe Perches
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1451945750.4334.111.camel@perches.com>
On Mon, Jan 04, 2016 at 02:15:50PM -0800, Joe Perches wrote:
> On Mon, 2016-01-04 at 22:45 +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> > > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > > SMP-only barriers were missing in checkpatch.pl
> > > >
> > > > Refactor code slightly to make adding more variants easier.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > scripts/checkpatch.pl | 9 ++++++++-
> > > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 2b3c228..0245bbe 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -5116,7 +5116,14 @@ sub process {
> > > > }
> > > > }
> > > > # check for memory barriers without a comment.
> > > > - if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
> > > > +
> > > > + my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
> > > > + my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 'smp_store_mb');
> > > > +
> > > > + @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
> > >
> > > I think using map, which so far checkpatch doesn't use,
> > > makes smp_barriers harder to understand and it'd be
> > > better to enumerate them.
> >
> > Okay - I'll rewrite using foreach.
> >
>
> I think using the qr form (like 'our $Attribute' and
> a bunch of others) is the general style that should
> be used for checkpatch.
Thanks - that's what I did in the new version (included in
v3 of the arch cleanup patchset now).
^ permalink raw reply
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Rafael Aquini @ 2016-01-09 23:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mm, linux-kernel, stable, virtualization, Minchan Kim,
Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160109234250-mutt-send-email-mst@redhat.com>
On Sat, Jan 09, 2016 at 11:43:31PM +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 08, 2016 at 02:56:14PM -0500, Rafael Aquini wrote:
> > 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.
> > >
> > > Did you test this with lockdep? You really should for
> > > locking changes, and I'd expect it to warn about this.
> > >
> > > Also, there's another issue there I think: after isolation page could
> > > also get freed before we try to lock it.
> > >
> > > 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.
> > >
> >
> > Nice catch! Thanks for spotting it. I just have one minor nit. See
> > below
> >
> > >
> > > 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;
> > ^^^^[1]
> >
> > > }
> > > + put_page(page);
> > > + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> > > }
> > >
> > > + /* re-add remaining entries */
> > > + list_splice(&processed, &b_dev_info->pages);
> >
> > By breaking the loop at its ordinary and expected way-out case [1]
> > we'll hit list_splice without holding b_dev_info->pages_lock, won't we?
>
> Ouch. right.
>
> > perhaps by adding the following on top of your patch we can address that pickle
> > aforementioned:
>
> I'd rather just goto outside or return.
> But maybe Minchan is right and the original patch is ok.
> I still need to go into this.
>
I went back and did follow up Minchan's argument and his old patch and,
yes, I think we're fine there because, as he states, we're using trylock_page()
in those points page lock will nest into b_dev_info->pages_lock.
OTOH, I understood your work on making sure we would follow up lock nesting
order with strict correctness.
As Minchan's approach keeps the code simpler, I'm voting for it.
Cheers!
-- Rafael
^ permalink raw reply
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Michael S. Tsirkin @ 2016-01-09 21:43 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-mm, linux-kernel, stable, virtualization, Minchan Kim,
Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160108195613.GK6808@t510.redhat.com>
On Fri, Jan 08, 2016 at 02:56:14PM -0500, Rafael Aquini wrote:
> 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.
> >
> > Did you test this with lockdep? You really should for
> > locking changes, and I'd expect it to warn about this.
> >
> > Also, there's another issue there I think: after isolation page could
> > also get freed before we try to lock it.
> >
> > 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.
> >
>
> Nice catch! Thanks for spotting it. I just have one minor nit. See
> below
>
> >
> > 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;
> ^^^^[1]
>
> > }
> > + put_page(page);
> > + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> > }
> >
> > + /* re-add remaining entries */
> > + list_splice(&processed, &b_dev_info->pages);
>
> By breaking the loop at its ordinary and expected way-out case [1]
> we'll hit list_splice without holding b_dev_info->pages_lock, won't we?
Ouch. right.
> perhaps by adding the following on top of your patch we can address that pickle
> aforementioned:
I'd rather just goto outside or return.
But maybe Minchan is right and the original patch is ok.
I still need to go into this.
> Cheers!
> Rafael
> --
>
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 66d69c5..74b3e9c 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -58,7 +58,7 @@ struct page *balloon_page_dequeue(struct
> balloon_dev_info *b_dev_info)
> {
> struct page *page;
> unsigned long flags;
> - bool dequeued_page;
> + bool dequeued_page, locked;
> LIST_HEAD(processed); /* protected by b_dev_info->pages_lock */
>
> dequeued_page = false;
> @@ -105,13 +105,17 @@ struct page *balloon_page_dequeue(struct
> balloon_dev_info *b_dev_info)
> unlock_page(page);
> put_page(page);
> dequeued_page = true;
> + locked = false;
> break;
> }
> put_page(page);
> spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + locked = true;
> }
>
> /* re-add remaining entries */
> + if (!locked)
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> list_splice(&processed, &b_dev_info->pages);
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
^ permalink raw reply
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Minchan Kim @ 2016-01-08 23:43 UTC (permalink / raw)
To: Rafael Aquini
Cc: Michael S. Tsirkin, linux-kernel, stable, virtualization,
linux-mm, Andrew Morton, Konstantin Khlebnikov
In-Reply-To: <20160108195613.GK6808@t510.redhat.com>
On Fri, Jan 08, 2016 at 02:56:14PM -0500, Rafael Aquini wrote:
> 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.
> >
> > Did you test this with lockdep? You really should for
> > locking changes, and I'd expect it to warn about this.
> >
> > Also, there's another issue there I think: after isolation page could
> > also get freed before we try to lock it.
> >
> > 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.
> >
>
> Nice catch! Thanks for spotting it. I just have one minor nit. See
> below
Hmm, As I replied mst's mail, I really cannot understand what you
gouys are pointing out.
If we use lock_page in balloon_page_dequeue, I agree it's deadlock
but we used trylock_page so it's not a deadlock.
About the page refcount, we don't need take a page reference because
if one of page in the list was isolated for migration, the page
shouldn't stay in b_dev_info->pages list so balloon_page_dequeue
cannot touch the page.
Could you elaborate it more detail if I have missed something?
It's a stable patch so I want to be careful.
Thanks.
balloon_page_isolate
{
trylock_page(page)
spin_lock_irqsave(&b_dev_info->pages_lock)
list_del(&page->lru);
}
balloon_page_dequeue
{
spin_lock_irqsave(&b_dev_info->pages_lock)
list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
trylock_page(page)
}
}
^ permalink raw reply
* Re: [PATCH 2/2] virtio_balloon: fix race between migration and ballooning
From: Rafael Aquini @ 2016-01-08 19:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mm, linux-kernel, stable, virtualization, Minchan Kim,
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.
>
> Did you test this with lockdep? You really should for
> locking changes, and I'd expect it to warn about this.
>
> Also, there's another issue there I think: after isolation page could
> also get freed before we try to lock it.
>
> 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.
>
Nice catch! Thanks for spotting it. I just have one minor nit. See
below
>
> 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;
^^^^[1]
> }
> + put_page(page);
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> }
>
> + /* re-add remaining entries */
> + list_splice(&processed, &b_dev_info->pages);
By breaking the loop at its ordinary and expected way-out case [1]
we'll hit list_splice without holding b_dev_info->pages_lock, won't we?
perhaps by adding the following on top of your patch we can address that pickle
aforementioned:
Cheers!
Rafael
--
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 66d69c5..74b3e9c 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -58,7 +58,7 @@ struct page *balloon_page_dequeue(struct
balloon_dev_info *b_dev_info)
{
struct page *page;
unsigned long flags;
- bool dequeued_page;
+ bool dequeued_page, locked;
LIST_HEAD(processed); /* protected by b_dev_info->pages_lock */
dequeued_page = false;
@@ -105,13 +105,17 @@ struct page *balloon_page_dequeue(struct
balloon_dev_info *b_dev_info)
unlock_page(page);
put_page(page);
dequeued_page = true;
+ locked = false;
break;
}
put_page(page);
spin_lock_irqsave(&b_dev_info->pages_lock, flags);
+ locked = true;
}
/* re-add remaining entries */
+ if (!locked)
+ spin_lock_irqsave(&b_dev_info->pages_lock, flags);
list_splice(&processed, &b_dev_info->pages);
spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
^ permalink raw reply related
* Re: [PATCH 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-08 10:14 UTC (permalink / raw)
To: Joe Perches
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Cc : Andy Whitcroft, Thomas Gleixner, linux-metag,
linux-arm-kernel, Andrew
In-Reply-To: <1451945497.4334.107.camel@perches.com>
On Mon, Jan 04, 2016 at 02:11:37PM -0800, Joe Perches wrote:
> 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.
I don't mind. I'll queue a patch like that.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio/s390: use dev_to_virtio
From: Cornelia Huck @ 2016-01-07 15:04 UTC (permalink / raw)
To: Geliang Tang
Cc: linux-s390, kvm, Heiko Carstens, linux-kernel, virtualization,
Christian Borntraeger, Martin Schwidefsky
In-Reply-To: <912bf59bd3a48f2d4d4994681e898dc084fe29d3.1451484163.git.geliangtang@163.com>
On Wed, 30 Dec 2015 22:05:25 +0800
Geliang Tang <geliangtang@163.com> wrote:
> Use dev_to_virtio() instead of open-coding it.
>
> Signed-off-by: Geliang Tang <geliangtang@163.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Thanks, added to my queue.
^ permalink raw reply
* Re: [RFC v4 0/5] Add virtio transport for AF_VSOCK
From: Stefan Hajnoczi @ 2016-01-07 8:34 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, kvm, Michael S. Tsirkin, netdev, virtualization,
Matt Benjamin, Christoffer Dall, matt.ma
In-Reply-To: <568A2577.2020104@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 4683 bytes --]
On Mon, Jan 04, 2016 at 03:55:35PM +0800, Jason Wang wrote:
>
>
> 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.
virtio-net is an Ethernet device with best-effort delivery while
virtio-vsock is guaranteed delivery. That's the fundamental difference
at the protocol level.
At the driver level AF_VSOCK isn't supposed to have a netdev or
user-visible network interface that can be misconfigured by the guest
administrator.
Trying to make the existing virtio-net device work for this use case is
probably nastier than using a dedicated device since lot of the
virtio-net configuration space and control features don't make sense for
AF_VSOCK.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 15/32] powerpc: define __smp_xxx
From: Boqun Feng @ 2016-01-07 0:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, virtualization, Paul Mackerras,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Davidlohr Bueso, Arnd Bergmann, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul E. McKenney, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, Andrey Konovalov,
adi-buildroot-devel, Thomas Gleixner
In-Reply-To: <20160106222337-mutt-send-email-mst@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 1283 bytes --]
On Wed, Jan 06, 2016 at 10:23:51PM +0200, Michael S. Tsirkin wrote:
[...]
> > >
> > > Sorry, I don't understand - why do you have to do anything?
> > > I changed all users of smp_lwsync so they
> > > use __smp_lwsync on SMP and barrier() on !SMP.
> > >
> > > This is exactly the current behaviour, I also tested that
> > > generated code does not change at all.
> > >
> > > Is there a patch in your tree that conflicts with this?
> > >
> >
> > Because in a patchset which implements atomic relaxed/acquire/release
> > variants on PPC I use smp_lwsync(), this makes it have another user,
> > please see this mail:
> >
> > http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
> >
> > in definition of PPC's __atomic_op_release().
> >
> >
> > But I think removing smp_lwsync() is a good idea and actually I think we
> > can go further to remove __smp_lwsync() and let __smp_load_acquire and
> > __smp_store_release call __lwsync() directly, but that is another thing.
> >
> > Anyway, I will modify my patch.
> >
> > Regards,
> > Boqun
>
>
> Thanks!
> Could you send an ack then please?
>
Sure, if you need one from me, feel free to add my ack for this patch:
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Regards,
Boqun
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 15/32] powerpc: define __smp_xxx
From: Michael S. Tsirkin @ 2016-01-06 20:23 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, virtualization, Paul Mackerras,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Davidlohr Bueso, Arnd Bergmann, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul E. McKenney, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, Andrey Konovalov,
adi-buildroot-devel, Thomas Gleixner
In-Reply-To: <20160106015152.GA14605@fixme-laptop.cn.ibm.com>
On Wed, Jan 06, 2016 at 09:51:52AM +0800, Boqun Feng wrote:
> On Tue, Jan 05, 2016 at 06:16:48PM +0200, Michael S. Tsirkin wrote:
> [snip]
> > > > > Another thing is that smp_lwsync() may have a third user(other than
> > > > > smp_load_acquire() and smp_store_release()):
> > > > >
> > > > > http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
> > > > >
> > > > > I'm OK to change my patch accordingly, but do we really want
> > > > > smp_lwsync() get involved in this cleanup? If I understand you
> > > > > correctly, this cleanup focuses on external API like smp_{r,w,}mb(),
> > > > > while smp_lwsync() is internal to PPC.
> > > > >
> > > > > Regards,
> > > > > Boqun
> > > >
> > > > I think you missed the leading ___ :)
> > > >
> > >
> > > What I mean here was smp_lwsync() was originally internal to PPC, but
> > > never mind ;-)
> > >
> > > > smp_store_release is external and it needs __smp_lwsync as
> > > > defined here.
> > > >
> > > > I can duplicate some code and have smp_lwsync *not* call __smp_lwsync
> > >
> > > You mean bringing smp_lwsync() back? because I haven't seen you defining
> > > in asm-generic/barriers.h in previous patches and you just delete it in
> > > this patch.
> > >
> > > > but why do this? Still, if you prefer it this way,
> > > > please let me know.
> > > >
> > >
> > > I think deleting smp_lwsync() is fine, though I need to change atomic
> > > variants patches on PPC because of it ;-/
> > >
> > > Regards,
> > > Boqun
> >
> > Sorry, I don't understand - why do you have to do anything?
> > I changed all users of smp_lwsync so they
> > use __smp_lwsync on SMP and barrier() on !SMP.
> >
> > This is exactly the current behaviour, I also tested that
> > generated code does not change at all.
> >
> > Is there a patch in your tree that conflicts with this?
> >
>
> Because in a patchset which implements atomic relaxed/acquire/release
> variants on PPC I use smp_lwsync(), this makes it have another user,
> please see this mail:
>
> http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
>
> in definition of PPC's __atomic_op_release().
>
>
> But I think removing smp_lwsync() is a good idea and actually I think we
> can go further to remove __smp_lwsync() and let __smp_load_acquire and
> __smp_store_release call __lwsync() directly, but that is another thing.
>
> Anyway, I will modify my patch.
>
> Regards,
> Boqun
Thanks!
Could you send an ack then please?
> >
> > > > > > WRITE_ONCE(*p, v); \
> > > > > > } while (0)
> > > > > >
> > > > > > -#define smp_load_acquire(p) \
> > > > > > +#define __smp_load_acquire(p) \
> > > > > > ({ \
> > > > > > typeof(*p) ___p1 = READ_ONCE(*p); \
> > > > > > compiletime_assert_atomic_type(*p); \
> > > > > > - smp_lwsync(); \
> > > > > > + __smp_lwsync(); \
> > > > > > ___p1; \
> > > > > > })
> > > > > >
> > > > > > --
> > > > > > MST
> > > > > >
> > > > > > --
> > > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH RFC] vhost: basic device IOTLB support
From: Jason Wang @ 2016-01-06 4:57 UTC (permalink / raw)
To: Yang Zhang, mst, kvm, virtualization, netdev, linux-kernel,
linux-api
In-Reply-To: <568B35F8.7080302@gmail.com>
On 01/05/2016 11:18 AM, Yang Zhang wrote:
> 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.
A little bit confused, I believe the first step is to exporting an IOMMU
to L1 guest for it to use for a assigned device?
> 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.
Interesting but I think the goal is different. This patch tries to make
vhost/virtio works with emulated IOMMU.
>
>>
>> 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
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