On Tue, Sep 23, 2014 at 09:03:48PM +0200, Oleg Nesterov wrote:
> On 09/22, Steven Rostedt wrote:
> >
> > On Mon, 22 Sep 2014 21:11:30 +0200
> > Oleg Nesterov <[email protected]> wrote:
> >
> > > > > @@ -1261,30 +1261,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_lock(), see
> > > > > + * rcu_read_unlock() comment header for details.
> > > > > + */
> > > > > + preempt_disable();
> > > >
> > > > The sad part is, this is going to break -rt.
> > >
> > > Hmm, why??
> >
> > Because in -rt, siglock is a mutex.
>
> Yes, thanks... I thougt that -rt should handle this somehow, we have
> more examples of preempt_disable() + spin_lock().
>
> OK, let's forger this patch. It was supposed to be a cleanup, it should
> not disturb -rt.
>
> > > In fact this deadlock is not really possible in any case, scheduler locks
> > > should be fine under ->siglock (for example, signal_wake_up() is called
> > > under this lock).
> > >
> > > But, the comment above rcu_read_unlock() says:
> > >
> > > Given that the set of locks acquired by rt_mutex_unlock() might change
> > > at any time, a somewhat more future-proofed approach is to make sure
> > > that that preemption never happens ...
> >
> > Hmm, I'm not sure we need to worry about this. As in -rt siglock is a
> > mutex, which is rt_mutex() itself, I highly doubt we will have
> > rt_mutex_unlock() grab siglock, otherwise that would cause havoc in -rt.
>
> Yes. And, the changelog in a841796f "signal: align __lock_task_sighand() irq
> disabling and RCU" says:
>
> It is therefore possible that this RCU read-side critical
> section will be preempted and later RCU priority boosted, which means
> that rcu_read_unlock() will call rt_mutex_unlock() in order to deboost
> itself, but with interrupts disabled. This results in lockdep splats
> ...
> It is quite possible that a better long-term fix is to make
> rt_mutex_unlock()
> disable irqs when acquiring the rt_mutex structure's ->wait_lock.
>
> but this doesn't look right, raw_spin_lock(&lock->wait_lock) should be
> fine with irqs disabled or I am totally confused. rt_mutex_adjust_prio()
> does _irqsave/irqrestore, so this can't enable interrupts.
>
> Paul, will you agree if we turn it into
If you guys continue the guarantee of no deadlock, I am OK with this change.
Thanx, Paul
> struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
> unsigned long *flags)
> {
> struct sighand_struct *sighand;
>
> rcu_read_lock();
> for (;;) {
> sighand = rcu_dereference(tsk->sighand);
> if (unlikely(sighand == NULL))
> break;
>
> spin_lock_irqsave(&sighand->siglock, *flags);
> if (likely(sighand == tsk->sighand))
> break;
> spin_unlock_irqrestore(&sighand->siglock, *flags);
> }
> /*
> * On the succesfull return we hold ->siglock. According to comment
> * above rcu_read_unlock() this is against the rules, but scheduler
> * locks are fine under this lock, signal_wake_up() takes them too.
> */
> rcu_read_unlock();
>
> return sighand;
> }
>
> ?
>
> Or I can leave this code alone, this is the minor cleanup. Just to me this
> sequence
>
> local_irq_save();
> rcu_read_lock();
> spin_lock();
>
> looks a bit confusing/annoying even with the comment.
>
> Oleg.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
