Re: [PATCH v8 4/6] MCS Lock: Move mcs_lock/unlock function into its own

[Tim Chen <tim.c.chen@linux.intel.com>]

On Tue, 2014-01-21 at 20:06 +0100, Ingo Molnar wrote:
> * Tim Chen <tim.c.chen@linux.intel.com> wrote:
> 
> > On Tue, 2014-01-21 at 11:41 +0100, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > On Mon, Jan 20, 2014 at 05:24:31PM -0800, Tim Chen wrote:
> > > > > +EXPORT_SYMBOL_GPL(mcs_spin_lock);
> > > > > +EXPORT_SYMBOL_GPL(mcs_spin_unlock);
> > > > 
> > > > Do we really need the EXPORTs? The only user so far is mutex and that's
> > > > core code. The other planned users are rwsems and rwlocks, for both it
> > > > would be in the slow path, which is also core code.
> > > >
> > > > We should generally only add EXPORTs once theres a need.
> > > 
> > > In fact I'd argue the hot path needs to be inlined.
> > > 
> > > We only don't inline regular locking primitives because it would blow 
> > > up the kernel's size in too many critical places.
> > > 
> > > But inlining an _internal_ locking implementation used in just a 
> > > handful of places is a no-brainer...
> > 
> > The original mspin_lock primitive from which mcs_spin_lock was 
> > derived has an explicit noinline annotation.  The comment says that 
> > it is so that perf can properly account for time spent in the lock 
> > function.  So it wasn't inlined in previous kernels when we started.
> 
> Not sure what comment that was, but it's not a valid argument: 
> profiling and measurement is in almost all cases secondary to any 
> performance considerations!
> 
> If we keep it out of line then we want to do it only if it's faster 
> that way.
> 
> > For the time being, I'll just remove the EXPORT.  If people feel 
> > that inline is the right way to go, then we'll leave the function in 
> > mcs_spin_lock.h and not create mcs_spin_lock.c.
> 
> Well, 'people' could be you, the person touching the code? This is 
> really something that is discoverable: look at the critical path in 
> the inlined and the out of line case, and compare the number of 
> instructions. This can be done based on disassembly of the affected 
> code.

Okay, will make it inline function and drop the move of
to mcs_spin_lock.c

Tim
> 
> Thanks,
> 
> 	Ingo


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>