Re: [PATCH v5 2/2] riscv: Set unaligned access speed at compile time

From: Conor Dooley <conor@kernel.org>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: "Albert Ou" <aou@eecs.berkeley.edu>,
	linux-kernel@vger.kernel.org,
	"Eric Biggers" <ebiggers@kernel.org>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Evan Green" <evan@rivosinc.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Jisheng Zhang" <jszhang@kernel.org>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Clément Léger" <cleger@rivosinc.com>,
	linux-riscv@lists.infradead.org,
	"Charles Lohr" <lohr85@gmail.com>
Subject: Re: [PATCH v5 2/2] riscv: Set unaligned access speed at compile time
Date: Thu, 29 Feb 2024 18:49:33 +0000	[thread overview]
Message-ID: <20240229-dolphin-pennant-eef41ce44043@spud> (raw)
In-Reply-To: <ZeDO4CVOi6q4u8Lt@ghost>

[-- Attachment #1.1: Type: text/plain, Size: 7836 bytes --]

On Thu, Feb 29, 2024 at 10:37:20AM -0800, Charlie Jenkins wrote:
> On Thu, Feb 29, 2024 at 12:26:37PM +0000, Conor Dooley wrote:
> > On Tue, Feb 27, 2024 at 03:13:14PM -0800, Charlie Jenkins wrote:
> > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index bffbd869a068..ad0a9c1f8802 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -690,25 +690,58 @@ config THREAD_SIZE_ORDER
> > >  config RISCV_MISALIGNED
> > >  	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 probing will dynamically determine
> > > +	  the speed of unaligned accesses on the boot hardware.
> > > +
> > > +config RISCV_EMULATED_UNALIGNED_ACCESS
> > > +	bool "Assume the system expects emulated unaligned memory accesses"
> > > +	help
> > > +	  Assume that the system expects emulated unaligned memory accesses.
> > > +	  When enabled, this option notifies the kernel and userspace that
> > > +	  unaligned memory accesses will be emulated by either the kernel or
> > > +	  firmware.
> > > +
> > > +config RISCV_SLOW_UNALIGNED_ACCESS
> > > +	bool "Assume the system supports slow unaligned memory accesses"
> > > +	depends on NONPORTABLE
> > > +	help
> > > +	  Assume that the system supports slow unaligned memory accesses. The
> > > +	  kernel may not be able to run at all on systems that do not support
> > > +	  unaligned memory accesses.
> > > +
> > >  config RISCV_EFFICIENT_UNALIGNED_ACCESS
> > > -	bool "Assume the CPU supports fast unaligned memory accesses"
> > > +	bool "Assume the system 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 system supports fast unaligned memory accesses. When
> > > +	  enabled, this option improves the performance of the kernel on such
> > > +	  systems.  However, the kernel will run much more slowly, or will not
> > > +	  be able to run at all, on systems that do not support efficient
> > > +	  unaligned memory accesses.
> > >  
> > > -	  If unsure what to do here, say N.
> > > +endchoice
> > 
> > Thinking about this some more, you've got 6 different options here:
> > 
> > 1 probed with no emulation available (choice set to probe + RISCV_MISALIGNED=n)
> > 2 probe with in-kernel emulation available (choice set to probe + RISCV_MISALIGNED=y)
> > 3 in-kernel emulation only (choice set to emulated + RISCV_MISALIGNED=y)
> > 4 no in-kernel emulation but report emulated (choice set to emulated + RISCV_MISALIGNED=n)
> > 5 slow unaligned (choice set to slow)
> > 6 fast unaligned (choice set to fast)
> > 
> > Out of these, only 2 and 3 are portable options, since 1, 4 and 5 will
> > cause uabi issues if the CPUs or firmware does not support unaligned
> > access & 6 will not run in the same circumstances.
> > 
> > My first thought here was about the motivation for the patch and what it
> > has resulted in. Being able to support HAVE_EFFICIENT_ALIGNED_ACCESS is
> > pretty nice, but it then seems like beyond that we are introducing
> > configuration for configurations sake, without looking at what the
> > resultant kernels will be useful for. Having 6 different options for how
> > the kernel can be configured in this way seems excessive and I don't
> > really get why some of them are even useful.
> > 
> > Take for example situation 4. Unless I have misunderstood the Kconfig
> > options above, if you configure a kernel in that way, it will always
> > report as emulated, but there is no emulation provided. This just seems
> > like a option that's only purpose is setting a hwprobe value, which is
> > a dog wagging the tail situation to me.
> 
> This goes back to my earlier comment that it would make sense for
> somebody to select "emulated" even if the unaligned address is being
> emulated by firmware. However, there may be no users for this and if
> needed we can add that in the future.

I don't think that is going to provide any useful information, give you
will never be able to know if slow means "slow but done by the cpu" or
"slow but emulated by the firmware".

> > The other thing is about what options are actually marked as
> > NONPORTABLE, given it is done in the choice option - but whether or not
> > something is actually non-portable actually depends on whether or not
> > the in-kernel emulator exists.
> > 
> > I think what I would do here is simplify this quite a bit, starting by
> > making RISCV_MISALIGNED an internal option that users cannot enable but
> > is selected by the PORTABLE choice options. I would then re-work the
> > choice options a bit. My 4 would be:
> > 
> > 1 probe: probe at boot time, falling back to emulated if not performant
> > 2 emulated: always emulate it in the kernel
> > 3 slow: don't probe or emulate in the kernel
> > 4 fast: Your current fast option
> 
> Emulated doesn't mean that the kernel will always emulate the unaligned
> accesses. It means that the kernel has the ability to emulate them. It
> will only emulate them if the CPU traps on unaligned accesses. Kernel
> code can choose to forcefully align an address it thinks may cause an
> unaligned access, but that's slightly different from emulated.

Sure, make option 2 "don't probe at boot time, emulate it in the kernel
if we trap". I suppose in this case though, to get a correct output in
hwprobe you'd have to still attempt an unaligned access at boot time to
see if you trap but it will not perform the speed test?

> Emulated is much slower than "slow", so it seems like it unfairly
> penalizes "slow" to group the options for firmware emulated with a CPU
> that actually just does slow unaligned accesses.

As I said in my previous email, "slow" has a huge variance and may not
be meaningfully faster. I wonder if it might actually even be slower on
some systems that emulate it in firmware?

> > 1 & 2 select RISCV_UNALIGNED and are portable because they contain the
> > emulated support and thus satisfy the UABI constaints.
> > 3 & 4 are marked NONPORTABLE. I think 3 will run on systems that don't
> > support unaligned accesses but it will have UABI problems. 4 for the
> > reason mentioned in the Kconfig option above.
> > 
> > I think that that gives you 4 meaningful options, what do you think?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv