| Message ID | 20210109032557.13831-5-cmr@codefail.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve signal performance on PPC64 with KUAP | expand |
Le 09/01/2021 à 04:25, Christopher M. Riedl a écrit : > Rework the messy ifdef breaking up the if-else for TM similar to > commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else"). > > Unlike that commit for ppc32, the ifdef can't be removed entirely since > uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM. > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > --- > arch/powerpc/kernel/signal_64.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index b211a8ea4f6e..dd3787f67a78 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn) > struct pt_regs *regs = current_pt_regs(); > struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1]; > sigset_t set; > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > unsigned long msr; > -#endif > > /* Always make any pending restarted system calls return -EINTR */ > current->restart_block.fn = do_no_restart_syscall; > @@ -762,10 +760,12 @@ SYSCALL_DEFINE0(rt_sigreturn) > * restore_tm_sigcontexts. > */ > regs->msr &= ~MSR_TS_MASK; > +#endif > > if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR])) > goto badframe; This means you are doing that __get_user() even when msr is not used. That should be avoided. > if (MSR_TM_ACTIVE(msr)) { > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > /* We recheckpoint on return. */ > struct ucontext __user *uc_transact; > > @@ -778,9 +778,8 @@ SYSCALL_DEFINE0(rt_sigreturn) > if (restore_tm_sigcontexts(current, &uc->uc_mcontext, > &uc_transact->uc_mcontext)) > goto badframe; > - } else > #endif > - { > + } else { > /* > * Fall through, for non-TM restore > * > @@ -818,10 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > unsigned long newsp = 0; > long err = 0; > struct pt_regs *regs = tsk->thread.regs; > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > /* Save the thread's msr before get_tm_stackpointer() changes it */ > - unsigned long msr = regs->msr; > -#endif > + unsigned long msr __maybe_unused = regs->msr; I don't thing __maybe_unused() is the right solution. I think MSR_TM_ACTIVE() should be fixed instead, either by changing it into a static inline function, or doing something similar to https://github.com/linuxppc/linux/commit/05a4ab823983d9136a460b7b5e0d49ee709a6f86 > > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); > if (!access_ok(frame, sizeof(*frame))) > @@ -836,8 +833,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > /* Create the ucontext. */ > err |= __put_user(0, &frame->uc.uc_flags); > err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + > if (MSR_TM_ACTIVE(msr)) { > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > /* The ucontext_t passed to userland points to the second > * ucontext_t (for transactional state) with its uc_link ptr. > */ > @@ -847,9 +845,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > tsk, ksig->sig, NULL, > (unsigned long)ksig->ka.sa.sa_handler, > msr); > - } else > #endif > - { > + } else { > err |= __put_user(0, &frame->uc.uc_link); > prepare_setup_sigcontext(tsk, 1); > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, > Christophe
On Mon Jan 11, 2021 at 7:29 AM CST, Christophe Leroy wrote: > > > Le 09/01/2021 à 04:25, Christopher M. Riedl a écrit : > > Rework the messy ifdef breaking up the if-else for TM similar to > > commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else"). > > > > Unlike that commit for ppc32, the ifdef can't be removed entirely since > > uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM. > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > --- > > arch/powerpc/kernel/signal_64.c | 17 +++++++---------- > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > > index b211a8ea4f6e..dd3787f67a78 100644 > > --- a/arch/powerpc/kernel/signal_64.c > > +++ b/arch/powerpc/kernel/signal_64.c > > @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn) > > struct pt_regs *regs = current_pt_regs(); > > struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1]; > > sigset_t set; > > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > unsigned long msr; > > -#endif > > > > /* Always make any pending restarted system calls return -EINTR */ > > current->restart_block.fn = do_no_restart_syscall; > > @@ -762,10 +760,12 @@ SYSCALL_DEFINE0(rt_sigreturn) > > * restore_tm_sigcontexts. > > */ > > regs->msr &= ~MSR_TS_MASK; > > +#endif > > > > if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR])) > > goto badframe; > > This means you are doing that __get_user() even when msr is not used. > That should be avoided. > Thanks, I moved it into the #ifdef block right above it instead for the next spin. > > if (MSR_TM_ACTIVE(msr)) { > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > /* We recheckpoint on return. */ > > struct ucontext __user *uc_transact; > > > > @@ -778,9 +778,8 @@ SYSCALL_DEFINE0(rt_sigreturn) > > if (restore_tm_sigcontexts(current, &uc->uc_mcontext, > > &uc_transact->uc_mcontext)) > > goto badframe; > > - } else > > #endif > > - { > > + } else { > > /* > > * Fall through, for non-TM restore > > * > > @@ -818,10 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > unsigned long newsp = 0; > > long err = 0; > > struct pt_regs *regs = tsk->thread.regs; > > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > /* Save the thread's msr before get_tm_stackpointer() changes it */ > > - unsigned long msr = regs->msr; > > -#endif > > + unsigned long msr __maybe_unused = regs->msr; > > I don't thing __maybe_unused() is the right solution. > > I think MSR_TM_ACTIVE() should be fixed instead, either by changing it > into a static inline > function, or doing something similar to > https://github.com/linuxppc/linux/commit/05a4ab823983d9136a460b7b5e0d49ee709a6f86 > Agreed, I'll change MSR_TM_ACTIVE() to reference its argument in the macro. This keeps it consistent with all the other MSR_TM_* macros in reg.h. Probably better than changing it to static inline since that would mean changing all the macros too which seems unecessary. > > > > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); > > if (!access_ok(frame, sizeof(*frame))) > > @@ -836,8 +833,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > /* Create the ucontext. */ > > err |= __put_user(0, &frame->uc.uc_flags); > > err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); > > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > + > > if (MSR_TM_ACTIVE(msr)) { > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > /* The ucontext_t passed to userland points to the second > > * ucontext_t (for transactional state) with its uc_link ptr. > > */ > > @@ -847,9 +845,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > tsk, ksig->sig, NULL, > > (unsigned long)ksig->ka.sa.sa_handler, > > msr); > > - } else > > #endif > > - { > > + } else { > > err |= __put_user(0, &frame->uc.uc_link); > > prepare_setup_sigcontext(tsk, 1); > > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, > > > > Christophe
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index b211a8ea4f6e..dd3787f67a78 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn) struct pt_regs *regs = current_pt_regs(); struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1]; sigset_t set; -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM unsigned long msr; -#endif /* Always make any pending restarted system calls return -EINTR */ current->restart_block.fn = do_no_restart_syscall; @@ -762,10 +760,12 @@ SYSCALL_DEFINE0(rt_sigreturn) * restore_tm_sigcontexts. */ regs->msr &= ~MSR_TS_MASK; +#endif if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR])) goto badframe; if (MSR_TM_ACTIVE(msr)) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM /* We recheckpoint on return. */ struct ucontext __user *uc_transact; @@ -778,9 +778,8 @@ SYSCALL_DEFINE0(rt_sigreturn) if (restore_tm_sigcontexts(current, &uc->uc_mcontext, &uc_transact->uc_mcontext)) goto badframe; - } else #endif - { + } else { /* * Fall through, for non-TM restore * @@ -818,10 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, unsigned long newsp = 0; long err = 0; struct pt_regs *regs = tsk->thread.regs; -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM /* Save the thread's msr before get_tm_stackpointer() changes it */ - unsigned long msr = regs->msr; -#endif + unsigned long msr __maybe_unused = regs->msr; frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); if (!access_ok(frame, sizeof(*frame))) @@ -836,8 +833,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, /* Create the ucontext. */ err |= __put_user(0, &frame->uc.uc_flags); err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM + if (MSR_TM_ACTIVE(msr)) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM /* The ucontext_t passed to userland points to the second * ucontext_t (for transactional state) with its uc_link ptr. */ @@ -847,9 +845,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, tsk, ksig->sig, NULL, (unsigned long)ksig->ka.sa.sa_handler, msr); - } else #endif - { + } else { err |= __put_user(0, &frame->uc.uc_link); prepare_setup_sigcontext(tsk, 1); err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
Rework the messy ifdef breaking up the if-else for TM similar to commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else"). Unlike that commit for ppc32, the ifdef can't be removed entirely since uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM. Signed-off-by: Christopher M. Riedl <cmr@codefail.de> --- arch/powerpc/kernel/signal_64.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)