linux-kernel - Re: [PATCH] sched; Simplify mutex_spin_on

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]

Date:	Fri, 10 Jun 2011 18:04:24 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] sched; Simplify mutex_spin_on_owner()

On Fri, Jun 10, 2011 at 03:08:55PM +0200, Thomas Gleixner wrote:
> It does not make sense to rcu_read_lock/unlock() in every loop
> iteration while spinning on the mutex.
> 
> Move the rcu protection once outside the loop. Also simplify the
> return path to always check for lock->owner == NULL which meets the
> requirements of both owner changed and need_resched() caused loop
> exits.

Interesting.  If the spin was preempted in the new form, then
RCU priority boosting would boost the priority of the task spinning
on the mutex.  My guess is that this would happen rarely enough
to not be a problem, but other thoughts?

							Thanx, Paul

> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  kernel/sched.c |   25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c
> +++ linux-2.6/kernel/sched.c
> @@ -4306,11 +4306,8 @@ EXPORT_SYMBOL(schedule);
> 
>  static inline bool owner_running(struct mutex *lock, struct task_struct *owner)
>  {
> -	bool ret = false;
> -
> -	rcu_read_lock();
>  	if (lock->owner != owner)
> -		goto fail;
> +		return false;
> 
>  	/*
>  	 * Ensure we emit the owner->on_cpu, dereference _after_ checking
> @@ -4320,11 +4317,7 @@ static inline bool owner_running(struct 
>  	 */
>  	barrier();
> 
> -	ret = owner->on_cpu;
> -fail:
> -	rcu_read_unlock();
> -
> -	return ret;
> +	return owner->on_cpu;
>  }
> 
>  /*
> @@ -4336,21 +4329,21 @@ int mutex_spin_on_owner(struct mutex *lo
>  	if (!sched_feat(OWNER_SPIN))
>  		return 0;
> 
> +	rcu_read_lock();
>  	while (owner_running(lock, owner)) {
>  		if (need_resched())
> -			return 0;
> +			break;
> 
>  		arch_mutex_cpu_relax();
>  	}
> +	rcu_read_unlock();
> 
>  	/*
> -	 * If the owner changed to another task there is likely
> -	 * heavy contention, stop spinning.
> +	 * We break out the loop above on need_resched() and when the
> +	 * owner changed, which is a sign for heavy contention. Return
> +	 * success only when lock->owner is NULL.
>  	 */
> -	if (lock->owner)
> -		return 0;
> -
> -	return 1;
> +	return lock->owner == NULL;
>  }
>  #endif
> 
--
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/