Messages in this thread | | | Date | Tue, 29 Aug 2023 13:34:22 -0400 | Subject | Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors | From | James Zhu <> |
| |
On 2023-08-28 17:08, Michał Winiarski wrote: > On Fri, Aug 25, 2023 at 12:59:26PM -0400, James Zhu wrote: >> On 2023-07-24 17:14, 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> >>> --- >>> drivers/gpu/drm/drm_drv.c | 63 ++++++++++++++++----------------------- >>> 1 file changed, 25 insertions(+), 38 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 3eda026ffac6..3faecb01186f 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_accel.h> >>> #include <drm/drm_cache.h> >>> @@ -54,8 +55,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, >>> @@ -101,26 +101,23 @@ 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); >>> - if (minor->type == DRM_MINOR_ACCEL) { >>> + if (minor->type == DRM_MINOR_ACCEL) >>> accel_minor_remove(minor->index); >>> - } else { >>> - spin_lock_irqsave(&drm_minor_lock, flags); >>> - idr_remove(&drm_minors_idr, minor->index); >>> - spin_unlock_irqrestore(&drm_minor_lock, flags); >>> - } >>> + else >>> + xa_erase(&drm_minors_xa, minor->index); >>> } >>> +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t + 63); }) >>> + >>> static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type) >>> { >>> struct drm_minor *minor; >>> - unsigned long flags; >>> - int r; >>> + int index, r; >>> minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); >>> if (!minor) >>> @@ -129,24 +126,17 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type) >>> minor->type = type; >>> minor->dev = dev; >>> - idr_preload(GFP_KERNEL); >>> if (type == DRM_MINOR_ACCEL) { >>> r = accel_minor_alloc(); >>> + index = r; >>> } else { >>> - 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); >>> + r = xa_alloc(&drm_minors_xa, &index, NULL, DRM_MINOR_LIMIT(type), GFP_KERNEL); >>> } >>> - idr_preload_end(); >>> if (r < 0) >>> return r; >>> - minor->index = r; >>> + minor->index = index; >>> r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); >>> if (r) >>> @@ -163,7 +153,7 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type) >>> static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type) >>> { >>> struct drm_minor *minor; >>> - unsigned long flags; >>> + void *entry; >>> int ret; >>> DRM_DEBUG("\n"); >>> @@ -190,9 +180,12 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type) >>> if (minor->type == DRM_MINOR_ACCEL) { >>> accel_minor_replace(minor, minor->index); >>> } else { >>> - 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); >> [JZ] would WARN_ON(entry != minor)be better? > We expect NULL here. > The same one that was previously stored here ⌄⌄⌄ > r = xa_alloc(&drm_minors_xa, &minor->index, NULL, DRM_EXTENDED_MINOR_LIMIT, GFP_KERNEL); > in drm_minor_alloc. [JZ] My mistake. > >>> } >>> DRM_DEBUG("new minor registered %d\n", minor->index); >>> @@ -206,20 +199,16 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type) >>> static void drm_minor_unregister(struct drm_device *dev, enum drm_minor_type 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 */ >>> - if (minor->type == DRM_MINOR_ACCEL) { >>> + if (minor->type == DRM_MINOR_ACCEL) >>> accel_minor_replace(NULL, minor->index); >>> - } else { >>> - spin_lock_irqsave(&drm_minor_lock, flags); >>> - idr_replace(&drm_minors_idr, NULL, minor->index); >>> - spin_unlock_irqrestore(&drm_minor_lock, flags); >>> - } >>> + else >>> + xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); >>> device_del(minor->kdev); >>> dev_set_drvdata(minor->kdev, NULL); /* safety belt */ >>> @@ -238,13 +227,12 @@ static void drm_minor_unregister(struct drm_device *dev, enum drm_minor_type typ >>> 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(&drm_minors_xa); >>> + minor = xa_load(&drm_minors_xa, minor_id); >>> if (minor) >>> drm_dev_get(minor->dev); >> [JZ] why minor->dev need ca_lock here? > We're relying on the ordering for acquire/release (previously > guaranteed by the drm_minor_lock, now by internal XArray locking). > xa_load doesn't take xa_lock internally: > https://docs.kernel.org/core-api/xarray.html#locking [JZ] I means that drm_dev_get(minor->dev); can move out of xa_lock. >>> - spin_unlock_irqrestore(&drm_minor_lock, flags); >>> + xa_unlock(&drm_minors_xa); >>> if (!minor) { >>> return ERR_PTR(-ENODEV); >>> @@ -1067,7 +1055,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); >> [JZ] Should we call xa_destroy instead here? > We could, if we expect the xarray to potentially not be empty. > From what I understand - all minors should be released at this point. [JZ] In practice, adding destroy here will be better. > > Thanks, > -Michał > >>> + WARN_ON(!xa_empty(&drm_minors_xa)); >>> drm_connector_ida_destroy(); >>> } >>> @@ -1076,7 +1064,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();
| |