Re: [PATCH v2 1/2] riscv: introduce RISCV_EFFICIENT_UNALIGNED_ACCESS

[Charlie Jenkins <charlie@rivosinc.com>]

On Tue, Dec 05, 2023 at 09:53:50PM +0800, Jisheng Zhang wrote:
> On Mon, Dec 04, 2023 at 06:14:06PM -0800, Eric Biggers wrote:
> > On Mon, Dec 04, 2023 at 11:15:28AM -0800, Charlie Jenkins wrote:
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index 7f8aa25457ba..0a76209e9b02 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -654,6 +654,18 @@ config RISCV_MISALIGNED
> > > >  	  load/store for both kernel and userspace. When disable, misaligned
> > > >  	  accesses will generate SIGBUS in userspace and panic in kernel.
> > > >  
> > > > +config RISCV_EFFICIENT_UNALIGNED_ACCESS
> > > 
> > > There already exists hwprobe for this purpose. If kernel code wants to
> > > leverage the efficient unaligned accesses of hardware, it can use static
> > > keys. I have a patch that will set this static key if the hardware was
> > > detected to have fast unaligned accesses:
> > > 
> > > https://lore.kernel.org/linux-riscv/20231117-optimize_checksum-v11-2-7d9d954fe361@rivosinc.com/
> > 
> > Is the plan to make the get_unaligned* and put_unaligned* macros expand to code
> > for both cases, and select between them using a static key?  Note that there are
> > a very large number of callers of these macros in the kernel.  And what about
> > kernel code that checks CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS directly?
> > 
> > AFAIK, no other Linux architecture supports kernel images where the unaligned
> > access support is unknown at compile time.  It's not clear to me that such an
> > approach is feasible.  A static key can easily be provided, but it's unclear
> > what code would use it, given that currently lots of kernel code assumes that
> > unaligned access support is known at compile time.
> > 
> > Meanwhile, there are people building kernels they know will only be deployed on
> > systems where unaligned accesses are supported.  To me, it seems useful to
> > provide a kconfig option for them to build a more efficient kernel.
> 
> Generally, I agree with Eric's above points. Various subsystem such as net, mm,
> lib and so on have different code path for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS,
> while Charlie's patch only touch partial code of arch/riscv, and even if those
> subsystem maintainers agree with dynamic code patching(I still believe
> persuading those subsystem maintainers is not easy), that's still a
> huge task which needs to be done step by step. So before that, we'd
> better let this series merged and benefit all efficient unaligned access
> riscv systems. When the huge task is completed, we can remove the config
> option.
> 
> Thanks

It would be best to enable all of the paths that leverage
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS at runtime (using hwprobe)
instead of using a compile-time flag to do so. However, as you say, that
is large task and doesn't need to be done immediately. For now I agree
it is sufficient to use this new RISCV_EFFICIENT_UNALIGNED_ACCESS
config.

- Charlie

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>