arm64: optimize memcpy_{from,to}io() and memset_io()

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Signed-off-by Joonwoo Park <[email protected]>

Acked-by Trilok Soni <[email protected]>

---
 arch/arm64/kernel/io.c | 72 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 10 deletions(-)

arch/arm64/kernel/io.c +62 -10

index 7d37ead..c0e3ab1 100644
@@ -20,18 +20,34 @@
 #include <linux/types.h>
 #include <linux/io.h>
 
+#define IO_CHECK_ALIGN(v, a) ((((unsigned long)(v)) & ((a) - 1)) == 0)
+
 /*
  * Copy data from IO memory space to "real" memory space.
  */
 void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
 {
-	unsigned char *t = to;
-	while (count) {
+	while (count && (!IO_CHECK_ALIGN(from, 8) || !IO_CHECK_ALIGN(to, 8))) {
+		*(u8 *)to = readb_relaxed(from);
+		from++;
+		to++;
 		count--;
-		*t = readb(from);
-		t++;
+	}
+
+	while (count >= 8) {
+		*(u64 *)to = readq_relaxed(from);
+		from += 8;
+		to += 8;
+		count -= 8;
+	}
+
+	while (count) {
+		*(u8 *)to = readb_relaxed(from);
 		from++;
+		to++;
+		count--;
 	}
+	__iormb();
 }
 EXPORT_SYMBOL(__memcpy_fromio);
 

@@ -40,12 +56,28 @@ EXPORT_SYMBOL(__memcpy_fromio);
  */
 void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
 {
-	const unsigned char *f = from;
+	void *p = (void __force *)from;
+
+	__iowmb();
+	while (count && (!IO_CHECK_ALIGN(p, 8) || !IO_CHECK_ALIGN(from, 8))) {
+		writeb_relaxed(*(volatile u8 *)from, p);
+		from++;
+		p++;
+		count--;
+	}
+
+	while (count >= 8) {
+		writeq_relaxed(*(volatile u64 *)from, p);
+		from += 8;
+		p += 8;
+		count -= 8;
+	}
+
 	while (count) {
+		writeb_relaxed(*(volatile u8 *)from, p);
+		from++;
+		p++;
 		count--;
-		writeb(*f, to);
-		f++;
-		to++;
 	}
 }
 EXPORT_SYMBOL(__memcpy_toio);

@@ -55,10 +87,30 @@ EXPORT_SYMBOL(__memcpy_toio);
  */
 void __memset_io(volatile void __iomem *dst, int c, size_t count)
 {
+	void *p = (void __force *)dst;
+	u64 qc = c;
+
+	qc |= qc << 8;
+	qc |= qc << 16;
+	qc |= qc << 32;
+
+	__iowmb();
+	while (count && !IO_CHECK_ALIGN(p, 8)) {
+		writeb_relaxed(c, p);
+		p++;
+		count--;
+	}
+
+	while (count >= 8) {
+		writeq_relaxed(c, p);
+		p += 8;
+		count -= 8;
+	}
+
 	while (count) {
+		writeb_relaxed(c, p);
+		p++;
 		count--;
-		writeb(c, dst);
-		dst++;
 	}
 }
 EXPORT_SYMBOL(__memset_io);

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

+ Catalin, Will

Thanks,
Joonwoo

Quoted 4 lines · Joonwoo Park

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Quoted 9 lines · Joonwoo Park

+ Catalin, Will

Thanks,
Joonwoo

Quoted 4 lines · Joonwoo Park

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Hmm, I've never really understood the use-case for memcpy_{to,from}io on ARM, so getting to the bottom of that would help in reviewing this patch.

Can you point me at the drivers which are using this for ARM please? Doing a

blind byte-by-byte copy could easily cause problems with some peripherals, so there must be an underlying assumption somewhere about how this is supposed to work.

Will

Quoted 11 lines · Will Deacon

Quoted 5 lines · Joonwoo Park

Quoted 4 lines · Joonwoo Park

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Hmm, I've never really understood the use-case for memcpy_{to,from}io on ARM, so getting to the bottom of that would help in reviewing this patch.

Can you point me at the drivers which are using this for ARM please? Doing a

Sure. This peripheral-loader.c driver now moved under drivers/soc/ so it
can be used for ARM and ARM64.
https://android.googlesource.com/kernel/msm.git/+/db34f44bcba24345d26b8a4b8137cf94d70afa73/arch/arm/mach-msm/peripheral-loader.c
static int load_segment(const struct elf32_phdr *phdr, unsigned num, struct pil_device *pil) {
<snip>

	while (count > 0) {
		int size;
		u8 __iomem *buf;
		size = min_t(size_t, IOMAP_SIZE, count);
		buf = ioremap(paddr, size);
	}
	<snip>
	memset(buf, 0, size);

<snip>

As you can see the function load_segment() does ioremap() followed by memset() and memcpy() which can cause unaligned multi-byte (maybe ARM64 traps 8byte unaligned access?) write to device memory.
Because of this I was fixing the driver to use memset_io() and memcpy_io() but the existing implementations were too slow compare to the one I'm proposing.

Quoted 3 lines

blind byte-by-byte copy could easily cause problems with some peripherals, so there must be an underlying assumption somewhere about how this is supposed to work.

Would you mind to explain more about the problem with byte-by-byte copying you're worried about?
I thought byte-by-byte copy always safe with regard to aligned access and that's the reason existing implementation does byte-by-byte copy. I can imagine there are some peripherals don't allow per-byte access. But if that is the case, should they not use memset_io() and memcpy_{from,to}io() anyway?

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Signed-off-by Joonwoo Park <[email protected]>

Acked-by Trilok Soni <[email protected]>

arch/arm64/kernel/io.c +8 -2

+		from++;
+		to++;
 		count--;
-		*t = readb(from);
-		t++;
+	}
+
+	while (count >= 8) {
+		*(u64 *)to = readq_relaxed(from);
+		from += 8;
+		to += 8;

Quoted 7 lines · Joonwoo Park

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Signed-off-by Joonwoo Park <[email protected]>

Acked-by Trilok Soni <[email protected]>

I thought about merging this but there are some issues. Comments below.

Quoted 9 lines

arch/arm64/kernel/io.c +0 -0

index 7d37ead..c0e3ab1 100644
@@ -20,18 +20,34 @@
 #include <linux/types.h>
 #include <linux/io.h>
 

arch/arm64/kernel/io.c line 23

Quoted 1 line

arch/arm64/kernel/io.c +1 -0

+#define IO_CHECK_ALIGN(v, a) ((((unsigned long)(v)) & ((a) - 1)) == 0)

Can you not use just IS_ALIGNED?

Quoted 5 lines

arch/arm64/kernel/io.c +0 -0

 /*
  * Copy data from IO memory space to "real" memory space.
  */
 void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
 {

arch/arm64/kernel/io.c line 31

Quoted 4 lines

arch/arm64/kernel/io.c +2 -2

-	unsigned char *t = to;
-	while (count) {
+	while (count && (!IO_CHECK_ALIGN(from, 8) || !IO_CHECK_ALIGN(to, 8))) {
+		*(u8 *)to = readb_relaxed(from);

We should not use the relaxed accessors here as we don't really care about endianness conversion. We just copy data from one place to another without interpreting it, so __raw_read*() is sufficient.

Quoted 16 lines

arch/arm64/kernel/io.c +13 -2

+		from++;
+		to++;
 		count--;
-		*t = readb(from);
-		t++;
+	}
+
+	while (count >= 8) {
+		*(u64 *)to = readq_relaxed(from);
+		from += 8;
+		to += 8;
+		count -= 8;
+	}
+
+	while (count) {
+		*(u8 *)to = readb_relaxed(from);

arch/arm64/kernel/io.c line 50

Quoted 5 lines

arch/arm64/kernel/io.c +3 -0

 		from++;
+		to++;
+		count--;
 	}
+	__iormb();

We don't need this barrier here. In the readl() implementation, it's use is for ordering between I/O polling and DMA buffer access.

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_fromio); · line 59

Quoted 9 lines

arch/arm64/kernel/io.c +1 -1

 }
 EXPORT_SYMBOL(__memcpy_fromio);
 

@@ -40,12 +56,28 @@ EXPORT_SYMBOL(__memcpy_fromio);
  */
 void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
 {
-	const unsigned char *f = from;
+	void *p = (void __force *)from;

Why do you need this?

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_fromio); · line 61

Quoted 2 lines

arch/arm64/kernel/io.c +2 -0

+
+	__iowmb();

Not needed here either.

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_fromio); · line 63

Quoted 2 lines

arch/arm64/kernel/io.c +2 -0

+	while (count && (!IO_CHECK_ALIGN(p, 8) || !IO_CHECK_ALIGN(from, 8))) {
+		writeb_relaxed(*(volatile u8 *)from, p);

Oh, so you copy from "from" to "from". Have you really tested this?

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_toio); · line 90

Quoted 5 lines

arch/arm64/kernel/io.c +1 -0

@@ -55,10 +87,30 @@ EXPORT_SYMBOL(__memcpy_toio);
  */
 void __memset_io(volatile void __iomem *dst, int c, size_t count)
 {
+	void *p = (void __force *)dst;

Do you need this?

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_toio); · line 97

Quoted 7 lines

arch/arm64/kernel/io.c +7 -0

+	u64 qc = c;
+
+	qc |= qc << 8;
+	qc |= qc << 16;
+	qc |= qc << 32;
+
+	__iowmb();

What's this for?

arch/arm64/kernel/io.c EXPORT_SYMBOL(__memcpy_toio); · line 99

Quoted 2 lines

arch/arm64/kernel/io.c +2 -0

+	while (count && !IO_CHECK_ALIGN(p, 8)) {
+		writeb_relaxed(c, p);

Using dst here directly here should work (__raw_writeb takes the same type as the second argument).

Signature · 2 lines

-- 
Catalin

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Joonwoo Park

arch/arm64/kernel/io.c +0 -0

index 7d37ead..c0e3ab1 100644
@@ -20,18 +20,34 @@
 #include <linux/types.h>
 #include <linux/io.h>
 

Quoted 3 lines

arch/arm64/kernel/io.c line 23

Quoted 1 line

arch/arm64/kernel/io.c +1 -0

+#define IO_CHECK_ALIGN(v, a) ((((unsigned long)(v)) & ((a) - 1)) == 0)

Can you not use just IS_ALIGNED?

Will do. I would need to cast from/to with unsigned long.

Quoted 16 lines

Quoted 16 lines

arch/arm64/kernel/io.c +13 -2

+		from++;
+		to++;
 		count--;
-		*t = readb(from);
-		t++;
+	}
+
+	while (count >= 8) {
+		*(u64 *)to = readq_relaxed(from);
+		from += 8;
+		to += 8;
+		count -= 8;
+	}
+
+	while (count) {
+		*(u8 *)to = readb_relaxed(from);

Quoted 8 lines

arch/arm64/kernel/io.c line 50

Quoted 5 lines

arch/arm64/kernel/io.c +3 -0

 		from++;
+		to++;
+		count--;
 	}
+	__iormb();

We don't need this barrier here. In the readl() implementation, it's use is for ordering between I/O polling and DMA buffer access.

The barriers here and down below are for accessing different devices in a row. I thought that's what your suggestion too.
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/123178.html

Quoted 5 lines

arch/arm64/kernel/io.c line 44

Quoted 2 lines

arch/arm64/kernel/io.c +2 -0

+	while (count && !IO_CHECK_ALIGN(p, 8)) {
+		writeb_relaxed(c, p);

Using dst here directly here should work (__raw_writeb takes the same type as the second argument).

Will update.

I'm quite not sure if barriers are not needed or not indeed. The situation I'm worried about is like 'memcpy_io(device A); memcpy_io(device B);' which I think memcpy_io() needs to guarantee the order.

Quoted 10 lines · Joonwoo Park

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Joonwoo Park

arch/arm64/kernel/io.c +0 -0

index 7d37ead..c0e3ab1 100644
@@ -20,18 +20,34 @@
 #include <linux/types.h>
 #include <linux/io.h>
 

Quoted 5 lines

Quoted 3 lines

arch/arm64/kernel/io.c line 23

Quoted 1 line

arch/arm64/kernel/io.c +1 -0

+#define IO_CHECK_ALIGN(v, a) ((((unsigned long)(v)) & ((a) - 1)) == 0)

Can you not use just IS_ALIGNED?

Will do. I would need to cast from/to with unsigned long.

Or define IO_CHECK_ALIGN as IS_ALIGNED((unsigned long)v, a)

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

arch/arm64/kernel/io.c +13 -2

+		from++;
+		to++;
 		count--;
-		*t = readb(from);
-		t++;
+	}
+
+	while (count >= 8) {
+		*(u64 *)to = readq_relaxed(from);
+		from += 8;
+		to += 8;
+		count -= 8;
+	}
+
+	while (count) {
+		*(u8 *)to = readb_relaxed(from);

Quoted 8 lines

arch/arm64/kernel/io.c line 50

Quoted 5 lines

arch/arm64/kernel/io.c +3 -0

 		from++;
+		to++;
+		count--;
 	}
+	__iormb();

We don't need this barrier here. In the readl() implementation, it's use is for ordering between I/O polling and DMA buffer access.

Quoted 3 lines

The barriers here and down below are for accessing different devices in a row. I thought that's what your suggestion too.
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/123178.html

I think we should leave them out until we find a use case. I currently don't see any (writel/readl etc. still have the barriers).

Quoted 6 lines

Quoted 5 lines

arch/arm64/kernel/io.c line 44

Quoted 2 lines

arch/arm64/kernel/io.c +2 -0

+	while (count && !IO_CHECK_ALIGN(p, 8)) {
+		writeb_relaxed(c, p);

Using dst here directly here should work (__raw_writeb takes the same type as the second argument).

Quoted 6 lines

Will update.

I'm quite not sure if barriers are not needed or not indeed. The situation I'm worried about is like 'memcpy_io(device A); memcpy_io(device B);' which I think memcpy_io() needs to guarantee the order.

Without barriers, ordering between device A and B would not be guaranteed. But do you have a scenario where this ordering actually matters? Most case we have something like:

memcpy_io(device A); // mapped as Device or Normal NonCacheable
writel(device B); // or an I/O register of device A

Here writel() has the correct barrier.

Signature · 2 lines

-- 
Catalin

From 1cb0bb81e0d399255e679337929a8438946a4b9a Mon Sep 17 00:00:00 2001

From 1cb0bb81e0d399255e679337929a8438946a4b9a Mon Sep 17 00:00:00 2001
From: Joonwoo Park <[email protected]>
Date: Mon, 28 Jul 2014 17:32:06 -0700
Subject: [PATCH v2] arm64: optimize memcpy_{from,to}io() and memset_io()

Optimize memcpy_{from,to}io() and memset_io() by transferring in 64 bit as much as possible with minimized barrier usage. This simplest optimization brings faster throughput compare to current byte-by-byte read and write with barrier in the loop. Code's skeleton is taken from the powerpc.

Reviewed-by Trilok Soni <[email protected]>

Signed-off-by Joonwoo Park <[email protected]>