Messages in this thread | | | Date | Wed, 24 Sep 2014 01:36:46 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 1/2] signal: simplify deadlock-avoidance in lock_task_sighand() |
| |
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 <oleg@redhat.com> 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. >
| |