* [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores
@ 2024-02-21 1:17 Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 1/6] x86: Stop using weak symbols for __iowrite32_copy() Jason Gunthorpe
` (5 more replies)
0 siblings, 6 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
mlx5 has a built in self-test at driver startup to evaluate if the
platform supports write combining to generate a 64 byte PCIe TLP or
not. This has proven necessary because a lot of common scenarios end up
with broken write combining (especially inside virtual machines) and there
is no other way to learn this information.
This self test has been consistently failing on new ARM64 CPU
designs (specifically with NVIDIA Grace's implementation of Neoverse
V2). The C loop around writeq() generates some pretty terrible ARM64
assembly, but historically this has worked on alot of existing ARM64 CPUs
till now.
We see it succeed about 1 time in 10,000 on the worst affected
systems. The CPU architects speculate that the load instructions
interspersed with the stores make the test unreliable.
Arrange things so that the ARM64 uses a predictable inline assembly block
of 8 STR instructions.
Catalin suggested implementing this in terms of the obscure
__iowrite64_copy() interface which was long ago added to optimize write
combining stores on Pathscale RDMA HW for x86. These copy routines have
the advantage of requiring the caller to supply alignment which allows an
optimal assembly implementation.
This is a good suggestion because it turns out that S390 has much the same
problem and already uses the __iowrite64_copy() to try to make its WC
operations work.
The first several patches modernize and improve the performance of
__iowriteXX_copy() so that an ARM64 implementation can be provided which
relies on __builtin_constant_p to generate fast inlined assembly code in a
few common cases.
It should come after the S390 fix:
https://lore.kernel.org/r/0-v1-9b1ea6869554+110c60-iommufd_ck_data_type_jgg@nvidia.com
With acks, I'd like to take this through the RDMA tree.
v2:
- Rework everything to use __iowrite64_copy().
- Don't use STP since that is not reliably supported in ARM VMs
- New patches to tidy up __iowriteXX_copy() on x86 and s390
v1: https://lore.kernel.org/r/cover.1700766072.git.leon@kernel.org
Jason Gunthorpe (6):
x86: Stop using weak symbols for __iowrite32_copy()
s390: Implement __iowrite32_copy()
s390: Stop using weak symbols for __iowrite64_copy()
arm64/io: Provide a WC friendly __iowriteXX_copy()
net: hns3: Remove io_stop_wc() calls after __iowrite64_copy()
IB/mlx5: Use __iowrite64_copy() for write combining stores
arch/arm64/include/asm/io.h | 132 ++++++++++++++++++
arch/arm64/kernel/io.c | 42 ++++++
arch/s390/include/asm/io.h | 15 ++
arch/s390/pci/pci.c | 6 -
arch/x86/include/asm/io.h | 17 +++
arch/x86/lib/Makefile | 1 -
arch/x86/lib/iomap_copy_64.S | 15 --
drivers/infiniband/hw/mlx5/mem.c | 8 +-
.../net/ethernet/hisilicon/hns3/hns3_enet.c | 4 -
include/linux/io.h | 8 +-
lib/iomap_copy.c | 13 +-
11 files changed, 222 insertions(+), 39 deletions(-)
delete mode 100644 arch/x86/lib/iomap_copy_64.S
base-commit: 3856559a378586366dc602f93febe1c57cdc366c
--
2.43.2
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/6] x86: Stop using weak symbols for __iowrite32_copy()
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 2/6] s390: Implement __iowrite32_copy() Jason Gunthorpe
` (4 subsequent siblings)
5 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
Start switching iomap_copy routines over to use #define and arch provided
inline/macro functions instead of weak symbols.
Inline functions allow more compiler optimization and this is often a
driver hot path.
x86 has the only weak implementation for __iowrite32_copy(), so replace it
with a static inline containing the same single instruction inline
assembly. The compiler will generate the "mov edx,ecx" in a more optimal
way.
Remove iomap_copy_64.S
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
arch/x86/include/asm/io.h | 17 +++++++++++++++++
arch/x86/lib/Makefile | 1 -
arch/x86/lib/iomap_copy_64.S | 15 ---------------
include/linux/io.h | 5 ++++-
lib/iomap_copy.c | 6 +++---
5 files changed, 24 insertions(+), 20 deletions(-)
delete mode 100644 arch/x86/lib/iomap_copy_64.S
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 3814a9263d64ea..2483988f0b7f7b 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -209,6 +209,23 @@ void memset_io(volatile void __iomem *, int, size_t);
#define memcpy_toio memcpy_toio
#define memset_io memset_io
+#ifdef CONFIG_X86_64
+/*
+ * Commit 0f07496144c2 ("[PATCH] Add faster __iowrite32_copy routine for
+ * x86_64") says that circa 2006 rep movsl is noticeably faster than a copy
+ * loop.
+ */
+static inline void __iowrite32_copy(void __iomem *to, const void *from,
+ size_t count)
+{
+ asm volatile("rep ; movsl"
+ : "=&c"(count), "=&D"(to), "=&S"(from)
+ : "0"(count), "1"(to), "2"(from)
+ : "memory");
+}
+#define __iowrite32_copy __iowrite32_copy
+#endif
+
/*
* ISA space is 'always mapped' on a typical x86 system, no need to
* explicitly ioremap() it. The fact that the ISA IO space is mapped
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index ea3a28e7b613cc..af4ea3d83ba1f6 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -66,7 +66,6 @@ ifneq ($(CONFIG_X86_CMPXCHG64),y)
lib-y += atomic64_386_32.o
endif
else
- obj-y += iomap_copy_64.o
ifneq ($(CONFIG_GENERIC_CSUM),y)
lib-y += csum-partial_64.o csum-copy_64.o csum-wrappers_64.o
endif
diff --git a/arch/x86/lib/iomap_copy_64.S b/arch/x86/lib/iomap_copy_64.S
deleted file mode 100644
index 6ff2f56cb0f71a..00000000000000
--- a/arch/x86/lib/iomap_copy_64.S
+++ /dev/null
@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright 2006 PathScale, Inc. All Rights Reserved.
- */
-
-#include <linux/linkage.h>
-
-/*
- * override generic version in lib/iomap_copy.c
- */
-SYM_FUNC_START(__iowrite32_copy)
- movl %edx,%ecx
- rep movsl
- RET
-SYM_FUNC_END(__iowrite32_copy)
diff --git a/include/linux/io.h b/include/linux/io.h
index 7304f2a69960a3..68cd551b6af112 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -16,7 +16,10 @@
struct device;
struct resource;
-__visible void __iowrite32_copy(void __iomem *to, const void *from, size_t count);
+#ifndef __iowrite32_copy
+void __iowrite32_copy(void __iomem *to, const void *from, size_t count);
+#endif
+
void __ioread32_copy(void *to, const void __iomem *from, size_t count);
void __iowrite64_copy(void __iomem *to, const void *from, size_t count);
diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
index 5de7c04e05ef56..8ddcbb53507dfe 100644
--- a/lib/iomap_copy.c
+++ b/lib/iomap_copy.c
@@ -16,9 +16,8 @@
* time. Order of access is not guaranteed, nor is a memory barrier
* performed afterwards.
*/
-void __attribute__((weak)) __iowrite32_copy(void __iomem *to,
- const void *from,
- size_t count)
+#ifndef __iowrite32_copy
+void __iowrite32_copy(void __iomem *to, const void *from, size_t count)
{
u32 __iomem *dst = to;
const u32 *src = from;
@@ -28,6 +27,7 @@ void __attribute__((weak)) __iowrite32_copy(void __iomem *to,
__raw_writel(*src++, dst++);
}
EXPORT_SYMBOL_GPL(__iowrite32_copy);
+#endif
/**
* __ioread32_copy - copy data from MMIO space, in 32-bit units
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/6] s390: Implement __iowrite32_copy()
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 1/6] x86: Stop using weak symbols for __iowrite32_copy() Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 3/6] s390: Stop using weak symbols for __iowrite64_copy() Jason Gunthorpe
` (3 subsequent siblings)
5 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
It is trivial to implement an inline to do this, so provide it in the s390
headers. Like the 64 bit version it should just invoke zpci_memcpy_toio()
with the correct size.
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
arch/s390/include/asm/io.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index 4453ad7c11aced..00704fc8a54b30 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -73,6 +73,14 @@ static inline void ioport_unmap(void __iomem *p)
#define __raw_writel zpci_write_u32
#define __raw_writeq zpci_write_u64
+/* combine single writes by using store-block insn */
+static inline void __iowrite32_copy(void __iomem *to, const void *from,
+ size_t count)
+{
+ zpci_memcpy_toio(to, from, count * 4);
+}
+#define __iowrite32_copy __iowrite32_copy
+
#endif /* CONFIG_PCI */
#include <asm-generic/io.h>
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/6] s390: Stop using weak symbols for __iowrite64_copy()
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 1/6] x86: Stop using weak symbols for __iowrite32_copy() Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 2/6] s390: Implement __iowrite32_copy() Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
` (2 subsequent siblings)
5 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
Complete switching the __iowriteXX_copy() routines over to use #define and
arch provided inline/macro functions instead of weak symbols.
S390 has an implementation that simply calls another memcpy
function. Inline this so the callers don't have to do two jumps.
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
arch/s390/include/asm/io.h | 7 +++++++
arch/s390/pci/pci.c | 6 ------
include/linux/io.h | 3 +++
lib/iomap_copy.c | 7 +++----
4 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index 00704fc8a54b30..0fbc992d7a5ea7 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -81,6 +81,13 @@ static inline void __iowrite32_copy(void __iomem *to, const void *from,
}
#define __iowrite32_copy __iowrite32_copy
+static inline void __iowrite64_copy(void __iomem *to, const void *from,
+ size_t count)
+{
+ zpci_memcpy_toio(to, from, count * 8);
+}
+#define __iowrite64_copy __iowrite64_copy
+
#endif /* CONFIG_PCI */
#include <asm-generic/io.h>
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 52a44e353796c0..fb81337a73eaab 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -249,12 +249,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
return 0;
}
-/* combine single writes by using store-block insn */
-void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
-{
- zpci_memcpy_toio(to, from, count * 8);
-}
-
void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
unsigned long prot)
{
diff --git a/include/linux/io.h b/include/linux/io.h
index 68cd551b6af112..fe12be9de6f7a6 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -21,7 +21,10 @@ void __iowrite32_copy(void __iomem *to, const void *from, size_t count);
#endif
void __ioread32_copy(void *to, const void __iomem *from, size_t count);
+
+#ifndef __iowrite64_copy
void __iowrite64_copy(void __iomem *to, const void *from, size_t count);
+#endif
#ifdef CONFIG_MMU
int ioremap_page_range(unsigned long addr, unsigned long end,
diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
index 8ddcbb53507dfe..2fd5712fb7c02b 100644
--- a/lib/iomap_copy.c
+++ b/lib/iomap_copy.c
@@ -60,9 +60,8 @@ EXPORT_SYMBOL_GPL(__ioread32_copy);
* time. Order of access is not guaranteed, nor is a memory barrier
* performed afterwards.
*/
-void __attribute__((weak)) __iowrite64_copy(void __iomem *to,
- const void *from,
- size_t count)
+#ifndef __iowrite64_copy
+void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
{
#ifdef CONFIG_64BIT
u64 __iomem *dst = to;
@@ -75,5 +74,5 @@ void __attribute__((weak)) __iowrite64_copy(void __iomem *to,
__iowrite32_copy(to, from, count * 2);
#endif
}
-
EXPORT_SYMBOL_GPL(__iowrite64_copy);
+#endif
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
` (2 preceding siblings ...)
2024-02-21 1:17 ` [PATCH 3/6] s390: Stop using weak symbols for __iowrite64_copy() Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
2024-02-21 19:22 ` Will Deacon
` (4 more replies)
2024-02-21 1:17 ` [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy() Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 6/6] IB/mlx5: Use __iowrite64_copy() for write combining stores Jason Gunthorpe
5 siblings, 5 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
The kernel provides driver support for using write combining IO memory
through the __iowriteXX_copy() API which is commonly used as an optional
optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
iomap_copy.c provides a generic implementation as a simple 4/8 byte at a
time copy loop that has worked well with past ARM64 CPUs, giving a high
frequency of large TLPs being successfully formed.
However modern ARM64 CPUs are quite sensitive to how the write combining
CPU HW is operated and a compiler generated loop with intermixed
load/store is not sufficient to frequently generate a large TLP. The CPUs
would like to see the entire TLP generated by consecutive store
instructions from registers. Compilers like gcc tend to intermix loads and
stores and have poor code generation, in part, due to the ARM64 situation
that writeq() does not codegen anything other than "[xN]". However even
with that resolved compilers like clang still do not have good code
generation.
This means on modern ARM64 CPUs the rate at which __iowriteXX_copy()
successfully generates large TLPs is very small (less than 1 in 10,000)
tries), to the point that the use of WC is pointless.
Implement __iowrite32/64_copy() specifically for ARM64 and use inline
assembly to build consecutive blocks of STR instructions. Provide direct
support for 64/32/16 large TLP generation in this manner. Optimize for
common constant lengths so that the compiler can directly inline the store
blocks.
This brings the frequency of large TLP generation up to a high level that
is comparable with older CPU generations.
As the __iowriteXX_copy() family of APIs is intended for use with WC
incorporate the DGH hint directly into the function.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arch@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
arch/arm64/include/asm/io.h | 132 ++++++++++++++++++++++++++++++++++++
arch/arm64/kernel/io.c | 42 ++++++++++++
2 files changed, 174 insertions(+)
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 3b694511b98f83..471ab46621e7d6 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -135,6 +135,138 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
#define memcpy_fromio(a,c,l) __memcpy_fromio((a),(c),(l))
#define memcpy_toio(c,a,l) __memcpy_toio((c),(a),(l))
+/*
+ * The ARM64 iowrite implementation is intended to support drivers that want to
+ * use write combining. For instance PCI drivers using write combining with a 64
+ * byte __iowrite64_copy() expect to get a 64 byte MemWr TLP on the PCIe bus.
+ *
+ * Newer ARM core have sensitive write combining buffers, it is important that
+ * the stores be contiguous blocks of store instructions. Normal memcpy
+ * approaches have a very low chance to generate write combining.
+ *
+ * Since this is the only API on ARM64 that should be used with write combining
+ * it also integrates the DGH hint which is supposed to lower the latency to
+ * emit the large TLP from the CPU.
+ */
+
+static inline void __const_memcpy_toio_aligned32(volatile u32 __iomem *to,
+ const u32 *from, size_t count)
+{
+ switch (count) {
+ case 8:
+ asm volatile("str %w0, [%8, #4 * 0]\n"
+ "str %w1, [%8, #4 * 1]\n"
+ "str %w2, [%8, #4 * 2]\n"
+ "str %w3, [%8, #4 * 3]\n"
+ "str %w4, [%8, #4 * 4]\n"
+ "str %w5, [%8, #4 * 5]\n"
+ "str %w6, [%8, #4 * 6]\n"
+ "str %w7, [%8, #4 * 7]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
+ "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
+ "rZ"(from[6]), "rZ"(from[7]), "r"(to));
+ break;
+ case 4:
+ asm volatile("str %w0, [%4, #4 * 0]\n"
+ "str %w1, [%4, #4 * 1]\n"
+ "str %w2, [%4, #4 * 2]\n"
+ "str %w3, [%4, #4 * 3]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
+ "rZ"(from[3]), "r"(to));
+ break;
+ case 2:
+ asm volatile("str %w0, [%2, #4 * 0]\n"
+ "str %w1, [%2, #4 * 1]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "r"(to));
+ break;
+ case 1:
+ __raw_writel(*from, to);
+ break;
+ default:
+ BUILD_BUG();
+ }
+}
+
+void __iowrite32_copy_full(void __iomem *to, const void *from, size_t count);
+
+static inline void __const_iowrite32_copy(void __iomem *to, const void *from,
+ size_t count)
+{
+ if (count == 8 || count == 4 || count == 2 || count == 1) {
+ __const_memcpy_toio_aligned32(to, from, count);
+ dgh();
+ } else {
+ __iowrite32_copy_full(to, from, count);
+ }
+}
+
+#define __iowrite32_copy(to, from, count) \
+ (__builtin_constant_p(count) ? \
+ __const_iowrite32_copy(to, from, count) : \
+ __iowrite32_copy_full(to, from, count))
+
+static inline void __const_memcpy_toio_aligned64(volatile u64 __iomem *to,
+ const u64 *from, size_t count)
+{
+ switch (count) {
+ case 8:
+ asm volatile("str %x0, [%8, #8 * 0]\n"
+ "str %x1, [%8, #8 * 1]\n"
+ "str %x2, [%8, #8 * 2]\n"
+ "str %x3, [%8, #8 * 3]\n"
+ "str %x4, [%8, #8 * 4]\n"
+ "str %x5, [%8, #8 * 5]\n"
+ "str %x6, [%8, #8 * 6]\n"
+ "str %x7, [%8, #8 * 7]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
+ "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
+ "rZ"(from[6]), "rZ"(from[7]), "r"(to));
+ break;
+ case 4:
+ asm volatile("str %x0, [%4, #8 * 0]\n"
+ "str %x1, [%4, #8 * 1]\n"
+ "str %x2, [%4, #8 * 2]\n"
+ "str %x3, [%4, #8 * 3]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
+ "rZ"(from[3]), "r"(to));
+ break;
+ case 2:
+ asm volatile("str %x0, [%2, #8 * 0]\n"
+ "str %x1, [%2, #8 * 1]\n"
+ :
+ : "rZ"(from[0]), "rZ"(from[1]), "r"(to));
+ break;
+ case 1:
+ __raw_writel(*from, to);
+ break;
+ default:
+ BUILD_BUG();
+ }
+}
+
+void __iowrite64_copy_full(void __iomem *to, const void *from, size_t count);
+
+static inline void __const_iowrite64_copy(void __iomem *to, const void *from,
+ size_t count)
+{
+ if (count == 8 || count == 4 || count == 2 || count == 1) {
+ __const_memcpy_toio_aligned64(to, from, count);
+ dgh();
+ } else {
+ __iowrite64_copy_full(to, from, count);
+ }
+}
+
+#define __iowrite64_copy(to, from, count) \
+ (__builtin_constant_p(count) ? \
+ __const_iowrite64_copy(to, from, count) : \
+ __iowrite64_copy_full(to, from, count))
+
/*
* I/O memory mapping functions.
*/
diff --git a/arch/arm64/kernel/io.c b/arch/arm64/kernel/io.c
index aa7a4ec6a3ae6f..ef48089fbfe1a4 100644
--- a/arch/arm64/kernel/io.c
+++ b/arch/arm64/kernel/io.c
@@ -37,6 +37,48 @@ void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
}
EXPORT_SYMBOL(__memcpy_fromio);
+/*
+ * This generates a memcpy that works on a from/to address which is aligned to
+ * bits. Count is in terms of the number of bits sized quantities to copy. It
+ * optimizes to use the STR groupings when possible so that it is WC friendly.
+ */
+#define memcpy_toio_aligned(to, from, count, bits) \
+ ({ \
+ volatile u##bits __iomem *_to = to; \
+ const u##bits *_from = from; \
+ size_t _count = count; \
+ const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
+ \
+ for (; _from < _end_from; _from += 8, _to += 8) \
+ __const_memcpy_toio_aligned##bits(_to, _from, 8); \
+ if ((_count % 8) >= 4) { \
+ __const_memcpy_toio_aligned##bits(_to, _from, 4); \
+ _from += 4; \
+ _to += 4; \
+ } \
+ if ((_count % 4) >= 2) { \
+ __const_memcpy_toio_aligned##bits(_to, _from, 2); \
+ _from += 2; \
+ _to += 2; \
+ } \
+ if (_count % 2) \
+ __const_memcpy_toio_aligned##bits(_to, _from, 1); \
+ })
+
+void __iowrite64_copy_full(void __iomem *to, const void *from, size_t count)
+{
+ memcpy_toio_aligned(to, from, count, 64);
+ dgh();
+}
+EXPORT_SYMBOL(__iowrite64_copy_full);
+
+void __iowrite32_copy_full(void __iomem *to, const void *from, size_t count)
+{
+ memcpy_toio_aligned(to, from, count, 32);
+ dgh();
+}
+EXPORT_SYMBOL(__iowrite32_copy_full);
+
/*
* Copy data from "real" memory space to IO memory space.
*/
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy()
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
` (3 preceding siblings ...)
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
2024-02-22 0:57 ` Jijie Shao
2024-02-21 1:17 ` [PATCH 6/6] IB/mlx5: Use __iowrite64_copy() for write combining stores Jason Gunthorpe
5 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
Now that the ARM64 arch implementation does the DGH as part of
__iowrite64_copy() there is no reason to open code this in drivers.
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index f1695c889d3a07..ff556c2b5bacb4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2068,8 +2068,6 @@ static void hns3_tx_push_bd(struct hns3_enet_ring *ring, int num)
__iowrite64_copy(ring->tqp->mem_base, desc,
(sizeof(struct hns3_desc) * HNS3_MAX_PUSH_BD_NUM) /
HNS3_BYTES_PER_64BIT);
-
- io_stop_wc();
}
static void hns3_tx_mem_doorbell(struct hns3_enet_ring *ring)
@@ -2088,8 +2086,6 @@ static void hns3_tx_mem_doorbell(struct hns3_enet_ring *ring)
u64_stats_update_begin(&ring->syncp);
ring->stats.tx_mem_doorbell += ring->pending_buf;
u64_stats_update_end(&ring->syncp);
-
- io_stop_wc();
}
static void hns3_tx_doorbell(struct hns3_enet_ring *ring, int num,
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 6/6] IB/mlx5: Use __iowrite64_copy() for write combining stores
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
` (4 preceding siblings ...)
2024-02-21 1:17 ` [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy() Jason Gunthorpe
@ 2024-02-21 1:17 ` Jason Gunthorpe
5 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 1:17 UTC (permalink / raw
To: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky, linux-arch,
linux-arm-kernel, Mark Rutland, Michael Guralnik, patches,
Niklas Schnelle, Will Deacon
mlx5 has a built in self-test at driver startup to evaluate if the
platform supports write combining to generate a 64 byte PCIe TLP or
not. This has proven necessary because a lot of common scenarios end up
with broken write combining (especially inside virtual machines) and there
is other way to learn this information.
This self test has been consistently failing on new ARM64 CPU
designs (specifically with NVIDIA Grace's implementation of Neoverse
V2). The C loop around writeq() generates some pretty terrible ARM64
assembly, but historically this has worked on a lot of existing ARM64 CPUs
till now.
We see it succeed about 1 time in 10,000 on the worst effected
systems. The CPU architects speculate that the load instructions
interspersed with the stores makes the WC buffers statistically flush too
often and thus the generation of large TLPs becomes infrequent. This makes
the boot up test unreliable in that it indicates no write-combining,
however userspace would be fine since it uses a ST4 instruction.
Further, S390 has similar issues where only the special zpci_memcpy_toio()
will actually generate large TLPs, and the open coded loop does not
trigger it at all.
Fix both ARM64 and S390 by switching to __iowrite64_copy() which now
provides architecture specific variants that have a high change of
generating a large TLP with write combining. x86 continues to use a
similar writeq loop in the generate __iowrite64_copy().
Fixes: 11f552e21755 ("IB/mlx5: Test write combining support")
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/infiniband/hw/mlx5/mem.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/mem.c b/drivers/infiniband/hw/mlx5/mem.c
index 96ffbbaf0a73d1..5a22be14d958f2 100644
--- a/drivers/infiniband/hw/mlx5/mem.c
+++ b/drivers/infiniband/hw/mlx5/mem.c
@@ -30,6 +30,7 @@
* SOFTWARE.
*/
+#include <linux/io.h>
#include <rdma/ib_umem_odp.h>
#include "mlx5_ib.h"
#include <linux/jiffies.h>
@@ -108,7 +109,6 @@ static int post_send_nop(struct mlx5_ib_dev *dev, struct ib_qp *ibqp, u64 wr_id,
__be32 mmio_wqe[16] = {};
unsigned long flags;
unsigned int idx;
- int i;
if (unlikely(dev->mdev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR))
return -EIO;
@@ -148,10 +148,8 @@ static int post_send_nop(struct mlx5_ib_dev *dev, struct ib_qp *ibqp, u64 wr_id,
* we hit doorbell
*/
wmb();
- for (i = 0; i < 8; i++)
- mlx5_write64(&mmio_wqe[i * 2],
- bf->bfreg->map + bf->offset + i * 8);
- io_stop_wc();
+ __iowrite64_copy(bf->bfreg->map + bf->offset, mmio_wqe,
+ sizeof(mmio_wqe) / 8);
bf->offset ^= bf->buf_size;
--
2.43.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
@ 2024-02-21 19:22 ` Will Deacon
2024-02-21 23:28 ` Jason Gunthorpe
2024-02-22 22:05 ` David Laight
` (3 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Will Deacon @ 2024-02-21 19:22 UTC (permalink / raw
To: Jason Gunthorpe
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch, linux-arm-kernel,
Mark Rutland, Michael Guralnik, patches, Niklas Schnelle
On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> +static inline void __const_memcpy_toio_aligned64(volatile u64 __iomem *to,
> + const u64 *from, size_t count)
> +{
> + switch (count) {
> + case 8:
> + asm volatile("str %x0, [%8, #8 * 0]\n"
> + "str %x1, [%8, #8 * 1]\n"
> + "str %x2, [%8, #8 * 2]\n"
> + "str %x3, [%8, #8 * 3]\n"
> + "str %x4, [%8, #8 * 4]\n"
> + "str %x5, [%8, #8 * 5]\n"
> + "str %x6, [%8, #8 * 6]\n"
> + "str %x7, [%8, #8 * 7]\n"
> + :
> + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> + "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
> + "rZ"(from[6]), "rZ"(from[7]), "r"(to));
> + break;
> + case 4:
> + asm volatile("str %x0, [%4, #8 * 0]\n"
> + "str %x1, [%4, #8 * 1]\n"
> + "str %x2, [%4, #8 * 2]\n"
> + "str %x3, [%4, #8 * 3]\n"
> + :
> + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> + "rZ"(from[3]), "r"(to));
> + break;
> + case 2:
> + asm volatile("str %x0, [%2, #8 * 0]\n"
> + "str %x1, [%2, #8 * 1]\n"
> + :
> + : "rZ"(from[0]), "rZ"(from[1]), "r"(to));
> + break;
> + case 1:
> + __raw_writel(*from, to);
Shouldn't this be __raw_writeq?
Will
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 19:22 ` Will Deacon
@ 2024-02-21 23:28 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-21 23:28 UTC (permalink / raw
To: Will Deacon
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch, linux-arm-kernel,
Mark Rutland, Michael Guralnik, patches, Niklas Schnelle
On Wed, Feb 21, 2024 at 07:22:06PM +0000, Will Deacon wrote:
> On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > +static inline void __const_memcpy_toio_aligned64(volatile u64 __iomem *to,
> > + const u64 *from, size_t count)
> > +{
> > + switch (count) {
> > + case 8:
> > + asm volatile("str %x0, [%8, #8 * 0]\n"
> > + "str %x1, [%8, #8 * 1]\n"
> > + "str %x2, [%8, #8 * 2]\n"
> > + "str %x3, [%8, #8 * 3]\n"
> > + "str %x4, [%8, #8 * 4]\n"
> > + "str %x5, [%8, #8 * 5]\n"
> > + "str %x6, [%8, #8 * 6]\n"
> > + "str %x7, [%8, #8 * 7]\n"
> > + :
> > + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> > + "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
> > + "rZ"(from[6]), "rZ"(from[7]), "r"(to));
> > + break;
> > + case 4:
> > + asm volatile("str %x0, [%4, #8 * 0]\n"
> > + "str %x1, [%4, #8 * 1]\n"
> > + "str %x2, [%4, #8 * 2]\n"
> > + "str %x3, [%4, #8 * 3]\n"
> > + :
> > + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> > + "rZ"(from[3]), "r"(to));
> > + break;
> > + case 2:
> > + asm volatile("str %x0, [%2, #8 * 0]\n"
> > + "str %x1, [%2, #8 * 1]\n"
> > + :
> > + : "rZ"(from[0]), "rZ"(from[1]), "r"(to));
> > + break;
> > + case 1:
> > + __raw_writel(*from, to);
>
> Shouldn't this be __raw_writeq?
Yes! Thanks
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy()
2024-02-21 1:17 ` [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy() Jason Gunthorpe
@ 2024-02-22 0:57 ` Jijie Shao
0 siblings, 0 replies; 31+ messages in thread
From: Jijie Shao @ 2024-02-22 0:57 UTC (permalink / raw
To: Jason Gunthorpe, Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma, linux-s390, llvm, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers, netdev,
Paolo Abeni, Salil Mehta, Sven Schnelle, Thomas Gleixner, x86,
Yisen Zhuang
Cc: shaojijie, Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch, linux-arm-kernel, Mark Rutland, Michael Guralnik,
patches, Niklas Schnelle, Will Deacon
Reviewed-by: Jijie Shao<shaojijie@huawei.com>
on 2024/2/21 9:17, Jason Gunthorpe wrote:
> Now that the ARM64 arch implementation does the DGH as part of
> __iowrite64_copy() there is no reason to open code this in drivers.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index f1695c889d3a07..ff556c2b5bacb4 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2068,8 +2068,6 @@ static void hns3_tx_push_bd(struct hns3_enet_ring *ring, int num)
> __iowrite64_copy(ring->tqp->mem_base, desc,
> (sizeof(struct hns3_desc) * HNS3_MAX_PUSH_BD_NUM) /
> HNS3_BYTES_PER_64BIT);
> -
> - io_stop_wc();
> }
>
> static void hns3_tx_mem_doorbell(struct hns3_enet_ring *ring)
> @@ -2088,8 +2086,6 @@ static void hns3_tx_mem_doorbell(struct hns3_enet_ring *ring)
> u64_stats_update_begin(&ring->syncp);
> ring->stats.tx_mem_doorbell += ring->pending_buf;
> u64_stats_update_end(&ring->syncp);
> -
> - io_stop_wc();
> }
>
> static void hns3_tx_doorbell(struct hns3_enet_ring *ring, int num,
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
2024-02-21 19:22 ` Will Deacon
@ 2024-02-22 22:05 ` David Laight
2024-02-22 22:36 ` Jason Gunthorpe
2024-02-27 10:37 ` Catalin Marinas
` (2 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: David Laight @ 2024-02-22 22:05 UTC (permalink / raw
To: 'Jason Gunthorpe', Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang
Cc: Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Niklas Schnelle, Will Deacon
From: Jason Gunthorpe
> Sent: 21 February 2024 01:17
>
> The kernel provides driver support for using write combining IO memory
> through the __iowriteXX_copy() API which is commonly used as an optional
> optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
>
...
> Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> assembly to build consecutive blocks of STR instructions. Provide direct
> support for 64/32/16 large TLP generation in this manner. Optimize for
> common constant lengths so that the compiler can directly inline the store
> blocks.
...
> +/*
> + * This generates a memcpy that works on a from/to address which is aligned to
> + * bits. Count is in terms of the number of bits sized quantities to copy. It
> + * optimizes to use the STR groupings when possible so that it is WC friendly.
> + */
> +#define memcpy_toio_aligned(to, from, count, bits) \
> + ({ \
> + volatile u##bits __iomem *_to = to; \
> + const u##bits *_from = from; \
> + size_t _count = count; \
> + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> + \
> + for (; _from < _end_from; _from += 8, _to += 8) \
> + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> + if ((_count % 8) >= 4) {
If (_count & 4) {
\
> + __const_memcpy_toio_aligned##bits(_to, _from, 4); \
> + _from += 4; \
> + _to += 4; \
> + } \
> + if ((_count % 4) >= 2) { \
Ditto
> + __const_memcpy_toio_aligned##bits(_to, _from, 2); \
> + _from += 2; \
> + _to += 2; \
> + } \
> + if (_count % 2) \
and again
> + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> + })
But that looks bit a bit large to be inlined.
Except, perhaps, for small constant lengths.
I'd guess that even with write-combining and posted PCIe writes it
doesn't take much for it to be PCIe limited rather than cpu limited?
Is there a sane way to do the same for reads - they are far worse
than writes.
I solved the problem a few years back on a little ppc by using an on-cpu
DMA controller that could do PCIe master accesses and spinning until
the transfer completed.
But that sort of DMA controller seems uncommon.
We now initiate most of the transfers from the slave (an fpga) - after
writing a suitable/sane dma controller for that end.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-22 22:05 ` David Laight
@ 2024-02-22 22:36 ` Jason Gunthorpe
2024-02-23 9:07 ` David Laight
0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-22 22:36 UTC (permalink / raw
To: David Laight
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Niklas Schnelle,
Will Deacon
On Thu, Feb 22, 2024 at 10:05:04PM +0000, David Laight wrote:
> From: Jason Gunthorpe
> > Sent: 21 February 2024 01:17
> >
> > The kernel provides driver support for using write combining IO memory
> > through the __iowriteXX_copy() API which is commonly used as an optional
> > optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
> >
> ...
> > Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> > assembly to build consecutive blocks of STR instructions. Provide direct
> > support for 64/32/16 large TLP generation in this manner. Optimize for
> > common constant lengths so that the compiler can directly inline the store
> > blocks.
> ...
> > +/*
> > + * This generates a memcpy that works on a from/to address which is aligned to
> > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > + */
> > +#define memcpy_toio_aligned(to, from, count, bits) \
> > + ({ \
> > + volatile u##bits __iomem *_to = to; \
> > + const u##bits *_from = from; \
> > + size_t _count = count; \
> > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > + \
> > + for (; _from < _end_from; _from += 8, _to += 8) \
> > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > + if ((_count % 8) >= 4) {
>
> If (_count & 4) {
That would be obfuscating, IMHO. The compiler doesn't need such things
to generate optimal code.
> > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > + })
>
> But that looks bit a bit large to be inlined.
You trimmed alot, this #define is in a C file and it is a template to
generate the 32 and 64 bit out of line functions. Things are done like
this because the 32/64 version are exactly the same logic except just
with different types and sizes.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-22 22:36 ` Jason Gunthorpe
@ 2024-02-23 9:07 ` David Laight
2024-02-23 11:01 ` Niklas Schnelle
2024-02-23 11:38 ` Niklas Schnelle
0 siblings, 2 replies; 31+ messages in thread
From: David Laight @ 2024-02-23 9:07 UTC (permalink / raw
To: 'Jason Gunthorpe'
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Niklas Schnelle,
Will Deacon
From: Jason Gunthorpe
> Sent: 22 February 2024 22:36
> To: David Laight <David.Laight@ACULAB.COM>
>
> On Thu, Feb 22, 2024 at 10:05:04PM +0000, David Laight wrote:
> > From: Jason Gunthorpe
> > > Sent: 21 February 2024 01:17
> > >
> > > The kernel provides driver support for using write combining IO memory
> > > through the __iowriteXX_copy() API which is commonly used as an optional
> > > optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
> > >
> > ...
> > > Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> > > assembly to build consecutive blocks of STR instructions. Provide direct
> > > support for 64/32/16 large TLP generation in this manner. Optimize for
> > > common constant lengths so that the compiler can directly inline the store
> > > blocks.
> > ...
> > > +/*
> > > + * This generates a memcpy that works on a from/to address which is aligned to
> > > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > > + */
> > > +#define memcpy_toio_aligned(to, from, count, bits) \
> > > + ({ \
> > > + volatile u##bits __iomem *_to = to; \
> > > + const u##bits *_from = from; \
> > > + size_t _count = count; \
> > > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > > + \
> > > + for (; _from < _end_from; _from += 8, _to += 8) \
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > > + if ((_count % 8) >= 4) {
> >
> > If (_count & 4) {
>
> That would be obfuscating, IMHO. The compiler doesn't need such things
> to generate optimal code.
Try it: https://godbolt.org/z/EvvGrTxv3
And it isn't that obfuscated - no more so than your version.
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > > + })
> >
> > But that looks bit a bit large to be inlined.
>
> You trimmed alot, this #define is in a C file and it is a template to
> generate the 32 and 64 bit out of line functions. Things are done like
> this because the 32/64 version are exactly the same logic except just
> with different types and sizes.
I missed that in a quick read at 11pm :-(
Although I doubt that generating long TLP from byte writes is
really necessary.
IIRC you were merging at most 4 writes.
So better to do a single 32bit write instead.
(Unless you have misaligned source data - unlikely.)
While write-combining to generate long TLP is probably mostly
safe for PCIe targets, there are some that will only handle
TLP for single 32bit data items.
Which might be why the code is explicitly requesting 4 byte copies.
So it may be entirely wrong to write-combine anything except
the generic memcpy_toio().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 9:07 ` David Laight
@ 2024-02-23 11:01 ` Niklas Schnelle
2024-02-23 11:05 ` David Laight
2024-02-23 11:38 ` Niklas Schnelle
1 sibling, 1 reply; 31+ messages in thread
From: Niklas Schnelle @ 2024-02-23 11:01 UTC (permalink / raw
To: David Laight, 'Jason Gunthorpe'
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Will Deacon
On Fri, 2024-02-23 at 09:07 +0000, David Laight wrote:
> From: Jason Gunthorpe
> > Sent: 22 February 2024 22:36
> > To: David Laight <David.Laight@ACULAB.COM>
> >
> > On Thu, Feb 22, 2024 at 10:05:04PM +0000, David Laight wrote:
> > > From: Jason Gunthorpe
> > > > Sent: 21 February 2024 01:17
> > > >
> > > > The kernel provides driver support for using write combining IO memory
> > > > through the __iowriteXX_copy() API which is commonly used as an optional
> > > > optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
> > > >
> > > ...
> > > > Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> > > > assembly to build consecutive blocks of STR instructions. Provide direct
> > > > support for 64/32/16 large TLP generation in this manner. Optimize for
> > > > common constant lengths so that the compiler can directly inline the store
> > > > blocks.
> > > ...
> > > > +/*
> > > > + * This generates a memcpy that works on a from/to address which is aligned to
> > > > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > > > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > > > + */
> > > > +#define memcpy_toio_aligned(to, from, count, bits) \
> > > > + ({ \
> > > > + volatile u##bits __iomem *_to = to; \
> > > > + const u##bits *_from = from; \
> > > > + size_t _count = count; \
> > > > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > > > + \
> > > > + for (; _from < _end_from; _from += 8, _to += 8) \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > > > + if ((_count % 8) >= 4) {
> > >
> > > If (_count & 4) {
> >
> > That would be obfuscating, IMHO. The compiler doesn't need such things
> > to generate optimal code.
>
> Try it: https://godbolt.org/z/EvvGrTxv3
> And it isn't that obfuscated - no more so than your version.
The godbolt link does "n % 8 > 4" instead of "... >= 4" as in Jason's
original code. With ">=" the compiled code matches that for "n & 4".
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 11:01 ` Niklas Schnelle
@ 2024-02-23 11:05 ` David Laight
2024-02-23 12:53 ` Jason Gunthorpe
0 siblings, 1 reply; 31+ messages in thread
From: David Laight @ 2024-02-23 11:05 UTC (permalink / raw
To: 'Niklas Schnelle', 'Jason Gunthorpe'
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Will Deacon
...
> > > > > + if ((_count % 8) >= 4) {
> > > >
> > > > If (_count & 4) {
> > >
> > > That would be obfuscating, IMHO. The compiler doesn't need such things
> > > to generate optimal code.
> >
> > Try it: https://godbolt.org/z/EvvGrTxv3
> > And it isn't that obfuscated - no more so than your version.
>
> The godbolt link does "n % 8 > 4" instead of "... >= 4" as in Jason's
> original code. With ">=" the compiled code matches that for "n & 4".
Bugger :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 9:07 ` David Laight
2024-02-23 11:01 ` Niklas Schnelle
@ 2024-02-23 11:38 ` Niklas Schnelle
2024-02-23 12:19 ` David Laight
2024-02-23 12:58 ` Jason Gunthorpe
1 sibling, 2 replies; 31+ messages in thread
From: Niklas Schnelle @ 2024-02-23 11:38 UTC (permalink / raw
To: David Laight, 'Jason Gunthorpe'
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Will Deacon
On Fri, 2024-02-23 at 09:07 +0000, David Laight wrote:
> From: Jason Gunthorpe
> > Sent: 22 February 2024 22:36
> > To: David Laight <David.Laight@ACULAB.COM>
> >
> > On Thu, Feb 22, 2024 at 10:05:04PM +0000, David Laight wrote:
> > > From: Jason Gunthorpe
> > > > Sent: 21 February 2024 01:17
> > > >
> > > > The kernel provides driver support for using write combining IO memory
> > > > through the __iowriteXX_copy() API which is commonly used as an optional
> > > > optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
> > > >
> > > ...
> > > > Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> > > > assembly to build consecutive blocks of STR instructions. Provide direct
> > > > support for 64/32/16 large TLP generation in this manner. Optimize for
> > > > common constant lengths so that the compiler can directly inline the store
> > > > blocks.
> > > ...
> > > > +/*
> > > > + * This generates a memcpy that works on a from/to address which is aligned to
> > > > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > > > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > > > + */
> > > > +#define memcpy_toio_aligned(to, from, count, bits) \
> > > > + ({ \
> > > > + volatile u##bits __iomem *_to = to; \
> > > > + const u##bits *_from = from; \
> > > > + size_t _count = count; \
> > > > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > > > + \
> > > > + for (; _from < _end_from; _from += 8, _to += 8) \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > > > + if ((_count % 8) >= 4) {
> > >
> > > If (_count & 4) {
> >
> > That would be obfuscating, IMHO. The compiler doesn't need such things
> > to generate optimal code.
>
> Try it: https://godbolt.org/z/EvvGrTxv3
> And it isn't that obfuscated - no more so than your version.
>
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > > > + })
> > >
> > > But that looks bit a bit large to be inlined.
> >
> > You trimmed alot, this #define is in a C file and it is a template to
> > generate the 32 and 64 bit out of line functions. Things are done like
> > this because the 32/64 version are exactly the same logic except just
> > with different types and sizes.
>
> I missed that in a quick read at 11pm :-(
>
> Although I doubt that generating long TLP from byte writes is
> really necessary.
I might have gotten confused but I think these are not byte writes.
Remember that the count is in terms of the number of bits sized
quantities to copy so "count == 1" is 4/8 bytes here.
> IIRC you were merging at most 4 writes.
> So better to do a single 32bit write instead.
> (Unless you have misaligned source data - unlikely.)
>
> While write-combining to generate long TLP is probably mostly
> safe for PCIe targets, there are some that will only handle
> TLP for single 32bit data items.
> Which might be why the code is explicitly requesting 4 byte copies.
> So it may be entirely wrong to write-combine anything except
> the generic memcpy_toio().
>
> David
On anything other than s390x this should only do write-combine if the
memory mapping allows it, no? Meaning a driver that can't handle larger
TLPs really shouldn't use ioremap_wc() then.
On s390x one could argue that our version of __iowriteXX_copy() is
strictly speaking not correct in that zpci_memcpy_toio() doesn't really
use XX bit writes which is why for us memcpy_toio() was actually a
better fit indeed. On the other hand doing 32 bit PCI stores (an s390x
thing) can't combine multiple stores into a single TLP which these
functions are used for and which has much more use cases than forcing a
copy loop with 32/64 bit sized writes which would also be a lot slower
on s390x than an aligned zpci_memcpy_toio().
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 11:38 ` Niklas Schnelle
@ 2024-02-23 12:19 ` David Laight
2024-02-23 13:03 ` Jason Gunthorpe
2024-02-23 12:58 ` Jason Gunthorpe
1 sibling, 1 reply; 31+ messages in thread
From: David Laight @ 2024-02-23 12:19 UTC (permalink / raw
To: 'Niklas Schnelle', 'Jason Gunthorpe'
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky,
linux-rdma@vger.kernel.org, linux-s390@vger.kernel.org,
llvm@lists.linux.dev, Ingo Molnar, Bill Wendling,
Nathan Chancellor, Nick Desaulniers, netdev@vger.kernel.org,
Paolo Abeni, Salil Mehta, Jijie Shao, Sven Schnelle,
Thomas Gleixner, x86@kernel.org, Yisen Zhuang, Arnd Bergmann,
Catalin Marinas, Leon Romanovsky, linux-arch@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland,
Michael Guralnik, patches@lists.linux.dev, Will Deacon
From: Niklas Schnelle
> Sent: 23 February 2024 11:38
...
> > Although I doubt that generating long TLP from byte writes is
> > really necessary.
>
> I might have gotten confused but I think these are not byte writes.
> Remember that the count is in terms of the number of bits sized
> quantities to copy so "count == 1" is 4/8 bytes here.
Something made me think you were generating a byte version
as well as the 32 and 64 bit ones.
...
> > While write-combining to generate long TLP is probably mostly
> > safe for PCIe targets, there are some that will only handle
> > TLP for single 32bit data items.
> > Which might be why the code is explicitly requesting 4 byte copies.
> > So it may be entirely wrong to write-combine anything except
> > the generic memcpy_toio().
> >
> > David
>
> On anything other than s390x this should only do write-combine if the
> memory mapping allows it, no? Meaning a driver that can't handle larger
> TLPs really shouldn't use ioremap_wc() then.
I can't decide whether merged writes could be required for some
target addresses but be problematic on others.
Probably not.
> On s390x one could argue that our version of __iowriteXX_copy() is
> strictly speaking not correct in that zpci_memcpy_toio() doesn't really
> use XX bit writes which is why for us memcpy_toio() was actually a
> better fit indeed. On the other hand doing 32 bit PCI stores (an s390x
> thing) can't combine multiple stores into a single TLP which these
> functions are used for and which has much more use cases than forcing a
> copy loop with 32/64 bit sized writes which would also be a lot slower
> on s390x than an aligned zpci_memcpy_toio().
If I read that correctly 32bit writes don't get merged?
Indeed any code that will benefit from merging can (probably)
do 64bit writes so is even attempting to merge 32bit ones
worth the effort?
Since writes get 'posted' all over the place.
How many writes do you need to do before write-combining makes a difference?
We've logic in our fpga to trace the RX and TX TLP [1].
Although the link is slow; back to back writes are limited by
what happens later in the fpga logic - not the pcie link.
Reads are another matter entirely.
The x86 cpu I've used assign a tag to each cpu core.
So while reads from multiple processes happen in parallel, those
from a single process are definitely synchronous.
The cpu stalls for a few thousand clock on every read.
Large read TLPs (and overlapped read TLPs) would have a much
bigger effect than large write TLP.
[1] It is nice to be able to see what is going on without having
to beg/steal/borrow an expensive PCIe analyser and persuade the
hardware to work with it connected.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 11:05 ` David Laight
@ 2024-02-23 12:53 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-23 12:53 UTC (permalink / raw
To: David Laight
Cc: 'Niklas Schnelle', Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, Feb 23, 2024 at 11:05:29AM +0000, David Laight wrote:
> ...
> > > > > > + if ((_count % 8) >= 4) {
> > > > >
> > > > > If (_count & 4) {
> > > >
> > > > That would be obfuscating, IMHO. The compiler doesn't need such things
> > > > to generate optimal code.
> > >
> > > Try it: https://godbolt.org/z/EvvGrTxv3
> > > And it isn't that obfuscated - no more so than your version.
> >
> > The godbolt link does "n % 8 > 4" instead of "... >= 4" as in Jason's
> > original code. With ">=" the compiled code matches that for "n & 4".
>
> Bugger :-)
Yes, I already fine tuned things to get good codegen.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 11:38 ` Niklas Schnelle
2024-02-23 12:19 ` David Laight
@ 2024-02-23 12:58 ` Jason Gunthorpe
2024-02-23 16:35 ` Niklas Schnelle
1 sibling, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-23 12:58 UTC (permalink / raw
To: Niklas Schnelle
Cc: David Laight, Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, Feb 23, 2024 at 12:38:18PM +0100, Niklas Schnelle wrote:
> > Although I doubt that generating long TLP from byte writes is
> > really necessary.
>
> I might have gotten confused but I think these are not byte writes.
> Remember that the count is in terms of the number of bits sized
> quantities to copy so "count == 1" is 4/8 bytes here.
Right.
There seem to be two callers of this API in the kernel, one is calling
with a constant size and wants a large TLP
Another seems to want memcpy_to_io with a guarenteed 32/64 bit store.
> > IIRC you were merging at most 4 writes.
> > So better to do a single 32bit write instead.
> > (Unless you have misaligned source data - unlikely.)
> >
> > While write-combining to generate long TLP is probably mostly
> > safe for PCIe targets, there are some that will only handle
> > TLP for single 32bit data items.
> > Which might be why the code is explicitly requesting 4 byte copies.
> > So it may be entirely wrong to write-combine anything except
> > the generic memcpy_toio().
>
> On anything other than s390x this should only do write-combine if the
> memory mapping allows it, no? Meaning a driver that can't handle larger
> TLPs really shouldn't use ioremap_wc() then.
Right.
> On s390x one could argue that our version of __iowriteXX_copy() is
> strictly speaking not correct in that zpci_memcpy_toio() doesn't really
> use XX bit writes which is why for us memcpy_toio() was actually a
> better fit indeed. On the other hand doing 32 bit PCI stores (an s390x
> thing) can't combine multiple stores into a single TLP which these
> functions are used for and which has much more use cases than forcing a
> copy loop with 32/64 bit sized writes which would also be a lot slower
> on s390x than an aligned zpci_memcpy_toio().
mlx5 will definitely not work right if __iowrite64_copy() results in
anything smaller than 32/64 bit PCIe TLPs.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 12:19 ` David Laight
@ 2024-02-23 13:03 ` Jason Gunthorpe
2024-02-23 13:52 ` David Laight
0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-23 13:03 UTC (permalink / raw
To: David Laight
Cc: 'Niklas Schnelle', Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, Feb 23, 2024 at 12:19:24PM +0000, David Laight wrote:
> Since writes get 'posted' all over the place.
> How many writes do you need to do before write-combining makes a
> difference?
The issue is that the HW can optimize if the entire transaction is
presented in one TLP, if it has to reassemble the transaction it takes
a big slow path hit.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 13:03 ` Jason Gunthorpe
@ 2024-02-23 13:52 ` David Laight
2024-02-23 14:44 ` Jason Gunthorpe
0 siblings, 1 reply; 31+ messages in thread
From: David Laight @ 2024-02-23 13:52 UTC (permalink / raw
To: 'Jason Gunthorpe'
Cc: 'Niklas Schnelle', Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
From: Jason Gunthorpe
> Sent: 23 February 2024 13:03
>
> On Fri, Feb 23, 2024 at 12:19:24PM +0000, David Laight wrote:
>
> > Since writes get 'posted' all over the place.
> > How many writes do you need to do before write-combining makes a
> > difference?
>
> The issue is that the HW can optimize if the entire transaction is
> presented in one TLP, if it has to reassemble the transaction it takes
> a big slow path hit.
Ah, so you aren't optimising to reduce the number of TLP for
(effectively) a write to a memory buffer, but have a pcie slave
that really want to see (for example) the writes for a ring buffer
entry in a single TLP?
So you really want something that (should) generate a 16 (or 32)
byte TLP? Rather than abusing the function that is expected to
generate multiple 8 byte TLP to generate larger TLP.
I'm guessing that on arm64 the ldp/stp instructions will generate
a single 16 byte TLP regardless of write combining?
They would definitely help memcpy_fromio().
Are they enough for arm64?
Getting but TLP on x86 is probably harder.
(Unless you use AVX512 registers and aligned accesses.)
It is rather a shame that there isn't an efficient way to get
access to a couple of large SIMD registers.
(eg save on stack and have the fpu code where they are for
a lazy fpu switch.)
There is quite a bit of code that would benefit, but kernel_fpu_begin()
is just too expensive.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 13:52 ` David Laight
@ 2024-02-23 14:44 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-23 14:44 UTC (permalink / raw
To: David Laight
Cc: 'Niklas Schnelle', Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, Feb 23, 2024 at 01:52:37PM +0000, David Laight wrote:
> > > Since writes get 'posted' all over the place.
> > > How many writes do you need to do before write-combining makes a
> > > difference?
> >
> > The issue is that the HW can optimize if the entire transaction is
> > presented in one TLP, if it has to reassemble the transaction it takes
> > a big slow path hit.
>
> Ah, so you aren't optimising to reduce the number of TLP for
> (effectively) a write to a memory buffer, but have a pcie slave
> that really want to see (for example) the writes for a ring buffer
> entry in a single TLP?
>
> So you really want something that (should) generate a 16 (or 32)
> byte TLP? Rather than abusing the function that is expected to
> generate multiple 8 byte TLP to generate larger TLP.
__iowriteXX_copy() was originally created by Pathscale (an RDMA device
company) to support RDMA drivers doing exactly this workload. It is
not an abuse.
> It is rather a shame that there isn't an efficient way to get
> access to a couple of large SIMD registers.
Yes, userspace uses SIMD to make this work alot better and run faster.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 12:58 ` Jason Gunthorpe
@ 2024-02-23 16:35 ` Niklas Schnelle
2024-02-23 17:05 ` Jason Gunthorpe
0 siblings, 1 reply; 31+ messages in thread
From: Niklas Schnelle @ 2024-02-23 16:35 UTC (permalink / raw
To: Jason Gunthorpe
Cc: David Laight, Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, 2024-02-23 at 08:58 -0400, Jason Gunthorpe wrote:
> On Fri, Feb 23, 2024 at 12:38:18PM +0100, Niklas Schnelle wrote:
> > > Although I doubt that generating long TLP from byte writes is
> > > really necessary.
> >
> > I might have gotten confused but I think these are not byte writes.
> > Remember that the count is in terms of the number of bits sized
> > quantities to copy so "count == 1" is 4/8 bytes here.
>
> Right.
>
> There seem to be two callers of this API in the kernel, one is calling
> with a constant size and wants a large TLP
>
> Another seems to want memcpy_to_io with a guarenteed 32/64 bit store.
I don't really understand how that works together with the order not
being guaranteed. Do they use normal ioremap() and then require 32/64
bit TLPs and don't care about the order? But then the generic and ARM
variants do things in order so who knows if they actually rely on that.
>
> > > IIRC you were merging at most 4 writes.
> > > So better to do a single 32bit write instead.
> > > (Unless you have misaligned source data - unlikely.)
> > >
> > > While write-combining to generate long TLP is probably mostly
> > > safe for PCIe targets, there are some that will only handle
> > > TLP for single 32bit data items.
> > > Which might be why the code is explicitly requesting 4 byte copies.
> > > So it may be entirely wrong to write-combine anything except
> > > the generic memcpy_toio().
> >
> > On anything other than s390x this should only do write-combine if the
> > memory mapping allows it, no? Meaning a driver that can't handle larger
> > TLPs really shouldn't use ioremap_wc() then.
>
> Right.
>
> > On s390x one could argue that our version of __iowriteXX_copy() is
> > strictly speaking not correct in that zpci_memcpy_toio() doesn't really
> > use XX bit writes which is why for us memcpy_toio() was actually a
> > better fit indeed. On the other hand doing 32 bit PCI stores (an s390x
> > thing) can't combine multiple stores into a single TLP which these
> > functions are used for and which has much more use cases than forcing a
> > copy loop with 32/64 bit sized writes which would also be a lot slower
> > on s390x than an aligned zpci_memcpy_toio().
>
> mlx5 will definitely not work right if __iowrite64_copy() results in
> anything smaller than 32/64 bit PCIe TLPs.
>
> Jason
Yes and we do actually have mlx5 on s390x so this is my priority.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-23 16:35 ` Niklas Schnelle
@ 2024-02-23 17:05 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-23 17:05 UTC (permalink / raw
To: Niklas Schnelle
Cc: David Laight, Alexander Gordeev, Andrew Morton,
Christian Borntraeger, Borislav Petkov, Dave Hansen,
David S. Miller, Eric Dumazet, Gerald Schaefer, Vasily Gorbik,
Heiko Carstens, H. Peter Anvin, Justin Stitt, Jakub Kicinski,
Leon Romanovsky, linux-rdma@vger.kernel.org,
linux-s390@vger.kernel.org, llvm@lists.linux.dev, Ingo Molnar,
Bill Wendling, Nathan Chancellor, Nick Desaulniers,
netdev@vger.kernel.org, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86@kernel.org, Yisen Zhuang,
Arnd Bergmann, Catalin Marinas, Leon Romanovsky,
linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Mark Rutland, Michael Guralnik, patches@lists.linux.dev,
Will Deacon
On Fri, Feb 23, 2024 at 05:35:42PM +0100, Niklas Schnelle wrote:
> On Fri, 2024-02-23 at 08:58 -0400, Jason Gunthorpe wrote:
> > On Fri, Feb 23, 2024 at 12:38:18PM +0100, Niklas Schnelle wrote:
> > > > Although I doubt that generating long TLP from byte writes is
> > > > really necessary.
> > >
> > > I might have gotten confused but I think these are not byte writes.
> > > Remember that the count is in terms of the number of bits sized
> > > quantities to copy so "count == 1" is 4/8 bytes here.
> >
> > Right.
> >
> > There seem to be two callers of this API in the kernel, one is calling
> > with a constant size and wants a large TLP
> >
> > Another seems to want memcpy_to_io with a guarenteed 32/64 bit store.
>
> I don't really understand how that works together with the order not
> being guaranteed. Do they use normal ioremap() and then require 32/64
> bit TLPs and don't care about the order?
Yes, I assume so. From my impression the cases looked like they were
copying to MMIO memory so order probably doesn't matter.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
2024-02-21 19:22 ` Will Deacon
2024-02-22 22:05 ` David Laight
@ 2024-02-27 10:37 ` Catalin Marinas
2024-02-28 23:06 ` Jason Gunthorpe
2024-02-29 10:33 ` Catalin Marinas
2024-03-01 18:52 ` Catalin Marinas
4 siblings, 1 reply; 31+ messages in thread
From: Catalin Marinas @ 2024-02-27 10:37 UTC (permalink / raw
To: Jason Gunthorpe
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> +/*
> + * This generates a memcpy that works on a from/to address which is aligned to
> + * bits. Count is in terms of the number of bits sized quantities to copy. It
> + * optimizes to use the STR groupings when possible so that it is WC friendly.
> + */
> +#define memcpy_toio_aligned(to, from, count, bits) \
> + ({ \
> + volatile u##bits __iomem *_to = to; \
> + const u##bits *_from = from; \
> + size_t _count = count; \
> + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> + \
> + for (; _from < _end_from; _from += 8, _to += 8) \
> + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> + if ((_count % 8) >= 4) { \
> + __const_memcpy_toio_aligned##bits(_to, _from, 4); \
> + _from += 4; \
> + _to += 4; \
> + } \
> + if ((_count % 4) >= 2) { \
> + __const_memcpy_toio_aligned##bits(_to, _from, 2); \
> + _from += 2; \
> + _to += 2; \
> + } \
> + if (_count % 2) \
> + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> + })
Do we actually need all this if count is not constant? If it's not
performance critical anywhere, I'd rather copy the generic
implementation, it's easier to read.
Otherwise, apart from the __raw_writeq() typo that Will mentioned, the
patch looks fine to me.
--
Catalin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-27 10:37 ` Catalin Marinas
@ 2024-02-28 23:06 ` Jason Gunthorpe
2024-02-29 10:24 ` Catalin Marinas
0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-28 23:06 UTC (permalink / raw
To: Catalin Marinas
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Tue, Feb 27, 2024 at 10:37:18AM +0000, Catalin Marinas wrote:
> On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > +/*
> > + * This generates a memcpy that works on a from/to address which is aligned to
> > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > + */
> > +#define memcpy_toio_aligned(to, from, count, bits) \
> > + ({ \
> > + volatile u##bits __iomem *_to = to; \
> > + const u##bits *_from = from; \
> > + size_t _count = count; \
> > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > + \
> > + for (; _from < _end_from; _from += 8, _to += 8) \
> > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > + if ((_count % 8) >= 4) { \
> > + __const_memcpy_toio_aligned##bits(_to, _from, 4); \
> > + _from += 4; \
> > + _to += 4; \
> > + } \
> > + if ((_count % 4) >= 2) { \
> > + __const_memcpy_toio_aligned##bits(_to, _from, 2); \
> > + _from += 2; \
> > + _to += 2; \
> > + } \
> > + if (_count % 2) \
> > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > + })
>
> Do we actually need all this if count is not constant? If it's not
> performance critical anywhere, I'd rather copy the generic
> implementation, it's easier to read.
Which generic version?
The point is to maximize WC effects with non-constant values, so I
think we do need something like this. ie we can't just fall back to
looping over 64 bit stores one at a time.
If we don't use the large block stores we know we get very poor WC
behavior. So at least the 8 and 4 constant value sections are
needed. At that point you may as well just do 4 and 2 instead of
another loop.
Most places I know about using this are performance paths, the entire
iocopy infrastructure was introduced as an x86 performance
optimization..
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-28 23:06 ` Jason Gunthorpe
@ 2024-02-29 10:24 ` Catalin Marinas
2024-02-29 13:28 ` Jason Gunthorpe
0 siblings, 1 reply; 31+ messages in thread
From: Catalin Marinas @ 2024-02-29 10:24 UTC (permalink / raw
To: Jason Gunthorpe
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Wed, Feb 28, 2024 at 07:06:16PM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 27, 2024 at 10:37:18AM +0000, Catalin Marinas wrote:
> > On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > > +/*
> > > + * This generates a memcpy that works on a from/to address which is aligned to
> > > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > > + */
> > > +#define memcpy_toio_aligned(to, from, count, bits) \
> > > + ({ \
> > > + volatile u##bits __iomem *_to = to; \
> > > + const u##bits *_from = from; \
> > > + size_t _count = count; \
> > > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > > + \
> > > + for (; _from < _end_from; _from += 8, _to += 8) \
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > > + if ((_count % 8) >= 4) { \
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 4); \
> > > + _from += 4; \
> > > + _to += 4; \
> > > + } \
> > > + if ((_count % 4) >= 2) { \
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 2); \
> > > + _from += 2; \
> > > + _to += 2; \
> > > + } \
> > > + if (_count % 2) \
> > > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > > + })
> >
> > Do we actually need all this if count is not constant? If it's not
> > performance critical anywhere, I'd rather copy the generic
> > implementation, it's easier to read.
>
> Which generic version?
The current __iowriteXX_copy() in lib/iomap_copy.c (copy them over or
add some preprocessor reuse the generic functions).
> The point is to maximize WC effects with non-constant values, so I
> think we do need something like this. ie we can't just fall back to
> looping over 64 bit stores one at a time.
If that's a case you are also targeting and have seen it in practice,
that's fine. But I had the impression that you are mostly after the
constant count case which is already addressed by the other part of this
patch. For the non-constant case, we have a DGH only at the end of
whatever buffer was copied rather than after every 64-byte increments
you'd get for a count of 8.
> Most places I know about using this are performance paths, the entire
> iocopy infrastructure was introduced as an x86 performance
> optimization..
At least the x86 case makes sense even from a maintenance perspective,
it's just a much simpler "rep movsl". I just want to make sure we don't
over-complicate this code on arm64 unnecessarily.
--
Catalin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
` (2 preceding siblings ...)
2024-02-27 10:37 ` Catalin Marinas
@ 2024-02-29 10:33 ` Catalin Marinas
2024-02-29 13:29 ` Jason Gunthorpe
2024-03-01 18:52 ` Catalin Marinas
4 siblings, 1 reply; 31+ messages in thread
From: Catalin Marinas @ 2024-02-29 10:33 UTC (permalink / raw
To: Jason Gunthorpe
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> + const u32 *from, size_t count)
> +{
> + switch (count) {
> + case 8:
> + asm volatile("str %w0, [%8, #4 * 0]\n"
> + "str %w1, [%8, #4 * 1]\n"
> + "str %w2, [%8, #4 * 2]\n"
> + "str %w3, [%8, #4 * 3]\n"
> + "str %w4, [%8, #4 * 4]\n"
> + "str %w5, [%8, #4 * 5]\n"
> + "str %w6, [%8, #4 * 6]\n"
> + "str %w7, [%8, #4 * 7]\n"
> + :
> + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> + "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
> + "rZ"(from[6]), "rZ"(from[7]), "r"(to));
> + break;
BTW, talking of maintenance, would a series of __raw_writel() with
Mark's recent patch for offset addressing generate similar code? I.e.:
__raw_writel(from[0], to);
__raw_writel(from[1], to + 1);
...
__raw_writel(from[7], to + 7);
(you may have mentioned it in previous threads, I did not check)
--
Catalin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-29 10:24 ` Catalin Marinas
@ 2024-02-29 13:28 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-29 13:28 UTC (permalink / raw
To: Catalin Marinas
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Thu, Feb 29, 2024 at 10:24:42AM +0000, Catalin Marinas wrote:
> On Wed, Feb 28, 2024 at 07:06:16PM -0400, Jason Gunthorpe wrote:
> > On Tue, Feb 27, 2024 at 10:37:18AM +0000, Catalin Marinas wrote:
> > > On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > > > +/*
> > > > + * This generates a memcpy that works on a from/to address which is aligned to
> > > > + * bits. Count is in terms of the number of bits sized quantities to copy. It
> > > > + * optimizes to use the STR groupings when possible so that it is WC friendly.
> > > > + */
> > > > +#define memcpy_toio_aligned(to, from, count, bits) \
> > > > + ({ \
> > > > + volatile u##bits __iomem *_to = to; \
> > > > + const u##bits *_from = from; \
> > > > + size_t _count = count; \
> > > > + const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
> > > > + \
> > > > + for (; _from < _end_from; _from += 8, _to += 8) \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 8); \
> > > > + if ((_count % 8) >= 4) { \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 4); \
> > > > + _from += 4; \
> > > > + _to += 4; \
> > > > + } \
> > > > + if ((_count % 4) >= 2) { \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 2); \
> > > > + _from += 2; \
> > > > + _to += 2; \
> > > > + } \
> > > > + if (_count % 2) \
> > > > + __const_memcpy_toio_aligned##bits(_to, _from, 1); \
> > > > + })
> > >
> > > Do we actually need all this if count is not constant? If it's not
> > > performance critical anywhere, I'd rather copy the generic
> > > implementation, it's easier to read.
> >
> > Which generic version?
>
> The current __iowriteXX_copy() in lib/iomap_copy.c (copy them over or
> add some preprocessor reuse the generic functions).
That just loops over 64 bit quantities - we know that doesn't work?
> > The point is to maximize WC effects with non-constant values, so I
> > think we do need something like this. ie we can't just fall back to
> > looping over 64 bit stores one at a time.
>
> If that's a case you are also targeting and have seen it in practice,
> that's fine. But I had the impression that you are mostly after the
> constant count case which is already addressed by the other part of this
> patch. For the non-constant case, we have a DGH only at the end of
> whatever buffer was copied rather than after every 64-byte increments
> you'd get for a count of 8.
mlx5 uses only the constant case. From my looking most places were
using the constant path.
However, from an API perspective, we know we need these runs of stores
for the CPU to work properly so it doesn't make any sense that the
same function called with a constant length would have good WC and the
very same function called with a variable length would have bad WC. I
would expect them to behave the same.
This is what the above does, if you pass in non-constant 64 or 32 you
get the same instruction sequence out of line as constant 64 or 32
length generates in-line. I think it is important to work like this
for basic sanity.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-29 10:33 ` Catalin Marinas
@ 2024-02-29 13:29 ` Jason Gunthorpe
0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2024-02-29 13:29 UTC (permalink / raw
To: Catalin Marinas
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Thu, Feb 29, 2024 at 10:33:04AM +0000, Catalin Marinas wrote:
> On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > + const u32 *from, size_t count)
> > +{
> > + switch (count) {
> > + case 8:
> > + asm volatile("str %w0, [%8, #4 * 0]\n"
> > + "str %w1, [%8, #4 * 1]\n"
> > + "str %w2, [%8, #4 * 2]\n"
> > + "str %w3, [%8, #4 * 3]\n"
> > + "str %w4, [%8, #4 * 4]\n"
> > + "str %w5, [%8, #4 * 5]\n"
> > + "str %w6, [%8, #4 * 6]\n"
> > + "str %w7, [%8, #4 * 7]\n"
> > + :
> > + : "rZ"(from[0]), "rZ"(from[1]), "rZ"(from[2]),
> > + "rZ"(from[3]), "rZ"(from[4]), "rZ"(from[5]),
> > + "rZ"(from[6]), "rZ"(from[7]), "r"(to));
> > + break;
>
> BTW, talking of maintenance, would a series of __raw_writel() with
> Mark's recent patch for offset addressing generate similar code? I.e.:
No
gcc intersperses reads/writes (which we were advised not to do) and
clang doesn't support the "o" directive so it produces poor
codegen.
Jason
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
` (3 preceding siblings ...)
2024-02-29 10:33 ` Catalin Marinas
@ 2024-03-01 18:52 ` Catalin Marinas
4 siblings, 0 replies; 31+ messages in thread
From: Catalin Marinas @ 2024-03-01 18:52 UTC (permalink / raw
To: Jason Gunthorpe
Cc: Alexander Gordeev, Andrew Morton, Christian Borntraeger,
Borislav Petkov, Dave Hansen, David S. Miller, Eric Dumazet,
Gerald Schaefer, Vasily Gorbik, Heiko Carstens, H. Peter Anvin,
Justin Stitt, Jakub Kicinski, Leon Romanovsky, linux-rdma,
linux-s390, llvm, Ingo Molnar, Bill Wendling, Nathan Chancellor,
Nick Desaulniers, netdev, Paolo Abeni, Salil Mehta, Jijie Shao,
Sven Schnelle, Thomas Gleixner, x86, Yisen Zhuang, Arnd Bergmann,
Leon Romanovsky, linux-arch, linux-arm-kernel, Mark Rutland,
Michael Guralnik, patches, Niklas Schnelle, Will Deacon
On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> The kernel provides driver support for using write combining IO memory
> through the __iowriteXX_copy() API which is commonly used as an optional
> optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.
>
> iomap_copy.c provides a generic implementation as a simple 4/8 byte at a
> time copy loop that has worked well with past ARM64 CPUs, giving a high
> frequency of large TLPs being successfully formed.
>
> However modern ARM64 CPUs are quite sensitive to how the write combining
> CPU HW is operated and a compiler generated loop with intermixed
> load/store is not sufficient to frequently generate a large TLP. The CPUs
> would like to see the entire TLP generated by consecutive store
> instructions from registers. Compilers like gcc tend to intermix loads and
> stores and have poor code generation, in part, due to the ARM64 situation
> that writeq() does not codegen anything other than "[xN]". However even
> with that resolved compilers like clang still do not have good code
> generation.
>
> This means on modern ARM64 CPUs the rate at which __iowriteXX_copy()
> successfully generates large TLPs is very small (less than 1 in 10,000)
> tries), to the point that the use of WC is pointless.
>
> Implement __iowrite32/64_copy() specifically for ARM64 and use inline
> assembly to build consecutive blocks of STR instructions. Provide direct
> support for 64/32/16 large TLP generation in this manner. Optimize for
> common constant lengths so that the compiler can directly inline the store
> blocks.
>
> This brings the frequency of large TLP generation up to a high level that
> is comparable with older CPU generations.
>
> As the __iowriteXX_copy() family of APIs is intended for use with WC
> incorporate the DGH hint directly into the function.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Apart from the slightly more complicated code, I don't expect it to make
things worse on any of the existing hardware.
So, with the typo fix that Will mentioned:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2024-03-01 18:52 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-21 1:17 [PATCH 0/6] Fix mlx5 write combining support on new ARM64 cores Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 1/6] x86: Stop using weak symbols for __iowrite32_copy() Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 2/6] s390: Implement __iowrite32_copy() Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 3/6] s390: Stop using weak symbols for __iowrite64_copy() Jason Gunthorpe
2024-02-21 1:17 ` [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy() Jason Gunthorpe
2024-02-21 19:22 ` Will Deacon
2024-02-21 23:28 ` Jason Gunthorpe
2024-02-22 22:05 ` David Laight
2024-02-22 22:36 ` Jason Gunthorpe
2024-02-23 9:07 ` David Laight
2024-02-23 11:01 ` Niklas Schnelle
2024-02-23 11:05 ` David Laight
2024-02-23 12:53 ` Jason Gunthorpe
2024-02-23 11:38 ` Niklas Schnelle
2024-02-23 12:19 ` David Laight
2024-02-23 13:03 ` Jason Gunthorpe
2024-02-23 13:52 ` David Laight
2024-02-23 14:44 ` Jason Gunthorpe
2024-02-23 12:58 ` Jason Gunthorpe
2024-02-23 16:35 ` Niklas Schnelle
2024-02-23 17:05 ` Jason Gunthorpe
2024-02-27 10:37 ` Catalin Marinas
2024-02-28 23:06 ` Jason Gunthorpe
2024-02-29 10:24 ` Catalin Marinas
2024-02-29 13:28 ` Jason Gunthorpe
2024-02-29 10:33 ` Catalin Marinas
2024-02-29 13:29 ` Jason Gunthorpe
2024-03-01 18:52 ` Catalin Marinas
2024-02-21 1:17 ` [PATCH 5/6] net: hns3: Remove io_stop_wc() calls after __iowrite64_copy() Jason Gunthorpe
2024-02-22 0:57 ` Jijie Shao
2024-02-21 1:17 ` [PATCH 6/6] IB/mlx5: Use __iowrite64_copy() for write combining stores Jason Gunthorpe
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 read-only IMAP folder(s) and NNTP newsgroup(s).