| Message ID | 20230526024715.8773-1-guohui@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: syscall: unmask DAIF for tracing status | expand |
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@uniontech.com> I was about to send a patch doing the same thing, so FWIW: Reviewed-by: Mark Rutland <mark.rutland@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 >
On 6/6/23 9:22 PM, Mark Rutland wrote: > 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@uniontech.com> > I was about to send a patch doing the same thing, so FWIW: > > Reviewed-by: Mark Rutland <mark.rutland@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. > Ok thanks, Mark, you are right. >> --- >> 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 >>
On Fri, 26 May 2023 10:47:15 +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); > } > > [...] Applied to arm64 (for-next/misc), thanks! [1/1] arm64: syscall: unmask DAIF for tracing status https://git.kernel.org/arm64/c/1da185fc8288
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:
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@uniontech.com> --- arch/arm64/kernel/syscall.c | 2 -- 1 file changed, 2 deletions(-)