Re: [PATCH] sched: remove unreliable pointer in mutex_spin_on_owner()

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, Jun 08, 2011 at 08:49:53PM +0800, Hillf Danton wrote:
> The dereference of unreliable owner pointer is unnecessary in owner_running(),
> though under RCU protection, because the true result is only determined by
> checking the validity of lock owner, as the comment says, due to likely heavy
> lock contention, which has little to do with whether owner->on_cpu is false.
> 
> If owner->on_cpu is really false, only the owner_running loop is shortened,
> but also returns incorrect result, since the lock owner is not changed, though
> maybe changed soon.
> 

Hillf, have you read anything that I posted before?

We don't want to spin if the owner of the lock sleeps. If it sleeps,
then the owner's on_cpu will be zero. That's the point of checking it.

The patch you just added would devastate the performance of the system.
Now if we have contention on a lock, and the owner sleeps, we continue
to spin. If the spinner is an RT task, this could also cause a deadlock,
especially if the owner is bound to the same CPU that the RT task is on.

-- Steve


> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>  kernel/sched.c |   36 +-----------------------------------
>  1 files changed, 1 insertions(+), 35 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index fd18f39..2c32616 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -4293,52 +4293,18 @@ EXPORT_SYMBOL(schedule);
> 
>  #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> 
> -static inline bool owner_running(struct mutex *lock, struct task_struct *owner)
> -{
> -	bool ret = false;
> -
> -	rcu_read_lock();
> -	if (lock->owner != owner)
> -		goto fail;
> -
> -	/*
> -	 * Ensure we emit the owner->on_cpu, dereference _after_ checking
> -	 * lock->owner still matches owner, if that fails, owner might
> -	 * point to free()d memory, if it still matches, the rcu_read_lock()
> -	 * ensures the memory stays valid.
> -	 */
> -	barrier();
> -
> -	ret = owner->on_cpu;
> -fail:
> -	rcu_read_unlock();
> -
> -	return ret;
> -}
> -
> -/*
> - * Look out! "owner" is an entirely speculative pointer
> - * access and not reliable.
> - */
>  int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
>  {
>  	if (!sched_feat(OWNER_SPIN))
>  		return 0;
> 
> -	while (owner_running(lock, owner)) {
> +	while (lock->owner != NULL) {
>  		if (need_resched())
>  			return 0;
> 
>  		arch_mutex_cpu_relax();
>  	}
> 
> -	/*
> -	 * If the owner changed to another task there is likely
> -	 * heavy contention, stop spinning.
> -	 */
> -	if (lock->owner)
> -		return 0;
> -
>  	return 1;
>  }
>  #endif
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/