Re: [PATCH] sched; Simplify mutex_spin_on_owner()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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@linutronix.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
>