diff mbox series

[v2,03/13] sh: remove unaligned access for sh4a

Message ID 20210514100106.3404011-4-arnd@kernel.org (mailing list archive)
State New
Headers show
Series Unify asm/unaligned.h around struct helper | expand

Commit Message

Arnd Bergmann May 14, 2021, 10 a.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

Unlike every other architecture, sh4a uses an inline asm implementation
for get_unaligned(). I have shown that this produces better object
code than the asm-generic version. However, there are very few users of
arch/sh/ overall, and most of those seem to use sh4 rather than sh4a CPU
cores, so it seems not worth keeping the complexity in the architecture
independent code.

Change over to the generic version to allow simplifying that in a
follow-up patch.

If there are sh4a users that want the best performance, it would probably
be best to add support for the movua instruction in gcc itself, as this
would not just help get_unaligned() callers but any code that accesses
a __packed variable in user space or kernel.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/sh/include/asm/unaligned-sh4a.h | 199 ---------------------------
 arch/sh/include/asm/unaligned.h      |  13 --
 2 files changed, 212 deletions(-)
 delete mode 100644 arch/sh/include/asm/unaligned-sh4a.h
 delete mode 100644 arch/sh/include/asm/unaligned.h

Comments

John Paul Adrian Glaubitz May 14, 2021, 10:34 a.m. UTC | #1
Hi Arnd!

On 5/14/21 12:00 PM, Arnd Bergmann wrote:
> Unlike every other architecture, sh4a uses an inline asm implementation
> for get_unaligned(). I have shown that this produces better object
> code than the asm-generic version. However, there are very few users of
> arch/sh/ overall, and most of those seem to use sh4 rather than sh4a CPU
> cores, so it seems not worth keeping the complexity in the architecture
> independent code.

My Renesas SH4-Boards actually run an sh4a-Kernel, not an sh4-Kernel:

root@tirpitz:~> uname -a
Linux tirpitz 5.11.0-rc4-00012-g10c03c5bf422 #161 PREEMPT Mon Jan 18 21:10:17 CET 2021 sh4a GNU/Linux
root@tirpitz:~>

So, if this change reduces performance on sh4a, I would rather not merge it.

Adrian
Arnd Bergmann May 14, 2021, 12:22 p.m. UTC | #2
On Fri, May 14, 2021 at 12:34 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
>
> Hi Arnd!
>
> On 5/14/21 12:00 PM, Arnd Bergmann wrote:
> > Unlike every other architecture, sh4a uses an inline asm implementation
> > for get_unaligned(). I have shown that this produces better object
> > code than the asm-generic version. However, there are very few users of
> > arch/sh/ overall, and most of those seem to use sh4 rather than sh4a CPU
> > cores, so it seems not worth keeping the complexity in the architecture
> > independent code.
>
> My Renesas SH4-Boards actually run an sh4a-Kernel, not an sh4-Kernel:
>
> root@tirpitz:~> uname -a
> Linux tirpitz 5.11.0-rc4-00012-g10c03c5bf422 #161 PREEMPT Mon Jan 18 21:10:17 CET 2021 sh4a GNU/Linux
> root@tirpitz:~>
>
> So, if this change reduces performance on sh4a, I would rather not merge it.

It only makes a difference in very specific scenarios in which unaligned
accesses are done in a fast path, e.g. when forwarding network packet
at a high rate on a big-endian kernel (little-endian kernels wouldn't run into
this on IP headers). If you have a use case for this machine on which the
you can show a performance regression, I can add a patch on top to put
the optimized sh4a get_unaligned_le32() back. Dropping this patch
altogether would make the series much more complex because most of
the associated code gets removed in the end.

As I mentioned, supporting "movua" in the compiler likely has a much
larger impact on performance, as it would also help in user space, and
it should improve the networking case on little-endian kernels by replacing
the four separate byte loads/shift pairs with a movua plus a byteswap.

Not sure if there are gcc developers that have an active interest in sh4a
support any more.

      Arnd
John Paul Adrian Glaubitz May 15, 2021, 3:36 p.m. UTC | #3
Hi Arnd!

On 5/14/21 2:22 PM, Arnd Bergmann wrote:
>> My Renesas SH4-Boards actually run an sh4a-Kernel, not an sh4-Kernel:
>>
>> root@tirpitz:~> uname -a
>> Linux tirpitz 5.11.0-rc4-00012-g10c03c5bf422 #161 PREEMPT Mon Jan 18 21:10:17 CET 2021 sh4a GNU/Linux
>> root@tirpitz:~>
>>
>> So, if this change reduces performance on sh4a, I would rather not merge it.
> 
> It only makes a difference in very specific scenarios in which unaligned
> accesses are done in a fast path, e.g. when forwarding network packet
> at a high rate on a big-endian kernel (little-endian kernels wouldn't run into
> this on IP headers). If you have a use case for this machine on which the
> you can show a performance regression, I can add a patch on top to put
> the optimized sh4a get_unaligned_le32() back. Dropping this patch
> altogether would make the series much more complex because most of
> the associated code gets removed in the end.

Hmm, okay. But why does code which sits below arch/sh have to be removed anyway?

I don't fully understand why it poses any maintenance burden/

> As I mentioned, supporting "movua" in the compiler likely has a much
> larger impact on performance, as it would also help in user space, and
> it should improve the networking case on little-endian kernels by replacing
> the four separate byte loads/shift pairs with a movua plus a byteswap.

The problem is that - at least in Debian - we use the sh4 baseline while the kernel
supports both sh4 and sh4a, so we can't use any of these instructions in userland at
the moment.

Adrian
Arnd Bergmann May 15, 2021, 8:10 p.m. UTC | #4
On Sat, May 15, 2021 at 5:36 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On 5/14/21 2:22 PM, Arnd Bergmann wrote:
> >> My Renesas SH4-Boards actually run an sh4a-Kernel, not an sh4-Kernel:
> >>
> >> root@tirpitz:~> uname -a
> >> Linux tirpitz 5.11.0-rc4-00012-g10c03c5bf422 #161 PREEMPT Mon Jan 18 21:10:17 CET 2021 sh4a GNU/Linux
> >> root@tirpitz:~>
> >>
> >> So, if this change reduces performance on sh4a, I would rather not merge it.
> >
> > It only makes a difference in very specific scenarios in which unaligned
> > accesses are done in a fast path, e.g. when forwarding network packet
> > at a high rate on a big-endian kernel (little-endian kernels wouldn't run into
> > this on IP headers). If you have a use case for this machine on which the
> > you can show a performance regression, I can add a patch on top to put
> > the optimized sh4a get_unaligned_le32() back. Dropping this patch
> > altogether would make the series much more complex because most of
> > the associated code gets removed in the end.
>
> Hmm, okay. But why does code which sits below arch/sh have to be removed anyway?
>
> I don't fully understand why it poses any maintenance burden/

What  I'm removing is the part that lets architectures override the
generic version.

> > As I mentioned, supporting "movua" in the compiler likely has a much
> > larger impact on performance, as it would also help in user space, and
> > it should improve the networking case on little-endian kernels by replacing
> > the four separate byte loads/shift pairs with a movua plus a byteswap.
>
> The problem is that - at least in Debian - we use the sh4 baseline while the kernel
> supports both sh4 and sh4a, so we can't use any of these instructions in userland at
> the moment.

I tried building an sh7785lcr_defconfig with and without the patch,
and found that
the only affected files are:

- in-kernel nfs client
- crc32c/sha1/sha256 hash functions
- device probing for libata, scsi-core, scsi-disk, hid, r8168
  (should not matter after boot)
- msdos partition parsing

Any nfs client performance difference is probably not even measurable even
at gigabit ethernet speed.
I see that the hash functions are notably different, but I don't know if the
output from the new generic code is actually better or worse than the
original. If you do think this is important, please try the version from

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git
unaligned-sh4a

against the version without the last change in that series. If you can find
a relevant test case that exercises it, you may want to add a custom
implementation of the hash functions as well.

       Arnd
diff mbox series

Patch

diff --git a/arch/sh/include/asm/unaligned-sh4a.h b/arch/sh/include/asm/unaligned-sh4a.h
deleted file mode 100644
index d311f00ed530..000000000000
--- a/arch/sh/include/asm/unaligned-sh4a.h
+++ /dev/null
@@ -1,199 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __ASM_SH_UNALIGNED_SH4A_H
-#define __ASM_SH_UNALIGNED_SH4A_H
-
-/*
- * SH-4A has support for unaligned 32-bit loads, and 32-bit loads only.
- * Support for 64-bit accesses are done through shifting and masking
- * relative to the endianness. Unaligned stores are not supported by the
- * instruction encoding, so these continue to use the packed
- * struct.
- *
- * The same note as with the movli.l/movco.l pair applies here, as long
- * as the load is guaranteed to be inlined, nothing else will hook in to
- * r0 and we get the return value for free.
- *
- * NOTE: Due to the fact we require r0 encoding, care should be taken to
- * avoid mixing these heavily with other r0 consumers, such as the atomic
- * ops. Failure to adhere to this can result in the compiler running out
- * of spill registers and blowing up when building at low optimization
- * levels. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34777.
- */
-#include <linux/unaligned/packed_struct.h>
-#include <linux/types.h>
-#include <asm/byteorder.h>
-
-static inline u16 sh4a_get_unaligned_cpu16(const u8 *p)
-{
-#ifdef __LITTLE_ENDIAN
-	return p[0] | p[1] << 8;
-#else
-	return p[0] << 8 | p[1];
-#endif
-}
-
-static __always_inline u32 sh4a_get_unaligned_cpu32(const u8 *p)
-{
-	unsigned long unaligned;
-
-	__asm__ __volatile__ (
-		"movua.l	@%1, %0\n\t"
-		 : "=z" (unaligned)
-		 : "r" (p)
-	);
-
-	return unaligned;
-}
-
-/*
- * Even though movua.l supports auto-increment on the read side, it can
- * only store to r0 due to instruction encoding constraints, so just let
- * the compiler sort it out on its own.
- */
-static inline u64 sh4a_get_unaligned_cpu64(const u8 *p)
-{
-#ifdef __LITTLE_ENDIAN
-	return (u64)sh4a_get_unaligned_cpu32(p + 4) << 32 |
-		    sh4a_get_unaligned_cpu32(p);
-#else
-	return (u64)sh4a_get_unaligned_cpu32(p) << 32 |
-		    sh4a_get_unaligned_cpu32(p + 4);
-#endif
-}
-
-static inline u16 get_unaligned_le16(const void *p)
-{
-	return le16_to_cpu(sh4a_get_unaligned_cpu16(p));
-}
-
-static inline u32 get_unaligned_le32(const void *p)
-{
-	return le32_to_cpu(sh4a_get_unaligned_cpu32(p));
-}
-
-static inline u64 get_unaligned_le64(const void *p)
-{
-	return le64_to_cpu(sh4a_get_unaligned_cpu64(p));
-}
-
-static inline u16 get_unaligned_be16(const void *p)
-{
-	return be16_to_cpu(sh4a_get_unaligned_cpu16(p));
-}
-
-static inline u32 get_unaligned_be32(const void *p)
-{
-	return be32_to_cpu(sh4a_get_unaligned_cpu32(p));
-}
-
-static inline u64 get_unaligned_be64(const void *p)
-{
-	return be64_to_cpu(sh4a_get_unaligned_cpu64(p));
-}
-
-static inline void nonnative_put_le16(u16 val, u8 *p)
-{
-	*p++ = val;
-	*p++ = val >> 8;
-}
-
-static inline void nonnative_put_le32(u32 val, u8 *p)
-{
-	nonnative_put_le16(val, p);
-	nonnative_put_le16(val >> 16, p + 2);
-}
-
-static inline void nonnative_put_le64(u64 val, u8 *p)
-{
-	nonnative_put_le32(val, p);
-	nonnative_put_le32(val >> 32, p + 4);
-}
-
-static inline void nonnative_put_be16(u16 val, u8 *p)
-{
-	*p++ = val >> 8;
-	*p++ = val;
-}
-
-static inline void nonnative_put_be32(u32 val, u8 *p)
-{
-	nonnative_put_be16(val >> 16, p);
-	nonnative_put_be16(val, p + 2);
-}
-
-static inline void nonnative_put_be64(u64 val, u8 *p)
-{
-	nonnative_put_be32(val >> 32, p);
-	nonnative_put_be32(val, p + 4);
-}
-
-static inline void put_unaligned_le16(u16 val, void *p)
-{
-#ifdef __LITTLE_ENDIAN
-	__put_unaligned_cpu16(val, p);
-#else
-	nonnative_put_le16(val, p);
-#endif
-}
-
-static inline void put_unaligned_le32(u32 val, void *p)
-{
-#ifdef __LITTLE_ENDIAN
-	__put_unaligned_cpu32(val, p);
-#else
-	nonnative_put_le32(val, p);
-#endif
-}
-
-static inline void put_unaligned_le64(u64 val, void *p)
-{
-#ifdef __LITTLE_ENDIAN
-	__put_unaligned_cpu64(val, p);
-#else
-	nonnative_put_le64(val, p);
-#endif
-}
-
-static inline void put_unaligned_be16(u16 val, void *p)
-{
-#ifdef __BIG_ENDIAN
-	__put_unaligned_cpu16(val, p);
-#else
-	nonnative_put_be16(val, p);
-#endif
-}
-
-static inline void put_unaligned_be32(u32 val, void *p)
-{
-#ifdef __BIG_ENDIAN
-	__put_unaligned_cpu32(val, p);
-#else
-	nonnative_put_be32(val, p);
-#endif
-}
-
-static inline void put_unaligned_be64(u64 val, void *p)
-{
-#ifdef __BIG_ENDIAN
-	__put_unaligned_cpu64(val, p);
-#else
-	nonnative_put_be64(val, p);
-#endif
-}
-
-/*
- * While it's a bit non-obvious, even though the generic le/be wrappers
- * use the __get/put_xxx prefixing, they actually wrap in to the
- * non-prefixed get/put_xxx variants as provided above.
- */
-#include <linux/unaligned/generic.h>
-
-#ifdef __LITTLE_ENDIAN
-# define get_unaligned __get_unaligned_le
-# define put_unaligned __put_unaligned_le
-#else
-# define get_unaligned __get_unaligned_be
-# define put_unaligned __put_unaligned_be
-#endif
-
-#endif /* __ASM_SH_UNALIGNED_SH4A_H */
diff --git a/arch/sh/include/asm/unaligned.h b/arch/sh/include/asm/unaligned.h
deleted file mode 100644
index 0c92e2c73af4..000000000000
--- a/arch/sh/include/asm/unaligned.h
+++ /dev/null
@@ -1,13 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_SH_UNALIGNED_H
-#define _ASM_SH_UNALIGNED_H
-
-#ifdef CONFIG_CPU_SH4A
-/* SH-4A can handle unaligned loads in a relatively neutered fashion. */
-#include <asm/unaligned-sh4a.h>
-#else
-/* Otherwise, SH can't handle unaligned accesses. */
-#include <asm-generic/unaligned.h>
-#endif
-
-#endif /* _ASM_SH_UNALIGNED_H */