Messages in this thread | | | Date | Tue, 27 Feb 2024 11:07:18 -0800 | From | Charlie Jenkins <> | 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@rivosinc.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@rivosinc.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
| |