Re: [PATCH] signal: Simplify __lock_task_sighand()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, May 05, 2014 at 08:53:08PM +0200, Oleg Nesterov wrote:
> On 05/05, Oleg Nesterov wrote:
> >
> > On 05/05, Paul E. McKenney wrote:
> > >
> > > Does the patch below cover it?
> >
> > Yes, thanks.
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> 
> Yes, but please consider the cleanup below, on top of your change.
> 
> This is subjective of course, but imho the code looks better without
> the extra unlock/restore inside the loop.

My only concern is that this might degrade real-time latency, but that
mmight just be my paranoia speaking.  Adding Steven, Sebastian, and
Thomas on CC for their thoughts.

Other than that possible issue, I do agree that your change makes the
code simpler.

							Thanx, Paul

> -------------------------------------------------------------------------------
> Subject: [PATCH] signal: Simplify __lock_task_sighand()
> 
> __lock_task_sighand() does local_irq_save() to prevent the potential
> deadlock, we can use preempt_disable() with the same effect. And in
> this case we can do preempt_disable/enable + rcu_read_lock/unlock only
> once outside of the main loop and simplify the code. Also shaves 112
> bytes from signal.o.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c |   31 +++++++++++++------------------
>  1 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4368370..03a0fd4 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1260,30 +1260,25 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
>  					   unsigned long *flags)
>  {
>  	struct sighand_struct *sighand;
> -
> +	/*
> +	 * We are going to do rcu_read_unlock() under spin_lock_irqsave().
> +	 * Make sure we can not be preempted after rcu_read_unlock(), see
> +	 * rcu_read_unlock comment header for details.
> +	 */
> +	preempt_disable();
> +	rcu_read_lock();
>  	for (;;) {
> -		/*
> -		 * Disable interrupts early to avoid deadlocks.
> -		 * See rcu_read_unlock comment header for details.
> -		 */
> -		local_irq_save(*flags);
> -		rcu_read_lock();
>  		sighand = rcu_dereference(tsk->sighand);
> -		if (unlikely(sighand == NULL)) {
> -			rcu_read_unlock();
> -			local_irq_restore(*flags);
> +		if (unlikely(sighand == NULL))
>  			break;
> -		}
> 
> -		spin_lock(&sighand->siglock);
> -		if (likely(sighand == tsk->sighand)) {
> -			rcu_read_unlock();
> +		spin_lock_irqsave(&sighand->siglock, *flags);
> +		if (likely(sighand == tsk->sighand))
>  			break;
> -		}
> -		spin_unlock(&sighand->siglock);
> -		rcu_read_unlock();
> -		local_irq_restore(*flags);
> +		spin_unlock_irqrestore(&sighand->siglock, *flags);
>  	}
> +	rcu_read_unlock();
> +	preempt_enable();
> 
>  	return sighand;
>  }
> -- 
> 1.5.5.1
> 
>