Message-ID: <Zd4y5llkvTfKHf6b@ghost>
Date: Tue, 27 Feb 2024 11:07:18 -0800
From: Charlie Jenkins <charlie@...osinc.com>
To: Evan Green <evan@...osinc.com>
Cc: Conor Dooley <conor.dooley@...rochip.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Jisheng Zhang <jszhang@...nel.org>,
	Clément Léger <cleger@...osinc.com>,
	Eric Biggers <ebiggers@...nel.org>,
	Elliot Berman <quic_eberman@...cinc.com>,
	Charles Lohr <lohr85@...il.com>, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] riscv: Set unalignment speed at compile time

On Tue, Feb 27, 2024 at 10:48:39AM -0800, Evan Green wrote:
> On Tue, Feb 27, 2024 at 10:17 AM Charlie Jenkins <charlie@...osinc.com> wrote:
> >
> > On Tue, Feb 27, 2024 at 11:39:25AM +0000, Conor Dooley wrote:
> > > On Fri, Feb 16, 2024 at 12:33:19PM -0800, Charlie Jenkins wrote:
> > > > Introduce Kconfig options to set the kernel unaligned access support.
> > > > These options provide a non-portable alternative to the runtime
> > > > unaligned access probe.
> > > >
> > > > To support this, the unaligned access probing code is moved into it's
> > > > own file and gated behind a new RISCV_PROBE_UNALIGNED_ACCESS_SUPPORT
> > > > option.
> > > >
> > > > Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
> > > > ---
> > > >  arch/riscv/Kconfig                          |  58 +++++-
> > > >  arch/riscv/include/asm/cpufeature.h         |  30 +++-
> > > >  arch/riscv/kernel/Makefile                  |   6 +-
> > > >  arch/riscv/kernel/cpufeature.c              | 255 --------------------------
> > > >  arch/riscv/kernel/misaligned_access_speed.c | 265 ++++++++++++++++++++++++++++
> > > >  arch/riscv/kernel/probe_emulated_access.c   |  64 +++++++
> > > >  arch/riscv/kernel/sys_hwprobe.c             |  25 +++
> > > >  arch/riscv/kernel/traps_misaligned.c        |  54 +-----
> > > >  8 files changed, 442 insertions(+), 315 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index bffbd869a068..3cf700adc43b 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -690,25 +690,71 @@ config THREAD_SIZE_ORDER
> > > >  config RISCV_MISALIGNED
> > >
> > >
> > > Why can we not make up our minds on what to call this? The majority of
> > > users are "unaligned" but the file you add and this config option are
> > > "misaligned."
> >
> > We have both everywhere, maybe we (I?) should go in and standardize the
> > wording everywhere. I personally prefer "misaligned" which means
> > "incorrectly aligned" over "unaligned" which means "not aligned" because
> > a 7-bit alignment is still "aligned" along a 7-bit boundary, but it is
> > certainly incorrectly aligned.
> >
> > >
> > > >     bool "Support misaligned load/store traps for kernel and userspace"
> > > >     select SYSCTL_ARCH_UNALIGN_ALLOW
> > > > +   depends on RISCV_PROBE_UNALIGNED_ACCESS || RISCV_EMULATED_UNALIGNED_ACCESS
> > > >     default y
> > > >     help
> > > >       Say Y here if you want the kernel to embed support for misaligned
> > > >       load/store for both kernel and userspace. When disable, misaligned
> > > >       accesses will generate SIGBUS in userspace and panic in kernel.
> > > >
> > > > +choice
> > > > +   prompt "Unaligned Accesses Support"
> > > > +   default RISCV_PROBE_UNALIGNED_ACCESS
> > > > +   help
> > > > +     This selects the hardware support for unaligned accesses. This
> > > > +     information is used by the kernel to perform optimizations. It is also
> > > > +     exposed to user space via the hwprobe syscall. The hardware will be
> > > > +     probed at boot by default.
> > > > +
> > > > +config RISCV_PROBE_UNALIGNED_ACCESS
> > > > +   bool "Probe for hardware unaligned access support"
> > > > +   help
> > > > +     During boot, the kernel will run a series of tests to determine the
> > > > +     speed of unaligned accesses. This is the only portable option. This
> > > > +     probing will dynamically determine the speed of unaligned accesses on
> > > > +     the boot hardware.
> > > > +
> > > > +config RISCV_EMULATED_UNALIGNED_ACCESS
> > > > +   bool "Assume the CPU expects emulated unaligned memory accesses"
> > > > +   depends on NONPORTABLE
> > >
> > > This is portable too, right?
> >
> > I guess so? I think I would prefer to have the probing being the only
> > portable option.
> >
> > >
> > > > +   select RISCV_MISALIGNED
> > > > +   help
> > > > +     Assume that the CPU expects emulated unaligned memory accesses.
> > > > +     When enabled, this option notifies the kernel and userspace that
> > > > +     unaligned memory accesses will be emulated by the kernel.
> > >
> > > > To enforce
> > > > +     this expectation, RISCV_MISALIGNED is selected by this option.
> > >
> > > Drop this IMO, let Kconfig handle displaying the dependencies.
> > >
> >
> > I was debating if Kconfig handling was enough, so I am glad it is, I
> > will remove this.
> >
> > > > +
> > > > +config RISCV_SLOW_UNALIGNED_ACCESS
> > > > +   bool "Assume the CPU supports slow unaligned memory accesses"
> > > > +   depends on NONPORTABLE
> > > > +   help
> > > > +     Assume that the CPU supports slow unaligned memory accesses. When
> > > > +     enabled, this option improves the performance of the kernel on such
> > > > +     CPUs.
> > >
> > > Does it? Are you sure that generating unaligned accesses on systems
> > > where they are slow is a performance increase?
> > > That said, I don't really see this option actually doing anything other
> > > than setting the value for hwprobe, so I don't actually know what the
> > > effect of this option actually is on the kernel's performance.
> > >
> > > Generally I would like to suggest a change from "CPU" to "system" here,
> > > since the slow cases that exist are mostly because the unaligned access
> > > is actually emulated in firmware.
> >
> > It would be ideal if "emulated" was used for any case of emulated
> > accesses (firmware or in the kernel).  Doing emulated accesses will be
> > orders of magnitude slower than a processor that "slowly" handles the
> > accesses.
> >
> > So even if the processor performs a "slow" access, it could still be
> > beneficial for the kernel to do the misaligned access rather than manual
> > do the alignment.
> >
> > Currently there is no place that takes into account this "slow" option
> > but I wanted to leave it open for future optimizations.
> >
> > >
> > > > However, the kernel will run much more slowly, or will not be
> > > > +     able to run at all, on CPUs that do not support unaligned memory
> > > > +     accesses.
> > > > +
> > > >  config RISCV_EFFICIENT_UNALIGNED_ACCESS
> > > >     bool "Assume the CPU supports fast unaligned memory accesses"
> > > >     depends on NONPORTABLE
> > > >     select DCACHE_WORD_ACCESS if MMU
> > > >     select HAVE_EFFICIENT_UNALIGNED_ACCESS
> > > >     help
> > > > -     Say Y here if you want the kernel to assume that the CPU supports
> > > > -     efficient unaligned memory accesses.  When enabled, this option
> > > > -     improves the performance of the kernel on such CPUs.  However, the
> > > > -     kernel will run much more slowly, or will not be able to run at all,
> > > > -     on CPUs that do not support efficient unaligned memory accesses.
> > > > +     Assume that the CPU supports fast unaligned memory accesses. When
> > > > +     enabled, this option improves the performance of the kernel on such
> > > > +     CPUs.  However, the kernel will run much more slowly, or will not be
> > > > +     able to run at all, on CPUs that do not support efficient unaligned
> > > > +     memory accesses.
> > > > +
> > > > +config RISCV_UNSUPPORTED_UNALIGNED_ACCESS
> > >
> > > This option needs to be removed. The uabi states that unaligned access
> > > is supported in userspace, if the cpu or firmware does not implement
> > > unaligned access then the kernel must emulate it.
> >
> > Should it removed from hwprobe as well then?
> 
> We had added it as a hwprobe value in this discussion:
> https://lore.kernel.org/lkml/Y+1VOXyKDDHEuejJ@spud/
> 
> Personally I like it as a possible hwprobe value, even if it is in
> conflict with the uabi. I can't fully defend it, other than as a very
> forward looking possibility, and as a nice value for people doing
> weird things off the beaten path. My preference would be to keep it in
> hwprobe, but I'm fine with not having a Kconfig for it.
> 
> -Evan

Seems reasonable to me, I will remove it from the Kconfig.

- Charlie

Powered by blists - more mailing lists