[PATCH] powerpc/traps: Avoid rate limit messages from show unhandled signals

[PATCH] powerpc/traps: Avoid rate limit messages from show unhandled signals

[Michael Ellerman]

In the recent commit to add an explicit ratelimit state when showing
unhandled signals, commit 35a52a10c3ac ("powerpc/traps: Use an
explicit ratelimit state for show_signal_msg()"), I put the check of
show_unhandled_signals and the ratelimit state before the call to
unhandled_signal() so as to avoid unnecessarily calling the latter
when show_unhandled_signals is false.

However that causes us to check the ratelimit state on every call, so
if we take a lot of *handled* signals that has the effect of making
the ratelimit code print warnings that callbacks have been suppressed
when they haven't.

So rearrange the code so that we check show_unhandled_signals first,
then call unhandled_signal() and finally check the ratelimit state.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/traps.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 070e96f1773a..c85adb858271 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -315,22 +315,21 @@ void user_single_step_siginfo(struct task_struct *tsk,
 	info->si_addr = (void __user *)regs->nip;
 }
 
-static bool show_unhandled_signals_ratelimited(void)
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+			    unsigned long addr)
 {
 	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
-	return show_unhandled_signals && __ratelimit(&rs);
-}
 
-static void show_signal_msg(int signr, struct pt_regs *regs, int code,
-			    unsigned long addr)
-{
-	if (!show_unhandled_signals_ratelimited())
+	if (!show_unhandled_signals)
 		return;
 
 	if (!unhandled_signal(current, signr))
 		return;
 
+	if (!__ratelimit(&rs))
+		return;
+
 	pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
 		current->comm, current->pid, signame(signr), signr,
 		addr, regs->nip, regs->link, code);
-- 
2.17.1

Re: [PATCH] powerpc/traps: Avoid rate limit messages from show unhandled signals

[Murilo Opsfelder Araujo]

Hi, Michael.

On Fri, Aug 17, 2018 at 04:55:00PM +1000, Michael Ellerman wrote:
> In the recent commit to add an explicit ratelimit state when showing
> unhandled signals, commit 35a52a10c3ac ("powerpc/traps: Use an
> explicit ratelimit state for show_signal_msg()"), I put the check of
> show_unhandled_signals and the ratelimit state before the call to
> unhandled_signal() so as to avoid unnecessarily calling the latter
> when show_unhandled_signals is false.
> 
> However that causes us to check the ratelimit state on every call, so
> if we take a lot of *handled* signals that has the effect of making
> the ratelimit code print warnings that callbacks have been suppressed
> when they haven't.
> 
> So rearrange the code so that we check show_unhandled_signals first,
> then call unhandled_signal() and finally check the ratelimit state.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Nice catch.  Thanks for patching it.

Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

> ---
>  arch/powerpc/kernel/traps.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 070e96f1773a..c85adb858271 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -315,22 +315,21 @@ void user_single_step_siginfo(struct task_struct *tsk,
>  	info->si_addr = (void __user *)regs->nip;
>  }
>  
> -static bool show_unhandled_signals_ratelimited(void)
> +static void show_signal_msg(int signr, struct pt_regs *regs, int code,
> +			    unsigned long addr)
>  {
>  	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
>  				      DEFAULT_RATELIMIT_BURST);
> -	return show_unhandled_signals && __ratelimit(&rs);
> -}
>  
> -static void show_signal_msg(int signr, struct pt_regs *regs, int code,
> -			    unsigned long addr)
> -{
> -	if (!show_unhandled_signals_ratelimited())
> +	if (!show_unhandled_signals)
>  		return;
>  
>  	if (!unhandled_signal(current, signr))
>  		return;
>  
> +	if (!__ratelimit(&rs))
> +		return;
> +
>  	pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
>  		current->comm, current->pid, signame(signr), signr,
>  		addr, regs->nip, regs->link, code);
> -- 
> 2.17.1
> 

-- 
Murilo

Re: powerpc/traps: Avoid rate limit messages from show unhandled signals

[Michael Ellerman]

On Fri, 2018-08-17 at 06:55:00 UTC, Michael Ellerman wrote:
> In the recent commit to add an explicit ratelimit state when showing
> unhandled signals, commit 35a52a10c3ac ("powerpc/traps: Use an
> explicit ratelimit state for show_signal_msg()"), I put the check of
> show_unhandled_signals and the ratelimit state before the call to
> unhandled_signal() so as to avoid unnecessarily calling the latter
> when show_unhandled_signals is false.
> 
> However that causes us to check the ratelimit state on every call, so
> if we take a lot of *handled* signals that has the effect of making
> the ratelimit code print warnings that callbacks have been suppressed
> when they haven't.
> 
> So rearrange the code so that we check show_unhandled_signals first,
> then call unhandled_signal() and finally check the ratelimit state.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

Applied to powerpc next.

https://git.kernel.org/powerpc/c/997dd26cb3c8b7c9b8765751cc1491

cheers