Re: [PATCH][RFC]: mutex: adaptive spin

[Linus Torvalds]

On Tue, 6 Jan 2009, Peter Zijlstra wrote:
> 
> One of the things the previous patch did wrong was that it never tracked
> the owner in the mutex fast path -- I initially didn't notice because I
> had all debugging infrastructure enabled, and that short circuits all
> the fast paths.

Why even worry?

Just set the owner non-atomically. We can consider a NULL owner in the 
slow-path to be a "let's schedule" case. After all, it's just a 
performance tuning thing after all, and the window is going to be very 
small, so it won't even be relevant from a performance tuning angle.

So there's _no_ reason why the fast-path can't just set owner, and no 
reason to disable preemption.

I also think the whole preempt_disable/enable around the 
mutex_spin_or_schedule() is pure garbage.

In fact, I suspect that's the real bug you're hitting: you're enabling 
preemption while holding a spinlock. That is NOT a good idea.

So 

 - remove all the "preempt_disable/enable" crud. It's wrong.

 - remove the whole

	+       if (!owner)
	+               return;

   thing. It's also wrong. Just go to the schedule case for that.

It all looks like unnecessary complexity that you added, and I think 
_that_ is what bites you.

Aim for _simple_. Not clever. Not complex. What you should aim for is to 
keep the _exact_ same code that we had before in mutex, and the absolute 
*only* change is replacing that "unlock+schedule()+relock" sequence with 
"mutex_spin_or_schedule()".

That should be your starting point.

Yeah, you'll need to set the owner for the fast case, but there are no 
locking or preemption issues there. Just forget them. You're confusing 
yourself and the code by trying to look for problems that aren't 
relevant.

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