Re: [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 29/09/2020 à 04:55, Christopher M. Riedl a écrit :
> On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
>> Change those two functions to be used within a user access block.
>>
>> For that, change save_general_regs() to and unsafe_save_general_regs(),
>> then replace all user accesses by unsafe_ versions.
>>
>> This series leads to a reduction from 2.55s to 1.73s of
>> the system CPU time with the following microbench app
>> on an mpc832x with KUAP (approx 32%)
>>
>> Without KUAP, the difference is in the noise.
>>
>> void sigusr1(int sig) { }
>>
>> int main(int argc, char **argv)
>> {
>> int i = 100000;
>>
>> signal(SIGUSR1, sigusr1);
>> for (;i--;)
>> raise(SIGUSR1);
>> exit(0);
>> }
>>
>> An additional 0.10s reduction is achieved by removing
>> CONFIG_PPC_FPU, as the mpc832x has no FPU.
>>
>> A bit less spectacular on an 8xx as KUAP is less heavy, prior to
>> the series (with KUAP) it ran in 8.10 ms. Once applies the removal
>> of FPU regs handling, we get 7.05s. With the full series, we get 6.9s.
>> If artificially re-activating FPU regs handling with the full series,
>> we get 7.6s.
>>
>> So for the 8xx, the removal of the FPU regs copy is what makes the
>> difference, but the rework of handle_signal also have a benefit.
>>
>> Same as above, without KUAP the difference is in the noise.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/kernel/signal_32.c | 224 ++++++++++++++++----------------
>> 1 file changed, 111 insertions(+), 113 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal_32.c
>> b/arch/powerpc/kernel/signal_32.c
>> index 86539a4e0514..f795fe0240a1 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -93,8 +93,8 @@ static inline int get_sigset_t(sigset_t *set,
>> #define to_user_ptr(p) ptr_to_compat(p)
>> #define from_user_ptr(p) compat_ptr(p)
>>   
>> -static inline int save_general_regs(struct pt_regs *regs,
>> - struct mcontext __user *frame)
>> +static __always_inline int
>> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
>> *frame)
>> {
>> elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
>> int val, i;
>> @@ -108,10 +108,12 @@ static inline int save_general_regs(struct pt_regs
>> *regs,
>> else
>> val = gregs[i];
>>   
>> - if (__put_user(val, &frame->mc_gregs[i]))
>> - return -EFAULT;
>> + unsafe_put_user(val, &frame->mc_gregs[i], failed);
>> }
>> return 0;
>> +
>> +failed:
>> + return 1;
>> }
>>   
>> static inline int restore_general_regs(struct pt_regs *regs,
>> @@ -148,11 +150,15 @@ static inline int get_sigset_t(sigset_t *set,
>> const sigset_t __user *uset)
>> #define to_user_ptr(p) ((unsigned long)(p))
>> #define from_user_ptr(p) ((void __user *)(p))
>>   
>> -static inline int save_general_regs(struct pt_regs *regs,
>> - struct mcontext __user *frame)
>> +static __always_inline int
>> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
>> *frame)
>> {
>> WARN_ON(!FULL_REGS(regs));
>> - return __copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE);
>> + unsafe_copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE, failed);
>> + return 0;
>> +
>> +failed:
>> + return 1;
>> }
>>   
>> static inline int restore_general_regs(struct pt_regs *regs,
>> @@ -170,6 +176,11 @@ static inline int restore_general_regs(struct
>> pt_regs *regs,
>> }
>> #endif
>>   
>> +#define unsafe_save_general_regs(regs, frame, label) do { \
>> + if (save_general_regs_unsafe(regs, frame)) \
> 
> Minor nitpick (sorry); this naming seems a bit strange to me, in x86 it
> is "__unsafe_" as a prefix instead of "_unsafe" as a suffix. That sounds
> a bit better to me, what do you think? Unless there is some convention I
> am not aware of here apart from "unsafe_" using a goto label for errors.

You are probably right, I have never been good at naming stuff.

Christophe