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 <o...@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 <o...@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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
