Re: [PATCH 1/2] drm: make idr_mutex a spinlock

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Aug 29, 2014 at 03:03:58PM +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Aug 29, 2014 at 2:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Aug 29, 2014 at 02:01:00PM +0200, David Herrmann wrote:
> >> There is no reason to use a heavy mutex for idr protection. Use a spinlock
> >> and make idr-allocation use idr_preload().
> >>
> >> This patch also makes mode-object lookup irq-save, in case you ever wanna
> >> lookup modeset objects from interrupts. This is just a side-effect of
> >> avoiding a mutex.
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >
> > I've thought irqsave/restore are terribly serializing instructions, so
> > this might actually be slower than a plain mutex. And imo if it doesn't
> > show up in profiles it's not worth to optimize it - generally mutexes are
> > really fast and in most cases already nicely degenerate to spinlocks
> > anyway.
> 
> Honestly, this patch is less about speed than 'correctness'. Sure, a
> mutex is just a spin-lock in low-contention cases and there really is
> no high-contention here. However, spin-locks are the major lock-type
> for pure data. mutexes only make sense when you have to lock data
> structures _while_ performing operations that can sleep. Using a
> spin-lock here prevents people from doing stupid things while holding
> this lock. And really, this lock is about ID registration and
> deregistration, nothing else.
> 
> Btw., I can happily turn all those lock/unlock sequences into
> spin_lock() and spin_unlock() so we ignore irq-contexts completely, if
> that's the only issue.

Yeah, if you want I'll r-b plain spinlocks. Imo locks also serve as
documentation, so making it clear that we neither allocate nor sleep while
holding them is good. But making it irq save just because is imo
counterproductive.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch