Re: [PATCH v4 2/2] riscv: Set unalignment speed at compile time

[Evan Green]

On Tue, Feb 27, 2024 at 10:17 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > >  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