[PATCH] avr32: Don't acquire siglock when reading sighand action

[PATCH] avr32: Don't acquire siglock when reading sighand action

[Matt Fleming]

From: Matt Fleming <matt.fleming@linux.intel.com>

There's no need to acquire the siglock when simply reading the action
handler. We would need to lock it if we were modifying the handler or
we were protecting it from modification across function calls, but if
we're just reading it, there's no need to lock it

Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
 arch/avr32/kernel/traps.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/avr32/kernel/traps.c b/arch/avr32/kernel/traps.c
index b91b204..82f47e6 100644
--- a/arch/avr32/kernel/traps.c
+++ b/arch/avr32/kernel/traps.c
@@ -106,9 +106,7 @@ void _exception(long signr, struct pt_regs *regs, int code,
 	if (is_global_init(current)) {
 		__sighandler_t handler;
 
-		spin_lock_irq(&current->sighand->siglock);
 		handler = current->sighand->action[signr-1].sa.sa_handler;
-		spin_unlock_irq(&current->sighand->siglock);
 		if (handler == SIG_DFL) {
 			/* init has generated a synchronous exception
 			   and it doesn't have a handler for the signal */
-- 
1.7.4

Re: [PATCH] avr32: Don't acquire siglock when reading sighand action

[Oleg Nesterov]

On 03/30, Matt Fleming wrote:
>
> From: Matt Fleming <matt.fleming@linux.intel.com>
>
> There's no need to acquire the siglock when simply reading the action
> handler. We would need to lock it if we were modifying the handler or
> we were protecting it from modification across function calls, but if
> we're just reading it, there's no need to lock it

Agreed, ->siglock buys nothing.

But,

> --- a/arch/avr32/kernel/traps.c
> +++ b/arch/avr32/kernel/traps.c
> @@ -106,9 +106,7 @@ void _exception(long signr, struct pt_regs *regs, int code,
>  	if (is_global_init(current)) {

is_global_init() is not right in theory, /sbin/init can be multithreaded.
And, this doesn't cover the sub-namespace inits... I'd suggest to check
SIGNAL_UNKILLABLE, but looking closer I think you can simply remove this
code.

The comment says:

	/*
	 * Init gets no signals that it doesn't have a handler for.
	 * That's all very well, but if it has caused a synchronous
	 * exception and we ignore the resulting signal, it will just
	 * generate the same exception over and over again and we get
	 * nowhere.  Better to kill it and let the kernel panic.
	 */

This is correct, but please look at force_sig_info(), it already does
what you need:

	* We don't want to have recursive SIGSEGV's etc, for example,
	* that is why we also clear SIGNAL_UNKILLABLE.

Oleg.

Re: [PATCH] avr32: Don't acquire siglock when reading sighand action

[Matt Fleming]

On Sat, 2 Apr 2011 18:37:22 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 03/30, Matt Fleming wrote:
> >
> > From: Matt Fleming <matt.fleming@linux.intel.com>
> >
> > There's no need to acquire the siglock when simply reading the
> > action handler. We would need to lock it if we were modifying the
> > handler or we were protecting it from modification across function
> > calls, but if we're just reading it, there's no need to lock it
> 
> Agreed, ->siglock buys nothing.
> 
> But,
> 
> > --- a/arch/avr32/kernel/traps.c
> > +++ b/arch/avr32/kernel/traps.c
> > @@ -106,9 +106,7 @@ void _exception(long signr, struct pt_regs
> > *regs, int code, if (is_global_init(current)) {
> 
> is_global_init() is not right in theory, /sbin/init can be
> multithreaded. And, this doesn't cover the sub-namespace inits... I'd
> suggest to check SIGNAL_UNKILLABLE, but looking closer I think you
> can simply remove this code.
> 
> The comment says:
> 
> 	/*
> 	 * Init gets no signals that it doesn't have a handler for.
> 	 * That's all very well, but if it has caused a synchronous
> 	 * exception and we ignore the resulting signal, it will just
> 	 * generate the same exception over and over again and we get
> 	 * nowhere.  Better to kill it and let the kernel panic.
> 	 */
> 
> This is correct, but please look at force_sig_info(), it already does
> what you need:
> 
> 	* We don't want to have recursive SIGSEGV's etc, for example,
> 	* that is why we also clear SIGNAL_UNKILLABLE.

Aha! Yeah, you're totally correct - good spot. Interestingly, after a
bit of git archaeology it seems that this code was copied from powerpc,

commit 623b0355d5b1f9c6d05005b649a2f3a7b9fd7816
Author: Haavard Skinnemoen <hskinnemoen@atmel.com>
Date:   Tue Mar 13 17:59:11 2007 +0100

    [AVR32] Clean up exception handling code
    
      * Use generic BUG() handling
      * Remove some useless debug statements
      * Use a common function _exception() to send signals or oops when
        an exception can't be handled. This makes sure init doesn't
        enter an infinite exception loop as well. Borrowed from powerpc.
      * Add some basic exception tracing support to the page fault code.
      * Rework dump_stack(), show_regs() and friends and move everything
        into process.c
      * Print information about configuration options and chip type when
        oopsing
    
    Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>


... and Ben H deleted the copy in arch/powerpc in Nov 2009, 

commit a0592d42fe3e12966db02f5c41f1edae2e59c490
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Tue Nov 10 15:13:15 2009 +0000

    powerpc: kill the obsolete code under is_global_init()
    
    The code under "if (is_global_init())" is bogus, and is_global_init()
    itself is not right in mt case.
    
    Contrary to what the comment says, nowadays force_sig_info() does kill
    init even if the handler is SIG_DFL. Note that force_sig_info() clears
    SIGNAL_UNKILLABLE exactly for this case.
    
    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


I'll send a new patch that deletes this code for avr32.

-- 
Matt Fleming, Intel Open Source Technology Center