Hi Chris,
On Wed, Mar 02, 2016 at 03:09:35PM -0500, Chris Metcalf wrote:
> Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> state machine that can be difficult to reason about due to duplicated
> code and a large number of branch targets.
>
> This patch factors the common logic out into the existing
> do_notify_resume function, converting the code to C in the process,
> making the code more legible.
>
> This patch tries to closely mirror the existing behaviour while using
> the usual C control flow primitives. As local_irq_{disable,enable} may
> be instrumented, we balance exception entry (where we will almost most
> likely enable IRQs) with a call to trace_hardirqs_on just before the
> return to userspace.
[...]
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 1f7f5a2b61bf..966d0d4308f2 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -674,18 +674,13 @@ ret_fast_syscall_trace:
> * Ok, we need to do extra processing, enter the slow path.
> */
> work_pending:
> - tbnz x1, #TIF_NEED_RESCHED, work_resched
> - /* TIF_SIGPENDING, TIF_NOTIFY_RESUME or TIF_FOREIGN_FPSTATE case */
> mov x0, sp // 'regs'
> - enable_irq // enable interrupts for
> do_notify_resume()
> bl do_notify_resume
> - b ret_to_user
> -work_resched:
> #ifdef CONFIG_TRACE_IRQFLAGS
> - bl trace_hardirqs_off // the IRQs are off here,
> inform the tracing code
> + bl trace_hardirqs_on // enabled while in userspace
This doesn't look right to me. We only get here after running
do_notify_resume, which returns with interrupts disabled.
Do we not instead need to inform the tracing code that interrupts are
disabled prior to calling do_notify_resume?
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index e18c48cb6db1..3432e14b7d6e 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -402,15 +402,29 @@ static void do_signal(struct pt_regs *regs)
> asmlinkage void do_notify_resume(struct pt_regs *regs,
> unsigned int thread_flags)
> {
> - if (thread_flags & _TIF_SIGPENDING)
> - do_signal(regs);
> + while (true) {
>
> - if (thread_flags & _TIF_NOTIFY_RESUME) {
> - clear_thread_flag(TIF_NOTIFY_RESUME);
> - tracehook_notify_resume(regs);
> - }
> + if (thread_flags & _TIF_NEED_RESCHED) {
> + schedule();
> + } else {
> + local_irq_enable();
> +
> + if (thread_flags & _TIF_SIGPENDING)
> + do_signal(regs);
>
> - if (thread_flags & _TIF_FOREIGN_FPSTATE)
> - fpsimd_restore_current_state();
> + if (thread_flags & _TIF_NOTIFY_RESUME) {
> + clear_thread_flag(TIF_NOTIFY_RESUME);
> + tracehook_notify_resume(regs);
> + }
> +
> + if (thread_flags & _TIF_FOREIGN_FPSTATE)
> + fpsimd_restore_current_state();
> + }
>
> + local_irq_disable();
> +
> + thread_flags = READ_ONCE(current_thread_info()->flags);
> + if (!(thread_flags & _TIF_WORK_MASK))
> + break;
> + }
This might be easier to read as a do { ... } while.
Will
