[PATCH 01/34] drm: Convert drm_minors_idr to XArray
Daniel Vetter
daniel at ffwll.ch
Fri Feb 22 09:11:14 UTC 2019
On Thu, Feb 21, 2019 at 10:41:21AM -0800, Matthew Wilcox wrote:
> Divide all the indices by 64 to save memory.
>
> Signed-off-by: Matthew Wilcox <willy at infradead.org>
Pretty sure this goes boom. Our char device minor allocation scheme is
device 0: card0=0, renderD0=64
device 1: card1=1, renderD1=65
...
I think your scheme aliases all devices with the first one.
And yes the minor(cardX) + 64 == minor(renderDX) is uapi :-)
If you want to save space we'd need to move the minor allocation from
drm_minor to drm_device (with a very strange allocation scheme of blocks
of 64 entries, every 128 entries). That would also solve the issue with
the current scheme potentially racing if you load multiple drivers at the
same time (except for drm_global_mutex, but that's more an accident than
intention). Not sure if worth the bother.
Or maybe coffee hasn't kicked in yet over here and I'm missing something?
-Daniel
> ---
> drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++-------------------------
> 1 file changed, 17 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 12e5e2be7890..17ed29f49060 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -64,8 +64,7 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
> "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> module_param_named(debug, drm_debug, int, 0600);
>
> -static DEFINE_SPINLOCK(drm_minor_lock);
> -static struct idr drm_minors_idr;
> +static DEFINE_XARRAY_FLAGS(drm_minors, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>
> /*
> * If the drm core fails to init for whatever reason,
> @@ -109,7 +108,6 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
> static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> {
> struct drm_minor *minor;
> - unsigned long flags;
> int r;
>
> minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> @@ -118,22 +116,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>
> minor->type = type;
> minor->dev = dev;
> + minor->index = 64 * type;
>
> - 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_insert_irq(&drm_minors, minor->index / 64, NULL, GFP_KERNEL);
> if (r < 0)
> goto err_free;
>
> - minor->index = r;
> -
> minor->kdev = drm_sysfs_minor_alloc(minor);
> if (IS_ERR(minor->kdev)) {
> r = PTR_ERR(minor->kdev);
> @@ -144,9 +132,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> return 0;
>
> err_index:
> - spin_lock_irqsave(&drm_minor_lock, flags);
> - idr_remove(&drm_minors_idr, minor->index);
> - spin_unlock_irqrestore(&drm_minor_lock, flags);
> + xa_erase_irq(&drm_minors, minor->index / 64);
> err_free:
> kfree(minor);
> return r;
> @@ -164,9 +150,9 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
>
> 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_lock_irqsave(&drm_minors, flags);
> + __xa_erase(&drm_minors, minor->index / 64);
> + xa_unlock_irqrestore(&drm_minors, flags);
>
> kfree(minor);
> *slot = NULL;
> @@ -195,9 +181,9 @@ 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);
> + xa_lock_irqsave(&drm_minors, flags);
> + __xa_store(&drm_minors, minor->index / 64, minor, 0);
> + xa_unlock_irqrestore(&drm_minors, flags);
>
> DRM_DEBUG("new minor registered %d\n", minor->index);
> return 0;
> @@ -217,9 +203,9 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
> 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_lock_irqsave(&drm_minors, flags);
> + __xa_store(&drm_minors, minor->index / 64, NULL, 0);
> + xa_unlock_irqrestore(&drm_minors, flags);
>
> device_del(minor->kdev);
> dev_set_drvdata(minor->kdev, NULL); /* safety belt */
> @@ -240,11 +226,11 @@ 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);
> + xa_lock_irqsave(&drm_minors, flags);
> + minor = xa_load(&drm_minors, minor_id / 64);
> if (minor)
> drm_dev_get(minor->dev);
> - spin_unlock_irqrestore(&drm_minor_lock, flags);
> + xa_unlock_irqrestore(&drm_minors, flags);
>
> if (!minor) {
> return ERR_PTR(-ENODEV);
> @@ -958,7 +944,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);
> + WARN_ON(!xa_empty(&drm_minors));
> drm_connector_ida_destroy();
> }
>
> @@ -967,7 +953,6 @@ static int __init drm_core_init(void)
> int ret;
>
> drm_connector_ida_init();
> - idr_init(&drm_minors_idr);
>
> ret = drm_sysfs_init();
> if (ret < 0) {
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list