linux-kernel - Re: [patch V4 02/10] rtmutex: Simplify rtmutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140613115809.2b032402@gandalf.local.home>
Date:	Fri, 13 Jun 2014 11:58:09 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [patch V4 02/10] rtmutex: Simplify rtmutex_slowtrylock()

On Wed, 11 Jun 2014 18:44:04 -0000
Thomas Gleixner <tglx@...utronix.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@...hat.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.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@...dmis.org>

-- Steve

>  
>  	raw_spin_unlock(&lock->wait_lock);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/