Re: [External] [PATCH] RISC-V: store percpu offset in CSR_SCRATCH

[yunhui cui <cuiyunhui@bytedance.com>]

Hi Radim,

On Tue, Jul 8, 2025 at 7:10 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-07-08T18:07:27+08:00, yunhui cui <cuiyunhui@bytedance.com>:
> > This patch cleverly differentiates whether an exception originates
> > from user mode or kernel mode. However, there's still an issue with
> > using CSR_SCRATCH: each time handle_exception() is called, the
> > following instructions must be executed:
> >
> > REG_L s0, TASK_TI_CPU(tp)
> > slli s0, s0, 3
> > la s1, __per_cpu_offset
> > add s1, s1, s0
> > REG_L s1, 0(s1)
> > csrw CSR_SCRATCH, s1
>
> We can minimize the cost at exception entry by storing the precomputed
> offset in thread_info, which bloats the struct, and also incurs update
> cost on cpu migration, but should still be a net performance gain.
>
> The minimal code at exception entry would be:
>
>   REG_L s0, TASK_TI_PERCPU_OFFSET(tp)
>   csrw CSR_SCRATCH, s0
>
> > Should we consider adding a dedicated CSR (e.g., CSR_SCRATCH2) to
> > store the percpu offset instead?
> > See: https://lists.riscv.org/g/tech-privileged/topic/113437553#msg2506
>
> It would be nice to gather more data on the CSR_SCRATCH approach.
> Basically, the overhead of "REG_L s0, TASK_TI_PERCPU_OFFSET(tp)".
> (Or the longer sequence if we think it is worth it.)
>
> Can you benchmark the patch after reverting percpu.h, so we include the
> overhead of switching CSR_SCRATCH, but without any benefits provided by
> the per-cpu offset?
> The baseline would be the patch with reverted percpu.h, and reverted the
> sequence that sets the CSR_SCRATCH in handle_exception, so we roughly
> estimate the benefit of adding CSR_SCRATCH2.
>
> The CSR_SCRATCH2 does add overhead to hardware, and to domain context
> switches, and we also have to do something else for a few years anyway,
> because it's not even ratified...  It's possible we might not benefit
> enough from CSR_SCRATCH2 to make a good case for it.
>
> Thanks.

Bench platform: Spacemit(R) X60
No changes:
6.77, 6.791, 6.792, 6.826, 6.784, 6.839, 6.776, 6.733, 6.795, 6.763
Geometric mean: 6.786839305
Reusing the current scratch:
7.085, 7.09, 7.021, 7.089, 7.068, 7.034, 7.06, 7.062, 7.065, 7.051
Geometric mean: 7.062466876
A degradation of approximately 4.06% is observed. The possible cause
of the degradation is that the CSR_TVEC register is set every time a
kernel/user exception occurs.

The following is the patch without percpu optimization, which only
tests the overhead of separating exceptions into kernel and user
modes.

---
 arch/riscv/kernel/entry.S | 39 ++++++++++++++++++++++-----------------
 arch/riscv/kernel/head.S  |  7 +------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 9d1a305d5508..cc2fd4cd54a0 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -19,17 +19,8 @@

  .section .irqentry.text, "ax"

-SYM_CODE_START(handle_exception)
- /*
- * If coming from userspace, preserve the user thread pointer and load
- * the kernel thread pointer.  If we came from the kernel, the scratch
- * register will contain 0, and we should continue on the current TP.
- */
- csrrw tp, CSR_SCRATCH, tp
- bnez tp, .Lsave_context
-
-.Lrestore_kernel_tpsp:
- csrr tp, CSR_SCRATCH
+SYM_CODE_START(handle_kernel_exception)
+ csrw CSR_SCRATCH, tp
  REG_S sp, TASK_TI_KERNEL_SP(tp)

 #ifdef CONFIG_VMAP_STACK
@@ -40,7 +31,20 @@ SYM_CODE_START(handle_exception)
  REG_L sp, TASK_TI_KERNEL_SP(tp)
 #endif

-.Lsave_context:
+ j handle_exception
+SYM_CODE_END(handle_kernel_exception)
+
+SYM_CODE_START(handle_user_exception)
+ /*
+ * If coming from userspace, preserve the user thread pointer and load
+ * the kernel thread pointer.
+ */
+ csrrw tp, CSR_SCRATCH, tp
+ j handle_exception
+
+SYM_CODE_END(handle_user_exception)
+
+SYM_CODE_START_NOALIGN(handle_exception)
  REG_S sp, TASK_TI_USER_SP(tp)
  REG_L sp, TASK_TI_KERNEL_SP(tp)
  addi sp, sp, -(PT_SIZE_ON_STACK)
@@ -71,11 +75,8 @@ SYM_CODE_START(handle_exception)
  REG_S s4, PT_CAUSE(sp)
  REG_S s5, PT_TP(sp)

- /*
- * Set the scratch register to 0, so that if a recursive exception
- * occurs, the exception vector knows it came from the kernel
- */
- csrw CSR_SCRATCH, x0
+ la s1, handle_kernel_exception
+ csrw CSR_TVEC, s1

  /* Load the global pointer */
  load_global_pointer
@@ -141,6 +142,10 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
  * structures again.
  */
  csrw CSR_SCRATCH, tp
+
+ la a0, handle_user_exception
+ csrw CSR_TVEC, a0
+
 1:
 #ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
  move a0, sp
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index a2e2f0dd3899..992acec3bc87 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -172,14 +172,9 @@ secondary_start_sbi:
 .align 2
 .Lsetup_trap_vector:
  /* Set trap vector to exception handler */
- la a0, handle_exception
+ la a0, handle_kernel_exception
  csrw CSR_TVEC, a0

- /*
- * Set sup0 scratch register to 0, indicating to exception vector that
- * we are presently executing in kernel.
- */
- csrw CSR_SCRATCH, zero
  ret

 .align 2
--
2.43.0


Thanks,
Yunhui