| Message ID | 20230724211428.3831636-2-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Use full allocated minor range for DRM | expand |
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? > } > > 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? > - 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? > + 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();
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. > > } > > 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 > > - 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. 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();
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();
On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote: > > > > @@ -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. Why do you say that?
On 2023-08-29 14:33, Matthew Wilcox wrote: > On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote: >>>>> @@ -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. > Why do you say that? [JZ] In case, the future, INIT adds something, need DESTROY to free completely.
On Tue, Aug 29, 2023 at 02:35:34PM -0400, James Zhu wrote: > > On 2023-08-29 14:33, Matthew Wilcox wrote: > > On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote: > > > > > > @@ -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. > > Why do you say that? > [JZ] In case, the future, INIT adds something, need DESTROY to free > completely. That isn't going to happen.
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); } 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); - 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); + 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();
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(-)