| Message ID | 1409313661-20696-1-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 34 ++++++++++++++++++++-------------- > include/drm/drm_crtc.h | 4 ++-- > 2 files changed, 22 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 61b6978..97eba56 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -283,8 +283,10 @@ static int drm_mode_object_get_reg(struct drm_device *dev, > { > int ret; > > - mutex_lock(&dev->mode_config.idr_mutex); > - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_KERNEL); > + idr_preload(GFP_KERNEL); > + spin_lock_irq(&dev->mode_config.idr_lock); > + > + ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); > if (ret >= 0) { > /* > * Set up the object linking under the protection of the idr > @@ -293,7 +295,9 @@ static int drm_mode_object_get_reg(struct drm_device *dev, > obj->id = ret; > obj->type = obj_type; > } > - mutex_unlock(&dev->mode_config.idr_mutex); > + > + spin_unlock_irq(&dev->mode_config.idr_lock); > + idr_preload_end(); > > return ret < 0 ? ret : 0; > } > @@ -322,9 +326,9 @@ int drm_mode_object_get(struct drm_device *dev, > static void drm_mode_object_register(struct drm_device *dev, > struct drm_mode_object *obj) > { > - mutex_lock(&dev->mode_config.idr_mutex); > + spin_lock_irq(&dev->mode_config.idr_lock); > idr_replace(&dev->mode_config.crtc_idr, obj, obj->id); > - mutex_unlock(&dev->mode_config.idr_mutex); > + spin_unlock_irq(&dev->mode_config.idr_lock); > } > > /** > @@ -339,17 +343,18 @@ static void drm_mode_object_register(struct drm_device *dev, > void drm_mode_object_put(struct drm_device *dev, > struct drm_mode_object *object) > { > - mutex_lock(&dev->mode_config.idr_mutex); > + spin_lock_irq(&dev->mode_config.idr_lock); > idr_remove(&dev->mode_config.crtc_idr, object->id); > - mutex_unlock(&dev->mode_config.idr_mutex); > + spin_unlock_irq(&dev->mode_config.idr_lock); > } > > static struct drm_mode_object *_object_find(struct drm_device *dev, > uint32_t id, uint32_t type) > { > struct drm_mode_object *obj = NULL; > + unsigned long flags; > > - mutex_lock(&dev->mode_config.idr_mutex); > + spin_lock_irqsave(&dev->mode_config.idr_lock, flags); > obj = idr_find(&dev->mode_config.crtc_idr, id); > if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type) > obj = NULL; > @@ -358,7 +363,7 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, > /* don't leak out unref'd fb's */ > if (obj && (obj->type == DRM_MODE_OBJECT_FB)) > obj = NULL; > - mutex_unlock(&dev->mode_config.idr_mutex); > + spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags); > > return obj; > } > @@ -433,9 +438,9 @@ EXPORT_SYMBOL(drm_framebuffer_init); > static void __drm_framebuffer_unregister(struct drm_device *dev, > struct drm_framebuffer *fb) > { > - mutex_lock(&dev->mode_config.idr_mutex); > + spin_lock_irq(&dev->mode_config.idr_lock); > idr_remove(&dev->mode_config.crtc_idr, fb->base.id); > - mutex_unlock(&dev->mode_config.idr_mutex); > + spin_unlock_irq(&dev->mode_config.idr_lock); > > fb->base.id = 0; > } > @@ -465,14 +470,15 @@ static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev, > { > struct drm_mode_object *obj = NULL; > struct drm_framebuffer *fb; > + unsigned long flags; > > - mutex_lock(&dev->mode_config.idr_mutex); > + spin_lock_irqsave(&dev->mode_config.idr_lock, flags); > obj = idr_find(&dev->mode_config.crtc_idr, id); > if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id)) > fb = NULL; > else > fb = obj_to_fb(obj); > - mutex_unlock(&dev->mode_config.idr_mutex); > + spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags); > > return fb; > } > @@ -5049,7 +5055,7 @@ void drm_mode_config_init(struct drm_device *dev) > { > mutex_init(&dev->mode_config.mutex); > drm_modeset_lock_init(&dev->mode_config.connection_mutex); > - mutex_init(&dev->mode_config.idr_mutex); > + spin_lock_init(&dev->mode_config.idr_lock); > mutex_init(&dev->mode_config.fb_lock); > INIT_LIST_HEAD(&dev->mode_config.fb_list); > INIT_LIST_HEAD(&dev->mode_config.crtc_list); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 77d9763..9c57b56 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -742,7 +742,7 @@ struct drm_mode_group { > /** > * drm_mode_config - Mode configuration control structure > * @mutex: mutex protecting KMS related lists and structures > - * @idr_mutex: mutex for KMS ID allocation and management > + * @idr_lock: lock for KMS ID allocation and management > * @crtc_idr: main KMS ID tracking object > * @num_fb: number of fbs available > * @fb_list: list of framebuffers available > @@ -772,7 +772,7 @@ struct drm_mode_config { > struct mutex mutex; /* protects configuration (mode lists etc.) */ > struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */ > struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ > - struct mutex idr_mutex; /* for IDR management */ > + spinlock_t idr_lock; /* for IDR management */ > struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ > /* this is limited to one for now */ > > -- > 2.1.0 >
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. Thanks David
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. I don't think that's entirely accurate. idr_preload(GFP_KERNEL) might sleep since GFP_KERNEL & __GFP_WAIT != 0. Thierry
Hi On Fri, Aug 29, 2014 at 3:10 PM, Thierry Reding <thierry.reding@gmail.com> 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. > > I don't think that's entirely accurate. idr_preload(GFP_KERNEL) might > sleep since GFP_KERNEL & __GFP_WAIT != 0. idr_preload() is only used in registration, not in lookups. Not sure what you refer to? Thanks David
On Fri, Aug 29, 2014 at 03:11:19PM +0200, David Herrmann wrote: > Hi > > On Fri, Aug 29, 2014 at 3:10 PM, Thierry Reding > <thierry.reding@gmail.com> 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. > > > > I don't think that's entirely accurate. idr_preload(GFP_KERNEL) might > > sleep since GFP_KERNEL & __GFP_WAIT != 0. > > idr_preload() is only used in registration, not in lookups. Not sure > what you refer to? Yes, you're right. I wasn't paying close enough attention and somehow applied "makes irq-safe" to the "idr_preload()" change without relating it to "lookup". Thierry
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
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 61b6978..97eba56 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -283,8 +283,10 @@ static int drm_mode_object_get_reg(struct drm_device *dev, { int ret; - mutex_lock(&dev->mode_config.idr_mutex); - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_KERNEL); + idr_preload(GFP_KERNEL); + spin_lock_irq(&dev->mode_config.idr_lock); + + ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); if (ret >= 0) { /* * Set up the object linking under the protection of the idr @@ -293,7 +295,9 @@ static int drm_mode_object_get_reg(struct drm_device *dev, obj->id = ret; obj->type = obj_type; } - mutex_unlock(&dev->mode_config.idr_mutex); + + spin_unlock_irq(&dev->mode_config.idr_lock); + idr_preload_end(); return ret < 0 ? ret : 0; } @@ -322,9 +326,9 @@ int drm_mode_object_get(struct drm_device *dev, static void drm_mode_object_register(struct drm_device *dev, struct drm_mode_object *obj) { - mutex_lock(&dev->mode_config.idr_mutex); + spin_lock_irq(&dev->mode_config.idr_lock); idr_replace(&dev->mode_config.crtc_idr, obj, obj->id); - mutex_unlock(&dev->mode_config.idr_mutex); + spin_unlock_irq(&dev->mode_config.idr_lock); } /** @@ -339,17 +343,18 @@ static void drm_mode_object_register(struct drm_device *dev, void drm_mode_object_put(struct drm_device *dev, struct drm_mode_object *object) { - mutex_lock(&dev->mode_config.idr_mutex); + spin_lock_irq(&dev->mode_config.idr_lock); idr_remove(&dev->mode_config.crtc_idr, object->id); - mutex_unlock(&dev->mode_config.idr_mutex); + spin_unlock_irq(&dev->mode_config.idr_lock); } static struct drm_mode_object *_object_find(struct drm_device *dev, uint32_t id, uint32_t type) { struct drm_mode_object *obj = NULL; + unsigned long flags; - mutex_lock(&dev->mode_config.idr_mutex); + spin_lock_irqsave(&dev->mode_config.idr_lock, flags); obj = idr_find(&dev->mode_config.crtc_idr, id); if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type) obj = NULL; @@ -358,7 +363,7 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, /* don't leak out unref'd fb's */ if (obj && (obj->type == DRM_MODE_OBJECT_FB)) obj = NULL; - mutex_unlock(&dev->mode_config.idr_mutex); + spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags); return obj; } @@ -433,9 +438,9 @@ EXPORT_SYMBOL(drm_framebuffer_init); static void __drm_framebuffer_unregister(struct drm_device *dev, struct drm_framebuffer *fb) { - mutex_lock(&dev->mode_config.idr_mutex); + spin_lock_irq(&dev->mode_config.idr_lock); idr_remove(&dev->mode_config.crtc_idr, fb->base.id); - mutex_unlock(&dev->mode_config.idr_mutex); + spin_unlock_irq(&dev->mode_config.idr_lock); fb->base.id = 0; } @@ -465,14 +470,15 @@ static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev, { struct drm_mode_object *obj = NULL; struct drm_framebuffer *fb; + unsigned long flags; - mutex_lock(&dev->mode_config.idr_mutex); + spin_lock_irqsave(&dev->mode_config.idr_lock, flags); obj = idr_find(&dev->mode_config.crtc_idr, id); if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id)) fb = NULL; else fb = obj_to_fb(obj); - mutex_unlock(&dev->mode_config.idr_mutex); + spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags); return fb; } @@ -5049,7 +5055,7 @@ void drm_mode_config_init(struct drm_device *dev) { mutex_init(&dev->mode_config.mutex); drm_modeset_lock_init(&dev->mode_config.connection_mutex); - mutex_init(&dev->mode_config.idr_mutex); + spin_lock_init(&dev->mode_config.idr_lock); mutex_init(&dev->mode_config.fb_lock); INIT_LIST_HEAD(&dev->mode_config.fb_list); INIT_LIST_HEAD(&dev->mode_config.crtc_list); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 77d9763..9c57b56 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -742,7 +742,7 @@ struct drm_mode_group { /** * drm_mode_config - Mode configuration control structure * @mutex: mutex protecting KMS related lists and structures - * @idr_mutex: mutex for KMS ID allocation and management + * @idr_lock: lock for KMS ID allocation and management * @crtc_idr: main KMS ID tracking object * @num_fb: number of fbs available * @fb_list: list of framebuffers available @@ -772,7 +772,7 @@ struct drm_mode_config { struct mutex mutex; /* protects configuration (mode lists etc.) */ struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */ struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ - struct mutex idr_mutex; /* for IDR management */ + spinlock_t idr_lock; /* for IDR management */ struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ /* this is limited to one for now */
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> --- drivers/gpu/drm/drm_crtc.c | 34 ++++++++++++++++++++-------------- include/drm/drm_crtc.h | 4 ++-- 2 files changed, 22 insertions(+), 16 deletions(-)