Received-SPF: pass (google.com: domain of linux-kernel+bounces-83913-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 27 Feb 2024 11:20:02 -0800
From: Charlie Jenkins <charlie@rivosinc.com>
To: Conor Dooley <conor@kernel.org>
Cc: Conor Dooley <conor.dooley@microchip.com>,
	Albert Ou <aou@eecs.berkeley.edu>, linux-kernel@vger.kernel.org,
	Eric Biggers <ebiggers@kernel.org>, Evan Green <evan@rivosinc.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Jisheng Zhang <jszhang@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	=?iso-8859-1?Q?Cl=E9ment_L=E9ger?= <cleger@rivosinc.com>,
	linux-riscv@lists.infradead.org, Charles Lohr <lohr85@gmail.com>
Subject: Re: [PATCH v4 2/2] riscv: Set unalignment speed at compile time
Message-ID: <Zd414nBtjHcsYWnL@ghost>
References: <20240216-disable_misaligned_probe_config-v4-0-dc01e581c0ac@rivosinc.com>
 <20240216-disable_misaligned_probe_config-v4-2-dc01e581c0ac@rivosinc.com>
 <20240227-condone-impeach-9469dffc6b47@wendy>
 <Zd4nMa+x28omuhiF@ghost>
 <20240227-citable-scanning-1fd48c96b758@spud>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240227-citable-scanning-1fd48c96b758@spud>

On Tue, Feb 27, 2024 at 06:48:54PM +0000, Conor Dooley wrote:
> On Tue, Feb 27, 2024 at 10:17:21AM -0800, Charlie Jenkins 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:
> 
> > > > +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.
> 
> I dunno, I think there could be value to someone in always emulating
> this in the kernel and I don't think that should relegate them to the
> naughty step, given it can work everywhere.

Alright, I will remove the nonportable.

> 
> 
> > > > +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.
> 
> Right. But, at least from a probing perspective, SLOW is what gets
> selected when firmware emulates the unaligned access so to userspace
> seeing slow means that the performance could be horrifically bad:
> 
> |     rzfive:
> |         cpu0: Ratio of byte access time to unaligned word access is
> | 1.05, unaligned accesses are fast
> | 
> |     icicle:
> | 
> |         cpu1: Ratio of byte access time to unaligned word access is
> | 0.00, unaligned accesses are slow
> |         cpu2: Ratio of byte access time to unaligned word access is
> | 0.00, unaligned accesses are slow
> |         cpu3: Ratio of byte access time to unaligned word access is
> | 0.00, unaligned accesses are slow
> | 
> |         cpu0: Ratio of byte access time to unaligned word access is
> | 0.00, unaligned accesses are slow
> | 
> |     k210:
> | 
> |         cpu1: Ratio of byte access time to unaligned word access is
> | 0.02, unaligned accesses are slow
> |         cpu0: Ratio of byte access time to unaligned word access is
> | 0.02, unaligned accesses are slow
> | 
> |     starlight:
> | 
> |         cpu1: Ratio of byte access time to unaligned word access is
> | 0.01, unaligned accesses are slow
> |         cpu0: Ratio of byte access time to unaligned word access is
> | 0.02, unaligned accesses are slow
> | 
> |     vexriscv/orangecrab:
> | 
> |         cpu0: Ratio of byte access time to unaligned word access is
> | 0.00, unaligned accesses are slow
>  https://lore.kernel.org/all/CAMuHMdVtXGjP8VFMiv-7OMFz1XvfU1cz=Fw4jL3fcp4wO1etzQ@mail.gmail.com/

If the accesses are horrifically slow then maybe they should be flagged
as emulated rather than slow by the probe.

> 
> > Currently there is no place that takes into account this "slow" option
> > but I wanted to leave it open for future optimizations.
> 
> I don't think you can really do much optimisation if you detect that it
> is slow, and since this option is analogous to detecting slow I dunno if
> you can do anything here either? This option really just seems to me to
> mean "don't do any optimisations for unaligned being fast but also don't
> emulate it in the kernel".

I am fine with that being the meaning of this option. However, on a
system that has misaligned accesses that are twice as slow as correctly
aligned accesses, the misaligned accesses would reasonably be selected
as "slow". However, something like the checksum functions would still
probably want to do the misaligned accesses because performing the
alignment would be even slower. This is all hypothetical and not a
"real" use case so maybe I am optimizing where no optimization is
needed.

> 
> > > > 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?
> 
> No, I actually suggested that it be documented etc. Originally
> "UNSUPPORTED" was "UNKNOWN" and nothing more than the default value but
> I suggested that it be documented since that would allow a system that
> did not have the same uabi problem to use all the same defines.
> Technically it is possible for unaligned access to be unsupported, if
> you have a kernel that does not have the emulator but does have the
> hwprobe stuff supported. I think there was about a 6 month period where
> this was the case, so combine that with firmware that does not do the
> emulation and unaligned accesses are unsupported.

Sounds great, will remove :)

- Charlie

> 
> Cheers,
> Conor.