Messages in this thread | | | Date | Thu, 12 Jun 2014 23:40:07 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: safety of *mutex_unlock() (Was: [BUG] signal: sighand unprotected when accessed by /proc) |
| |
On Thu, 12 Jun 2014, Paul E. McKenney wrote: > On Thu, Jun 12, 2014 at 07:28:44PM +0200, Oleg Nesterov wrote: > > On 06/11, Paul E. McKenney wrote: > > > > > > On Wed, Jun 11, 2014 at 07:59:34PM +0200, Oleg Nesterov wrote: > > > > On 06/11, Paul E. McKenney wrote: > > > > > > > > > > I was thinking of ->boost_completion as the way to solve it easily, but > > > > > what did you have in mind? > > > > > > > > I meant, rcu_boost() could probably just do "mtx->owner = t", we know that > > > > it was unlocked by us and nobody else can use it until we set > > > > t->rcu_boost_mutex. > > > > > > My concern with this is that rcu_read_unlock_special() could hypothetically > > > get preempted (either by kernel or hypervisor), so that it might be a long > > > time until it makes its reference. But maybe that reference would be > > > harmless in this case. > > > > Confused... Not sure I understand what did you mean, and certainly I do not > > understand how this connects to the proxy-locking method. > > > > Could you explain? > > Here is the hypothetical sequence of events, which cannot happen unless > the CPU releasing the lock accesses the structure after releasing > the lock: > > CPU 0 CPU 1 (booster) > > releases boost_mutex > > acquires boost_mutex > releases boost_mutex > post-release boost_mutex access? > Loops to next task to boost > proxy-locks boost_mutex > > post-release boost_mutex access: > confused due to proxy-lock > operation? > > Now maybe this ends up being safe, but it sure feels like an accident > waiting to happen. Some bright developer comes up with a super-fast > handoff, and blam, RCU priority boosting takes it in the shorts. ;-) > In contrast, using the completion prevents this. > > > > > And if we move it into rcu_node, then we can probably kill ->rcu_boost_mutex, > > > > rcu_read_unlock_special() could check rnp->boost_mutex->owner == current. > > > > > > If this was anywhere near a hot code path, I would be sorely tempted. > > > > Ah, but I didn't mean perfomance. I think it is always good to try to remove > > something from task_struct, it is huge. I do not mean sizeof() in the first > > place, the very fact that I can hardly understand the purpose of a half of its > > members makes me sad ;) > > Now -that- just might make a huge amount of sense! Let's see... > > o We hold the rcu_node structure's ->lock when checking the owner > (looks like rt_mutex_owner() is the right API). > > o We hold the rcu_node structure's ->lock when doing > rt_mutex_init_proxy_locked(). > > o We -don't- hold ->lock when releasing the rt_mutex, but that > should be OK: The owner is releasing it, and it is going to > not-owned, so no other task can possibly see ownership moving > to/from them. > > o The rcu_node structure grows a bit, but not enough to worry > about, and on most systems, the decrease in task_struct size > will more than outweigh the increase in rcu_node size. > > Looks quite promising, how about the following? (Hey, it builds, so it > must be correct, right?)
True. Why should we have users if we would test the crap we produce?
Just FYI, I have a patch pending which makes the release safe.
http://marc.info/?l=linux-kernel&m=140251240630730&w=2
Thanks,
tglx
| |