Re: [v5, 5/6] riscv: lib: vectorize copy_to_user/copy_from_user

From: Andrew Jones <ajones@ventanamicro.com>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: "Andy Chiu" <andy.chiu@sifive.com>,
	linux-riscv@lists.infradead.org, palmer@dabbelt.com,
	greentime.hu@sifive.com, guoren@linux.alibaba.com,
	bjorn@kernel.org, ardb@kernel.org, arnd@arndb.de,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Han-Kuan Chen" <hankuan.chen@sifive.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Bo YU" <tsu.yubo@gmail.com>,
	"Alexandre Ghiti" <alexghiti@rivosinc.com>,
	"Clément Léger" <cleger@rivosinc.com>
Subject: Re: [v5, 5/6] riscv: lib: vectorize copy_to_user/copy_from_user
Date: Fri, 15 Dec 2023 14:52:57 +0100	[thread overview]
Message-ID: <20231215-583aa7503721723916a1819e@orel> (raw)
In-Reply-To: <ZXvxbbBWXqIy/05b@ghost>

On Thu, Dec 14, 2023 at 10:25:49PM -0800, Charlie Jenkins wrote:
> On Thu, Dec 14, 2023 at 03:57:20PM +0000, Andy Chiu wrote:
...
> >  SYM_FUNC_START(__asm_copy_to_user)
> > +#ifdef CONFIG_RISCV_ISA_V
> > +	ALTERNATIVE("j fallback_scalar_usercopy", "nop", 0, RISCV_ISA_EXT_v, CONFIG_RISCV_ISA_V)
> 
> has_vector uses riscv_has_extension_unlikely, but this is the equivalent
> of riscv_has_extension_likely. It seems like this should be consistent
> across all call sites. Since has_vector uses the unlikely version, this
> should probably be rearranged so that the nop is in the non-vector
> version and the jump is for the vector version.

I think I prefer it the way it is, where the optimized path is fully
optimized and the fallback path also suffers the jump. (I've also
taken that approach for clear_page()). Also, as extensions are adopted
by more an more platforms, and we start to consider switching unlikelys
to likelys, then it would be easy to miss stuff like this.

> 
> A neat optimization you can do here is replace the "nop" with the
> instruction that will be executed first. With how it's written right now
> you could replace the nop with the la instruction. It's just a nop so
> the performance difference is probably not going to be noticable but
> it's theoretically better without the nop. The downside of doing this is

I think I prefer the nop, because it's easier to read and maintain the 
assembly function when the ALTERNATIVE doesn't do anything other than
choose the entry point.

> that it seems like alternatives do not work with macros so you couldn't
> replace the nop with a REG_L instruction, unless there is some trick to
> make it work.

One should be able to use REG_L in an alternative since macro expansion
will result in the string "ld" or "lw", which can then be concatenated
with its parameters, e.g.

  ALTERNATIVE(REG_L " a1, 0(a2)", "nop", 0, 0, 0)

(But note the space before the a1. Without it, we'd get "lda1,")

Thanks,
drew

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