Received-SPF: pass (google.com: domain of linux-kernel+bounces-83859-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 27 Feb 2024 18:48:54 +0000
From: Conor Dooley <conor@kernel.org>
To: Charlie Jenkins <charlie@rivosinc.com>
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: <20240227-citable-scanning-1fd48c96b758@spud>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="A7lTG7rB6ssaDl7T"
Content-Disposition: inline
In-Reply-To: <Zd4nMa+x28omuhiF@ghost>


--A7lTG7rB6ssaDl7T
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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
> >=20
> > This is portable too, right?
>=20
> 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.


> > > +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 su=
ch
> > > +	  CPUs.
> >=20
> > 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.
> >=20
> > 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.
>=20
> 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.
>=20
> 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
|=20
|     icicle:
|=20
|         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
|=20
|         cpu0: Ratio of byte access time to unaligned word access is
| 0.00, unaligned accesses are slow
|=20
|     k210:
|=20
|         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
|=20
|     starlight:
|=20
|         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
|=20
|     vexriscv/orangecrab:
|=20
|         cpu0: Ratio of byte access time to unaligned word access is
| 0.00, unaligned accesses are slow
 https://lore.kernel.org/all/CAMuHMdVtXGjP8VFMiv-7OMFz1XvfU1cz=3DFw4jL3fcp4=
wO1etzQ@mail.gmail.com/

> 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".

> > > 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 a=
ll,
> > > -	  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 su=
ch
> > > +	  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 unalign=
ed
> > > +	  memory accesses.
> > > +
> > > +config RISCV_UNSUPPORTED_UNALIGNED_ACCESS
> >=20
> > 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.
>=20
> 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.

Cheers,
Conor.

--A7lTG7rB6ssaDl7T
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZd4ulgAKCRB4tDGHoIJi
0kuJAP9MYXtRJnS8kVEPM3J3GBXPiW1BrtrqVqpMeqaykH3O9AEAp8BCDPdp8oIV
kh1+j1M36s5fTlWCuPBx975g86gHVwk=
=Gp5I
-----END PGP SIGNATURE-----

--A7lTG7rB6ssaDl7T--