lkml.org 
[lkml]   [2014]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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);
>
>



\
 
 \ /
  Last update: 2014-06-13 19:21    [W:0.749 / U:1.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site