* Re: [PATCH] arm64: syscall: Fixed the problem that local_daif_mask was called twice in a row when there was no _TIF_SYSCALL_WORK when the do_el0_svc call returned.
2021-12-03 12:17 [PATCH] arm64: syscall: Fixed the problem that local_daif_mask was called twice in a row when there was no _TIF_SYSCALL_WORK when the do_el0_svc call returned yushengjin
@ 2021-12-03 14:48 ` Mark Rutland
0 siblings, 0 replies; 2+ messages in thread
From: Mark Rutland @ 2021-12-03 14:48 UTC (permalink / raw)
To: yushengjin
Cc: linux-arm-kernel, catalin.marinas, will, keescook, maz,
joey.gouly
On Fri, Dec 03, 2021 at 08:17:54PM +0800, yushengjin wrote:
> This commit f37099b6992a ("arm64: convert syscall trace logic to C") is
> a patch that converts syscall trace to C. It was submitted to the v4.18
> branch in 2018, but there is a small problem in the conversion process,
> which is still there.
>
> After calling disable_daif, the previous assembly code ret_fast_syscall
> will directly call kernel_exit to exit the kernel state if there is no
> _TIF_SYSCALL_WORK flag bit,Instead of calling ret_to_user.But after
> converting this assembly code into C,After checking the flag bit and
> finding no _TIF_SYSCALL_WORK and it returns, ret_to_user will be called
> unconditionally, and ret_to_user also has a code of disable_daif, which
> will cause ret_to_user to be called twice in a row in this program execution
^^^^^^^^^^^
> flow.
I think you mean `disable_daif` here? AFAICT we can't call `ret_to_user` twice,
so I assume that's a typo.
Looking around a bit, I think the DAIF masking in el0_svc_common() isn't
actually unnecessary, and we can probably restructure things to avoid that. The
common entry code checks the tracing flags and performs the tracing with IRQs
unmasked, and I'm not aware of any reason we need to do differently. I'll need
to dig back through the history, but I think we're doing this for historical
reasons which no longer hold.
If we unmask DAIF in el0_svc() and el0_svc_compat(), and remove all the DAIF
manipulation from el0_svc_common(), that would align with all the other EL0
handlers, and avoid poking DAIF redundantly.
> Although calling twice doesn't have much impact, I found that this disable_daif
> code has some impact on the performance of arm64 machines.
Can you say which machines you see this on, specifically which CPU
implementations?
Do you see any noticeable impact on a more complex test, e.g. something like
hackbench of `perf bench sched` ?
> Taking the gitpid test of "Unixbench syscall" as an example, we know that if
> getpid is not specially optimized in C library, the performance of getpid
> is almost the performance of system call.
>
> The performance test after patching is as follows:
>
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|83397768|1|lps
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|83521095|1|lps
>
> At the same time, I add a line of repeated code under XXX, similar to this
> -----------------
> local_daif_mask();
> local_daif_mask();
> -----------------
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|78040911|1|lps
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|78046938|1|lps
>
> This shows that the operation of this daIf has a loss of performance，
> Therefore, after fixing the problem of repeated calls twice, the performance
> is as follows.
>
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|93664649|1|lps
> ~/UnixBench$ ./pgms/syscall 10 g
> COUNT|94852639|1|lps
>
> Signed-off-by: yushengjin <yushengjin@uniontech.com>
> ---
> arch/arm64/kernel/entry-common.c | 13 ++++++++++++-
> arch/arm64/kernel/syscall.c | 1 +
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index f7408edf8571..99baf32b3e3d 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -141,6 +141,17 @@ static __always_inline void exit_to_user_mode(struct pt_regs *regs)
> __exit_to_user_mode();
> }
>
> +static __always_inline void el0_svc_exit_to_user_mode(struct pt_regs *regs)
> +{
> + unsigned long flags;
> +
> + flags = READ_ONCE(current_thread_info()->flags);
> + if (unlikely(flags & _TIF_WORK_MASK))
> + do_notify_resume(regs, flags);
> + mte_check_tfsr_exit();
> + __exit_to_user_mode();
> +}
> +
> asmlinkage void noinstr asm_exit_to_user_mode(struct pt_regs *regs)
> {
> exit_to_user_mode(regs);
> @@ -601,7 +612,7 @@ static void noinstr el0_svc(struct pt_regs *regs)
> enter_from_user_mode(regs);
> cortex_a76_erratum_1463225_svc_handler();
> do_el0_svc(regs);
> - exit_to_user_mode(regs);
> + el0_svc_exit_to_user_mode(regs);
> }
I would strongly prefer to have the EL0 handlers all use the same
exit_to_user_mode(). I believe (as above) that we can remove the masking from
el0_svc_common() and make this:
| static void noinstr el0_svc(struct pt_regs *regs)
| {
| enter_from_user_mode(regs);
| cortex_a76_erratum_1463225_svc_handler();
| local_daif_restore(DAIF_PROCCTX);
| do_el0_svc(regs);
| exit_to_user_mode(regs);
| }
... but if there's some reason we can't do that, I'd prefer we pull the DAIF
masking out of exit_to_user_mode(), and have it in each handler, so that it
clearly balances with the prior local_daif_restore() or similar.
Thanks,
Mark.
>
> static void noinstr el0_fpac(struct pt_regs *regs, unsigned long esr)
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index 50a0f1a38e84..4095786c85f1 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -156,6 +156,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>
> trace_exit:
> syscall_trace_exit(regs);
> + local_daif_mask();
> }
>
> static inline void sve_user_discard(void)
> --
> 2.20.1
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread