From: Oleg Nesterov <oleg@redhat.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
linux-kernel@vger.kernel.org
Subject: Re: lock_task_sighand() && rcu_boost()
Date: Sun, 4 May 2014 21:17:57 +0200 [thread overview]
Message-ID: <20140504191757.GA11319@redhat.com> (raw)
In-Reply-To: <20140504180145.GC8754@linux.vnet.ibm.com>
On 05/04, Paul E. McKenney wrote:
>
> On Sat, May 03, 2014 at 06:11:33PM +0200, Oleg Nesterov wrote:
> >
> > OK, if we can't rcu_read_unlock() with irqs disabled, then we can at least
> > cleanup it (and document the problem).
>
> Just to clarify (probably unnecessarily), it is OK to invoke rcu_read_unlock()
> with irqs disabled, but only if preemption has been disabled throughout
> the entire RCU read-side critical section.
Yes, yes, I understand, thanks.
> > and add rcu_read_unlock() into unlock_task_sighand().
>
> That should also work.
OK.
> > But. I simply can't understand why lockdep should complain? Why it is bad
> > to lock/unlock ->wait_lock with irqs disabled?
>
> Well, lockdep doesn't -always- complain, and some cases are OK.
>
> The problem is that if the RCU read-side critical section has been
> preempted, and if this task gets RCU priority-boosted in the meantime,
> then the task will need to acquire scheduler rq and pi locks at
> rcu_read_unlock() time.
Yes,
> If the reason that interrupts are disabled at
> rcu_read_unlock() time is that either rq or pi locks are held (or some
> other locks are held that are normally acquired while holding rq or
> pi locks), then we can deadlock. And lockdep will of course complain.
Yes. but not in this case?
> If I recall corectly, at one point, the ->siglock lock was acquired
> while holding the rq locks, which would have resulted in lockdep
> complaints.
No, this must not be possible. signal_wake_up_state() was always called
under ->siglock and it does wake_up_state() which takes rq/pi locks.
And if lock_task_sighand() is preempted after rcu_read_lock(), then the
caller doesn't hold any lock.
So perhaps we can revert a841796f11c90d53 ?
Otherwise please see below.
> Hmmm... A better description of the bad case might be as follows:
>
> Deadlock can occur if you have an RCU read-side critical
> section that is anywhere preemptible, and where the outermost
> rcu_read_unlock() is invoked while holding and lock acquired
> by either wakeup_next_waiter() or rt_mutex_adjust_prio(),
> or while holding any lock that is ever acquired while holding
> one of those locks.
>
> Does that help?
>
> Avoiding this bad case could be a bit ugly, as it is a dynamic set
> of locks that is acquired while holding any lock acquired by either
> wakeup_next_waiter() or rt_mutex_adjust_prio(). So I simplified the
> rule by prohibiting invoking rcu_read_unlock() with irqs disabled
> if the RCU read-side critical section had ever been preemptible.
OK, if you prefer to enforce this rule even if (say) lock_task_sighand()
is fine, then it needs the comment. And a cleanup ;)
We can move rcu_read_unlock() into unlock_task_sighand() as I suggested
before, or we can simply add preempt_disable/enable into lock_(),
struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
unsigned long *flags)
{
struct sighand_struct *sighand;
/*
* COMMENT TO EXPLAIN WHY
*/
preempt_disable();
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);
}
rcu_read_unlock();
preempt_enable();
return sighand;
}
The only problem is the "COMMENT" above. Perhaps the "prohibit invoking
rcu_read_unlock() with irqs disabled if ..." rule should documented
near/above rcu_read_unlock() ? In this case that COMMENT could simply
say "see the comment above rcu_read_unlock()".
What do you think?
Oleg.
next prev parent reply other threads:[~2014-05-04 19:18 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-03 16:11 lock_task_sighand() && rcu_boost() Oleg Nesterov
2014-05-04 18:01 ` Paul E. McKenney
2014-05-04 19:17 ` Oleg Nesterov [this message]
2014-05-04 22:38 ` Paul E. McKenney
2014-05-05 13:26 ` Oleg Nesterov
2014-05-05 15:26 ` Paul E. McKenney
2014-05-05 16:47 ` Oleg Nesterov
2014-05-05 18:53 ` [PATCH] signal: Simplify __lock_task_sighand() Oleg Nesterov
2014-05-05 19:55 ` Oleg Nesterov
2014-05-05 20:56 ` Paul E. McKenney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140504191757.GA11319@redhat.com \
--to=oleg@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).