| Message ID | 20220906201629.419160-2-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Use full allocated minor range for DRM | expand |
On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote: > IDR is deprecated, and since XArray manages its own state with internal > locking, it simplifies the locking on DRM side. > Additionally, don't use the IRQ-safe variant, since operating on drm > minor is not done in IRQ context. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Suggested-by: Matthew Wilcox <willy@infradead.org> I have a few questions, but I like where you're going. > @@ -98,21 +98,18 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, > static void drm_minor_alloc_release(struct drm_device *dev, void *data) > { > struct drm_minor *minor = data; > - unsigned long flags; > > WARN_ON(dev != minor->dev); > > put_device(minor->kdev); > > - spin_lock_irqsave(&drm_minor_lock, flags); > - idr_remove(&drm_minors_idr, minor->index); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + xa_release(&drm_minors_xa, minor->index); Has it definitely been unused at this point? I would think that xa_erase() (an unconditional store) would be the correct function to call. > @@ -122,20 +119,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > minor->type = type; > minor->dev = dev; > > - idr_preload(GFP_KERNEL); > - spin_lock_irqsave(&drm_minor_lock, flags); > - r = idr_alloc(&drm_minors_idr, > - NULL, > - 64 * type, > - 64 * (type + 1), > - GFP_NOWAIT); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > - idr_preload_end(); > - > + r = xa_alloc(&drm_minors_xa, &id, NULL, > + XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL); > if (r < 0) > return r; > > - minor->index = r; > + minor->index = id; Wouldn't it be better to call: r = xa_alloc(&drm_minors_xa, &minor->index, NULL, XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL); I might also prefer a little syntactic sugar like: #define DRM_MINOR_LIMIT(type) XA_LIMIT(64 * (type), 64 * (type) + 63) but that's definitely a matter of taste. > @@ -172,9 +161,12 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > goto err_debugfs; > > /* replace NULL with @minor so lookups will succeed from now on */ > - spin_lock_irqsave(&drm_minor_lock, flags); > - idr_replace(&drm_minors_idr, minor, minor->index); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL); > + if (xa_is_err(entry)) { > + ret = xa_err(entry); > + goto err_debugfs; > + } > + WARN_ON(entry); Might be better as an xa_cmpxchg()? > @@ -187,16 +179,13 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > { > struct drm_minor *minor; > - unsigned long flags; > > minor = *drm_minor_get_slot(dev, type); > if (!minor || !device_is_registered(minor->kdev)) > return; > > /* replace @minor with NULL so lookups will fail from now on */ > - spin_lock_irqsave(&drm_minor_lock, flags); > - idr_replace(&drm_minors_idr, NULL, minor->index); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + xa_erase(&drm_minors_xa, minor->index); This isn't an exact replacement, but I'm not sure whether that makes a difference. xa_erase() allows allocation of this ID again while idr_replace() means that lookups return NULL, but the ID remains in use. The equivalent of idr_replace() is: xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); > @@ -215,13 +204,10 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > struct drm_minor *drm_minor_acquire(unsigned int minor_id) > { > struct drm_minor *minor; > - unsigned long flags; > > - spin_lock_irqsave(&drm_minor_lock, flags); > - minor = idr_find(&drm_minors_idr, minor_id); > + minor = xa_load(&drm_minors_xa, minor_id); > if (minor) > drm_dev_get(minor->dev); > - spin_unlock_irqrestore(&drm_minor_lock, flags); This is also not an exact equivalent as the dev_drm_get() is now outside the lock. Does that matter in this case? I don't know the code well enough to say. If you want it to be equivalent, then: xa_lock(&drm_minors_xa); minor = xa_load(&drm_minors_xa, minor_id); if (minor) drm_dev_get(minor->dev); xa_unlock(&drm_minors_xa); would be the code to use. > @@ -1037,7 +1023,7 @@ static void drm_core_exit(void) > unregister_chrdev(DRM_MAJOR, "drm"); > debugfs_remove(drm_debugfs_root); > drm_sysfs_destroy(); > - idr_destroy(&drm_minors_idr); > + xa_destroy(&drm_minors_xa); I don't know if this is the right call. xa_destroy() is the exact replacement, but if the xarray should already be empty (and it should, right?) then asserting the xa_empty() is true may be the better call to make. Phew, that was a lot of comments/questions. I hope that was useful!
On Tue, Sep 06, 2022 at 10:02:24PM +0100, Matthew Wilcox wrote: > On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote: > > IDR is deprecated, and since XArray manages its own state with internal > > locking, it simplifies the locking on DRM side. > > Additionally, don't use the IRQ-safe variant, since operating on drm > > minor is not done in IRQ context. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > I have a few questions, but I like where you're going. > > > @@ -98,21 +98,18 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, > > static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > { > > struct drm_minor *minor = data; > > - unsigned long flags; > > > > WARN_ON(dev != minor->dev); > > > > put_device(minor->kdev); > > > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - idr_remove(&drm_minors_idr, minor->index); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > + xa_release(&drm_minors_xa, minor->index); > > Has it definitely been unused at this point? I would think that > xa_erase() (an unconditional store) would be the correct function to > call. Yes, unless there's a programmers error somewhere - I'll replace it though. > > > @@ -122,20 +119,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > minor->type = type; > > minor->dev = dev; > > > > - idr_preload(GFP_KERNEL); > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - r = idr_alloc(&drm_minors_idr, > > - NULL, > > - 64 * type, > > - 64 * (type + 1), > > - GFP_NOWAIT); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > - idr_preload_end(); > > - > > + r = xa_alloc(&drm_minors_xa, &id, NULL, > > + XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL); > > if (r < 0) > > return r; > > > > - minor->index = r; > > + minor->index = id; > > Wouldn't it be better to call: > > r = xa_alloc(&drm_minors_xa, &minor->index, NULL, > XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL); > > I might also prefer a little syntactic sugar like: > > #define DRM_MINOR_LIMIT(type) XA_LIMIT(64 * (type), 64 * (type) + 63) > > but that's definitely a matter of taste. Sure. > > > @@ -172,9 +161,12 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > > goto err_debugfs; > > > > /* replace NULL with @minor so lookups will succeed from now on */ > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - idr_replace(&drm_minors_idr, minor, minor->index); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > + entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL); > > + if (xa_is_err(entry)) { > > + ret = xa_err(entry); > > + goto err_debugfs; > > + } > > + WARN_ON(entry); > > Might be better as an xa_cmpxchg()? Ack. > > > @@ -187,16 +179,13 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > > static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > > { > > struct drm_minor *minor; > > - unsigned long flags; > > > > minor = *drm_minor_get_slot(dev, type); > > if (!minor || !device_is_registered(minor->kdev)) > > return; > > > > /* replace @minor with NULL so lookups will fail from now on */ > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - idr_replace(&drm_minors_idr, NULL, minor->index); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > + xa_erase(&drm_minors_xa, minor->index); > > This isn't an exact replacement, but I'm not sure whether that makes a > difference. xa_erase() allows allocation of this ID again while > idr_replace() means that lookups return NULL, but the ID remains in > use. The equivalent of idr_replace() is: > xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); It does makes a difference, I'll change it to the equivalent. > > > @@ -215,13 +204,10 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > > struct drm_minor *drm_minor_acquire(unsigned int minor_id) > > { > > struct drm_minor *minor; > > - unsigned long flags; > > > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - minor = idr_find(&drm_minors_idr, minor_id); > > + minor = xa_load(&drm_minors_xa, minor_id); > > if (minor) > > drm_dev_get(minor->dev); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > This is also not an exact equivalent as the dev_drm_get() is now outside > the lock. Does that matter in this case? I don't know the code well > enough to say. If you want it to be equivalent, then: > > xa_lock(&drm_minors_xa); > minor = xa_load(&drm_minors_xa, minor_id); > if (minor) > drm_dev_get(minor->dev); > xa_unlock(&drm_minors_xa); > > would be the code to use. Again, it does matter, I'll change it. > > > @@ -1037,7 +1023,7 @@ static void drm_core_exit(void) > > unregister_chrdev(DRM_MAJOR, "drm"); > > debugfs_remove(drm_debugfs_root); > > drm_sysfs_destroy(); > > - idr_destroy(&drm_minors_idr); > > + xa_destroy(&drm_minors_xa); > > I don't know if this is the right call. xa_destroy() is the exact > replacement, but if the xarray should already be empty (and it should, > right?) then asserting the xa_empty() is true may be the better call > to make. Yeah - I'll replace it with WARN_ON. > > > Phew, that was a lot of comments/questions. I hope that was useful! Very useful indeed, thanks! -Michał
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8214a0b1ab7f..41799e4d0432 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -34,6 +34,7 @@ #include <linux/pseudo_fs.h> #include <linux/slab.h> #include <linux/srcu.h> +#include <linux/xarray.h> #include <drm/drm_cache.h> #include <drm/drm_client.h> @@ -53,8 +54,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl"); MODULE_DESCRIPTION("DRM shared core routines"); MODULE_LICENSE("GPL and additional rights"); -static DEFINE_SPINLOCK(drm_minor_lock); -static struct idr drm_minors_idr; +static DEFINE_XARRAY_ALLOC(drm_minors_xa); /* * If the drm core fails to init for whatever reason, @@ -98,21 +98,18 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, static void drm_minor_alloc_release(struct drm_device *dev, void *data) { struct drm_minor *minor = data; - unsigned long flags; WARN_ON(dev != minor->dev); put_device(minor->kdev); - spin_lock_irqsave(&drm_minor_lock, flags); - idr_remove(&drm_minors_idr, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + xa_release(&drm_minors_xa, minor->index); } static int drm_minor_alloc(struct drm_device *dev, unsigned int type) { struct drm_minor *minor; - unsigned long flags; + u32 id; int r; minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); @@ -122,20 +119,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) minor->type = type; minor->dev = dev; - idr_preload(GFP_KERNEL); - spin_lock_irqsave(&drm_minor_lock, flags); - r = idr_alloc(&drm_minors_idr, - NULL, - 64 * type, - 64 * (type + 1), - GFP_NOWAIT); - spin_unlock_irqrestore(&drm_minor_lock, flags); - idr_preload_end(); - + r = xa_alloc(&drm_minors_xa, &id, NULL, + XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL); if (r < 0) return r; - minor->index = r; + minor->index = id; r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); if (r) @@ -152,7 +141,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) static int drm_minor_register(struct drm_device *dev, unsigned int type) { struct drm_minor *minor; - unsigned long flags; + void *entry; int ret; DRM_DEBUG("\n"); @@ -172,9 +161,12 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) goto err_debugfs; /* replace NULL with @minor so lookups will succeed from now on */ - spin_lock_irqsave(&drm_minor_lock, flags); - idr_replace(&drm_minors_idr, minor, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL); + if (xa_is_err(entry)) { + ret = xa_err(entry); + goto err_debugfs; + } + WARN_ON(entry); DRM_DEBUG("new minor registered %d\n", minor->index); return 0; @@ -187,16 +179,13 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) static void drm_minor_unregister(struct drm_device *dev, unsigned int type) { struct drm_minor *minor; - unsigned long flags; minor = *drm_minor_get_slot(dev, type); if (!minor || !device_is_registered(minor->kdev)) return; /* replace @minor with NULL so lookups will fail from now on */ - spin_lock_irqsave(&drm_minor_lock, flags); - idr_replace(&drm_minors_idr, NULL, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + xa_erase(&drm_minors_xa, minor->index); device_del(minor->kdev); dev_set_drvdata(minor->kdev, NULL); /* safety belt */ @@ -215,13 +204,10 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) struct drm_minor *drm_minor_acquire(unsigned int minor_id) { struct drm_minor *minor; - unsigned long flags; - spin_lock_irqsave(&drm_minor_lock, flags); - minor = idr_find(&drm_minors_idr, minor_id); + minor = xa_load(&drm_minors_xa, minor_id); if (minor) drm_dev_get(minor->dev); - spin_unlock_irqrestore(&drm_minor_lock, flags); if (!minor) { return ERR_PTR(-ENODEV); @@ -1037,7 +1023,7 @@ static void drm_core_exit(void) unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy(); - idr_destroy(&drm_minors_idr); + xa_destroy(&drm_minors_xa); drm_connector_ida_destroy(); } @@ -1046,7 +1032,6 @@ static int __init drm_core_init(void) int ret; drm_connector_ida_init(); - idr_init(&drm_minors_idr); drm_memcpy_init_early(); ret = drm_sysfs_init();
IDR is deprecated, and since XArray manages its own state with internal locking, it simplifies the locking on DRM side. Additionally, don't use the IRQ-safe variant, since operating on drm minor is not done in IRQ context. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Suggested-by: Matthew Wilcox <willy@infradead.org> --- drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++------------------------- 1 file changed, 17 insertions(+), 32 deletions(-)