Messages in this thread Patch in this message | | | Date | Fri, 13 Jun 2014 11:58:09 -0400 | From | Steven Rostedt <> | Subject | Re: [patch V4 02/10] rtmutex: Simplify rtmutex_slowtrylock() |
| |
On Wed, 11 Jun 2014 18:44:04 -0000 Thomas Gleixner <tglx@linutronix.de> wrote:
> Oleg noticed that rtmutex_slowtrylock() has a pointless check for > rt_mutex_owner(lock) != current. > > To avoid calling try_to_take_rtmutex() we really want to check whether > the lock has an owner at all or whether the trylock failed because the > owner is NULL, but the RT_MUTEX_HAS_WAITERS bit is set. This covers > the lock is owned by caller situation as well. > > We can actually do this check lockless. trylock is taking a chance > whether we take lock->wait_lock to do the check or not. > > Add comments to the function while at it. > > Reported-by: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/locking/rtmutex.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > Index: tip/kernel/locking/rtmutex.c > =================================================================== > --- tip.orig/kernel/locking/rtmutex.c > +++ tip/kernel/locking/rtmutex.c > @@ -963,22 +963,32 @@ rt_mutex_slowlock(struct rt_mutex *lock, > /* > * Slow path try-lock function: > */ > -static inline int > -rt_mutex_slowtrylock(struct rt_mutex *lock) > +static inline int rt_mutex_slowtrylock(struct rt_mutex *lock) > { > - int ret = 0; > + int ret; > > + /* > + * trylock is taking a chance. So we dont have to take > + * @lock->wait_lock to figure out whether @lock has a real or
"whether @lock has a real"
real what?
> + * if @lock owner is NULL and the RT_MUTEX_HAS_WAITERS bit is > + * set.
I don't understand the above. As rt_mutex_owner() will ignore the RT_MUTEX_HAS_WAITERS bit.
I think a simple comment is good enough:
/* * If the lock already has an owner we fail to get the lock. * This can be done without taking the @lock->wait_lock as * it is only being read, and this is a trylock anyway. > + */ > + if (rt_mutex_owner(lock)) > + return 0; > + > + /* > + * The mutex has currently no owner. Lock the wait lock and > + * try to acquire the lock. > + */ > raw_spin_lock(&lock->wait_lock); > > - if (likely(rt_mutex_owner(lock) != current)) { > + ret = try_to_take_rt_mutex(lock, current, NULL); > > - ret = try_to_take_rt_mutex(lock, current, NULL); > - /* > - * try_to_take_rt_mutex() sets the lock waiters > - * bit unconditionally. Clean this up. > - */ > - fixup_rt_mutex_waiters(lock); > - } > + /* > + * try_to_take_rt_mutex() sets the lock waiters bit > + * unconditionally. Clean this up. > + */ > + fixup_rt_mutex_waiters(lock);
Rest looks good.
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
> > raw_spin_unlock(&lock->wait_lock); > >
| |