[PATCH] arm64: syscall: unmask DAIF for tracing status

[Mark Rutland]

On Fri, May 26, 2023 at 10:47:15AM +0800, Guo Hui wrote:
> The following code:
> static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>                             const syscall_fn_t syscall_table[])
> {
>     ...
> 
>     if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
>         local_daif_mask();
>         flags = read_thread_flags(); -------------------------------- A
>         if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
>              return;
>         local_daif_restore(DAIF_PROCCTX);
>      }
> 
> trace_exit:
>      syscall_trace_exit(regs); ------- B
> }
> 
> 1. The flags in the if conditional statement should be used
> in the same way as the flags in the function syscall_trace_exit,
> because DAIF is not shielded in the function syscall_trace_exit,
> so when the flags are obtained in line A of the code,
> there is no need to shield DAIF.
> Don't care about the modification of flags after line A.
> 
> 2. Masking DAIF caused syscall performance to deteriorate by about 10%.
> The Unixbench single core syscall performance data is as follows:
> 
> Machine: Kunpeng 920
> 
> Mask DAIF:
> System Call Overhead                    1172314.1 lps   (10.0 s, 7 samples)
> 
> System Benchmarks Partial Index          BASELINE       RESULT    INDEX
> System Call Overhead                      15000.0    1172314.1    781.5
>                                                                ========
> System Benchmarks Index Score (Partial Only)                      781.5
> 
> Unmask DAIF:
> System Call Overhead                    1287944.6 lps   (10.0 s, 7 samples)
> 
> System Benchmarks Partial Index          BASELINE       RESULT    INDEX
> System Call Overhead                      15000.0    1287944.6    858.6
>                                                                ========
> System Benchmarks Index Score (Partial Only)                      858.6
> 
> Signed-off-by: Guo Hui <guohui at uniontech.com>

I was about to send a patch doing the same thing, so FWIW:

Reviewed-by: Mark Rutland <mark.rutland at arm.com>

As for why this is safe, rationale below.

This masking is an artifact of the old "ret_fast_syscall" assembly that
was converted to C in commit:

  f37099b6992a0b81 ("arm64: convert syscall trace logic to C")

The assembly would mask DAIF, check the thread flags, and fall through
to kernel_exit without unmasking if no tracing was needed. The
conversion copied this masking into the C version, though this wasn't
strictly necessary.

As (in general) thread flags can be manipulated by other threads, it's
not safe to manipulate the thread flags with plain reads and writes, and
since commit:

  342b3808786518ce ("arm64: Snapshot thread flags")

... we use read_thread_flags() to read the flags atomically.

With this, there is no need to mask DAIF transiently around reading the
flags, as we only decide whether to trace while DAIF is masked, and the
actual tracing occurs with DAIF unmasked. When el0_svc_common() returns
its caller will unconditionally mask DAIF via exit_to_user_mode(), so
the masking is redundant.

Mark.


> ---
>  arch/arm64/kernel/syscall.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index da84cf855c44..5a668d7f3c1f 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -147,11 +147,9 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>  	 * exit regardless, as the old entry assembly did.
>  	 */
>  	if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
> -		local_daif_mask();
>  		flags = read_thread_flags();
>  		if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
>  			return;
> -		local_daif_restore(DAIF_PROCCTX);
>  	}
>  
>  trace_exit:
> -- 
> 2.20.1
>