Message-ID: <694A9E5FCE0EEEFD+aKUmaxvbpuKN3_oz@LT-Guozexi>
Date: Wed, 20 Aug 2025 09:35:39 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Drew Fustini <fustini@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Alexandre Ghiti <alex@...ti.fr>
Cc: Samuel Holland <samuel.holland@...ive.com>,
	Björn Töpel <bjorn@...osinc.com>,
	Andy Chiu <andybnac@...il.com>,
	Conor Dooley <conor.dooley@...rochip.com>,
	Darius Rad <darius@...espec.com>,
	Vivian Wang <wangruikang@...as.ac.cn>,
	Florian Weimer <fweimer@...hat.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Drew Fustini <dfustini@...storrent.com>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Subject: Re: [PATCH v3] riscv: Add sysctl to control discard of vstate on
 syscall entry

On Wed, Aug 20, 2025 at 09:31:24AM +0800, Troy Mitchell wrote:
> On Tue, Aug 19, 2025 at 02:40:21AM -0700, Drew Fustini wrote:
> > From: Drew Fustini <dfustini@...storrent.com>
> > 
> > Vector registers are always clobbered in the syscall entry path to
> > enforce the documented ABI that vector state is not preserved across
> > syscalls. However, this operation can be slow on some RISC-V cores.
> > To mitigate this performance impact, add a sysctl knob to control
> > whether vector state is discarded in the syscall entry path:
> > 
> > /proc/sys/abi/riscv_v_vstate_discard
> > 
> > Valid values are:
> > 
> > 0: Vector state is not intentionally clobbered when entering a syscall
> > 1: Vector state is always clobbered when entering a syscall
> > 
> > The initial state is controlled by CONFIG_RISCV_ISA_V_VSTATE_DISCARD.
> > 
> > Fixes: 9657e9b7d253 ("riscv: Discard vector state on syscalls")
> > Signed-off-by: Drew Fustini <dfustini@...storrent.com>
> > ---
> > Changes in v3:
> >  - Reword the Kconfig description to clarify that the sysctl can still
> >    be changed during runtime regardless of the initial value chosen
> >  - Improve the description of vstate clobbering and the sysctl in
> >    section 3 of vector.rst
> >  - v2: https://lore.kernel.org/linux-riscv/20250806-riscv_v_vstate_discard-v2-1-6bfd61b2c23b@kernel.org/
> > 
> > Changes in v2:
> >  - Reword the description of the abi.riscv_v_vstate_discard sysctl to
> >    clarify that option '0' does not preserve the vector state - it just
> >    means that vector state will not always be clobbered in the syscall
> >    path.
> >  - Add clarification suggested by Palmer in v1 to the "Vector Register
> >    State Across System Calls" documentation section.
> >  - v1: https://lore.kernel.org/linux-riscv/20250719033912.1313955-1-fustini@kernel.org/
> > 
> > Test results:
> > I've tested the impact of riscv_v_vstate_discard() on the SiFive X280
> > cores [1] in the Tenstorrent Blackhole SoC [2]. The results from the
> > Blackhole P100 [3] card show that discarding the vector registers
> > increases null syscall latency by 25%.
> > 
> > The null syscall program [4] executes vsetvli and then calls getppid()
> > in a loop. The average duration of getppid() is 198 ns when registers
> > are clobbered in riscv_v_vstate_discard(). The average duration drops
> > to 149 ns when riscv_v_vstate_discard() skips clobbering the registers
> > because riscv_v_vstate_discard is set to 0.
> > 
> > $ sudo sysctl abi.riscv_v_vstate_discard=1
> > abi.riscv_v_vstate_discard = 1
> > 
> > $ ./null_syscall --vsetvli
> > vsetvli complete
> >  iterations: 1000000000
> >    duration: 198 seconds
> > avg latency: 198.73 ns
> > 
> > $ sudo sysctl abi.riscv_v_vstate_discard=0
> > abi.riscv_v_vstate_discard = 0
> > 
> > $ ./null_syscall --vsetvli
> > vsetvli complete
> >  iterations: 1000000000
> >    duration: 149 seconds
> > avg latency: 149.89 ns
> > 
> > I'm testing on the tt-blackhole-v6.16-rc1_vstate_discard [5] branch that
> > has 13 patches, including this one, on top of v6.16-rc1. Most are simple
> > yaml patches for dt bindings along with dts files and a bespoke network
> > driver. I don't think the other patches are relevant to this discussion.
> > 
> > This patch applies clean on its own mainline and riscv/for-next.
> > 
> > [1] https://www.sifive.com/cores/intelligence-x200-series
> > [2] https://tenstorrent.com/en/hardware/blackhole
> > [3] https://github.com/tenstorrent/tt-bh-linux
> > [4] https://gist.github.com/tt-fustini/ab9b217756912ce75522b3cce11d0d58
> > [5] https://github.com/tenstorrent/linux/tree/tt-blackhole-v6.16-rc1_vstate_discard
> > 
> > Signed-off-by: Drew Fustini <fustini@...nel.org>
> > ---
> [...]
> 
> > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > index 184f780c932d443d81eecac7a6fb8070ee7a5824..7a4c209ad337efd7a3995cfc7cf1700c03e55b40 100644
> > --- a/arch/riscv/kernel/vector.c
> > +++ b/arch/riscv/kernel/vector.c
> > @@ -26,6 +26,7 @@ static struct kmem_cache *riscv_v_user_cachep;
> >  static struct kmem_cache *riscv_v_kernel_cachep;
> >  #endif
> >  
> > +bool riscv_v_vstate_discard_ctl = IS_ENABLED(CONFIG_RISCV_ISA_V_VSTATE_DISCARD);
> >  unsigned long riscv_v_vsize __read_mostly;
> >  EXPORT_SYMBOL_GPL(riscv_v_vsize);
> >  
> > @@ -307,11 +308,24 @@ static const struct ctl_table riscv_v_default_vstate_table[] = {
> >  	},
> >  };
> >  
> > +static const struct ctl_table riscv_v_vstate_discard_table[] = {
> > +	{
> > +		.procname       = "riscv_v_vstate_discard",
> > +		.data           = &riscv_v_vstate_discard_ctl,
> > +		.maxlen         = sizeof(riscv_v_vstate_discard_ctl),
> > +		.mode           = 0644,
> > +		.proc_handler   = proc_dobool,
> > +	},
> > +};
> > +
> >  static int __init riscv_v_sysctl_init(void)
> >  {
> > -	if (has_vector() || has_xtheadvector())
> > +	if (has_vector() || has_xtheadvector()) {
> Is this pair of curly braces strictly necessary?
> for potential extensibility?
> 
> Acked-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> 
> Best regards,
> Troy
> >  		if (!register_sysctl("abi", riscv_v_default_vstate_table))
> >  			return -EINVAL;
> > +		if (!register_sysctl("abi", riscv_v_vstate_discard_table))
> > +			return -EINVAL;
Ah, my earlier comment was based on the assumption that this was a modified line.
Since it's actually newly added, that was my mistake. :(

> > +	}
> >  	return 0;
> >  }
> >  
> > 
> > ---
> > base-commit: 3ac864c2d9bb8608ee236e89bf561811613abfce
> > change-id: 20250818-riscv_v_vstate_discard-e89b3181e0ac
> > 
> > Best regards,
> > -- 
> > Drew Fustini <fustini@...nel.org>
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists