Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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(_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.
Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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(_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.
Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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(_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?
Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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 Suggested-by: Matthew Wilcox --- 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 #include #include +#include #include #include @@ -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(_minor_lock, flags); - idr_remove(_minors_idr, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); - } + else + xa_erase(_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(_minor_lock, flags); - r = idr_alloc(_minors_idr, - NULL, - 64 * type, - 64 * (type + 1), - GFP_NOWAIT); - spin_unlock_irqrestore(_minor_lock, flags); + r = xa_alloc(_minors_xa, , 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(_minor_lock, flags); - idr_replace(_minors_idr, minor, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); + entry = xa_store(_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(_minors_xa, >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) { +
Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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 > > Suggested-by: Matthew Wilcox > > --- > > 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 > > #include > > #include > > +#include > > #include > > #include > > @@ -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(_minor_lock, flags); > > - idr_remove(_minors_idr, minor->index); > > - spin_unlock_irqrestore(_minor_lock, flags); > > - } > > + else > > + xa_erase(_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(_minor_lock, flags); > > - r = idr_alloc(_minors_idr, > > - NULL, > > - 64 * type, > > - 64 * (type + 1), > > - GFP_NOWAIT); > > - spin_unlock_irqrestore(_minor_lock, flags); > > + r = xa_alloc(_minors_xa, , 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(_minor_lock, flags); > > - idr_replace(_minors_idr, minor, minor->index); > > - spin_unlock_irqrestore(_minor_lock, flags); > > + entry = xa_store(_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(_minors_xa, >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))
Re: [Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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 Suggested-by: Matthew Wilcox --- 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 #include #include +#include #include #include @@ -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(_minor_lock, flags); - idr_remove(_minors_idr, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); - } + else + xa_erase(_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(_minor_lock, flags); - r = idr_alloc(_minors_idr, - NULL, - 64 * type, - 64 * (type + 1), - GFP_NOWAIT); - spin_unlock_irqrestore(_minor_lock, flags); + r = xa_alloc(_minors_xa, , 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(_minor_lock, flags); - idr_replace(_minors_idr, minor, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); + entry = xa_store(_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(_minor_lock, flags); - idr_replace(_minors_idr, NULL, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); - } + else +
[Intel-gfx] [PATCH v6 1/4] drm: Use XArray instead of IDR for minors
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 Suggested-by: Matthew Wilcox --- 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 #include #include +#include #include #include @@ -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(_minor_lock, flags); - idr_remove(_minors_idr, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); - } + else + xa_erase(_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(_minor_lock, flags); - r = idr_alloc(_minors_idr, - NULL, - 64 * type, - 64 * (type + 1), - GFP_NOWAIT); - spin_unlock_irqrestore(_minor_lock, flags); + r = xa_alloc(_minors_xa, , 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(_minor_lock, flags); - idr_replace(_minors_idr, minor, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); + entry = xa_store(_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(_minor_lock, flags); - idr_replace(_minors_idr, NULL, minor->index); - spin_unlock_irqrestore(_minor_lock, flags); - } + else + xa_store(_minors_xa, minor->index, NULL, GFP_KERNEL);