| Message ID | 20220911211443.581481-2-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Use full allocated minor range for DRM | expand |
On 9/11/2022 3:14 PM, 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> > --- Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski <michal.winiarski@intel.com> 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 | 51 ++++++++++++++------------------------- > 1 file changed, 18 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 8214a0b1ab7f..61d24cdcd0f8 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,19 @@ 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_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, unsigned int type) > { > struct drm_minor *minor; > - unsigned long flags; > int r; > > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); > @@ -122,21 +120,10 @@ 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, &minor->index, NULL, DRM_MINOR_LIMIT(type), GFP_KERNEL); > if (r < 0) > return r; > > - minor->index = r; > - > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); > if (r) > return r; > @@ -152,7 +139,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 +159,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_cmpxchg(&drm_minors_xa, minor->index, NULL, &minor, GFP_KERNEL); I believe we should pass in "minor", without the &, as &minor will give you the address of the local pointer. Oded > + 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 +177,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_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); > > device_del(minor->kdev); > dev_set_drvdata(minor->kdev, NULL); /* safety belt */ > @@ -215,13 +202,12 @@ 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); > + 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); > @@ -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); > + WARN_ON(!xa_empty(&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(); > -- > 2.37.3 >
On Wed, Nov 2, 2022 at 4:23 PM Oded Gabbay <ogabbay@kernel.org> wrote: > > On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski > <michal.winiarski@intel.com> 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 | 51 ++++++++++++++------------------------- > > 1 file changed, 18 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 8214a0b1ab7f..61d24cdcd0f8 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,19 @@ 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_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, unsigned int type) > > { > > struct drm_minor *minor; > > - unsigned long flags; > > int r; > > > > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); > > @@ -122,21 +120,10 @@ 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, &minor->index, NULL, DRM_MINOR_LIMIT(type), GFP_KERNEL); This was GFP_NOWAIT in the original code. > > if (r < 0) > > return r; > > > > - minor->index = r; > > - > > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); > > if (r) > > return r; > > @@ -152,7 +139,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 +159,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_cmpxchg(&drm_minors_xa, minor->index, NULL, &minor, GFP_KERNEL); > I believe we should pass in "minor", without the &, as &minor will > give you the address of the local pointer. > > Oded > > > + 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 +177,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_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); > > > > device_del(minor->kdev); > > dev_set_drvdata(minor->kdev, NULL); /* safety belt */ > > @@ -215,13 +202,12 @@ 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); > > + xa_lock(&drm_minors_xa); > > + minor = xa_load(&drm_minors_xa, minor_id); Did you check this part ? Because this always gives me NULL... I tried executing the following code in a dummy driver I wrote: static DEFINE_XARRAY_ALLOC(xa_dummy); void check_xa(void *pdev) { void *entry; int ret, index; ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT); if (ret < 0) return ret; entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL); if (xa_is_err(entry)) return; xa_lock(&xa_dummy); xa_dev = xa_load(&xa_dummy, index); xa_unlock(&xa_dummy); } And to my surprise xa_dev is always NULL, when it should be pdev. I believe that because we first allocate the entry with NULL, it is considered a "zero" entry in the XA. And when we replace it, this attribute doesn't change so when we do xa_load, the xa code thinks the entry is a "zero" entry and returns NULL. If that's correct, you need to either fix xarray code or change the flow of allocating this in drm. If I send a real pointer (just a dummy object I allocated) instead of NULL in xa_alloc, and then do xa_cmpxchg with pdev, xa_load returns pdev successfully. That points to the NULL being problematic in allocating an entry. Oded > > 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); > > @@ -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); > > + WARN_ON(!xa_empty(&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(); > > -- > > 2.37.3 > >
On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote: > I tried executing the following code in a dummy driver I wrote: You don't need to write a dummy driver; you can write test-cases entirely in userspace. Just add them to lib/test_xarray.c and then make -C tools/testing/radix-tree/ > static DEFINE_XARRAY_ALLOC(xa_dummy); > void check_xa(void *pdev) > { > void *entry; > int ret, index; > > ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT); > if (ret < 0) > return ret; > > entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL); > if (xa_is_err(entry)) > return; > > xa_lock(&xa_dummy); > xa_dev = xa_load(&xa_dummy, index); > xa_unlock(&xa_dummy); > } > > And to my surprise xa_dev is always NULL, when it should be pdev. > I believe that because we first allocate the entry with NULL, it is > considered a "zero" entry in the XA. > And when we replace it, this attribute doesn't change so when we do > xa_load, the xa code thinks the entry is a "zero" entry and returns > NULL. There's no "attribute" to mark a zero entry. It's just a zero entry. The problem is that you're cmpxchg'ing against NULL, and it's not NULL, it's the ZERO entry. This is even documented in the test code: /* cmpxchg sees a reserved entry as ZERO */ XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0); XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY, xa_mk_value(12345678), GFP_NOWAIT) != NULL); I'm not quite sure why you're using xa_cmpxchg() here anyway; if you allocated it, it's yours and you can just xa_store() to it.
On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote: > On Wed, Nov 2, 2022 at 4:23 PM Oded Gabbay <ogabbay@kernel.org> wrote: > > > > On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski > > <michal.winiarski@intel.com> 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 | 51 ++++++++++++++------------------------- > > > 1 file changed, 18 insertions(+), 33 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 8214a0b1ab7f..61d24cdcd0f8 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,19 @@ 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_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, unsigned int type) > > > { > > > struct drm_minor *minor; > > > - unsigned long flags; > > > int r; > > > > > > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); > > > @@ -122,21 +120,10 @@ 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, &minor->index, NULL, DRM_MINOR_LIMIT(type), GFP_KERNEL); > This was GFP_NOWAIT in the original code. Because we were using spinlock and idr_preload. We're actually fine with GFP_KERNEL (and we could just use mutex with idr): https://lore.kernel.org/dri-devel/20220823210612.296922-3-michal.winiarski@intel.com/ > > > > if (r < 0) > > > return r; > > > > > > - minor->index = r; > > > - > > > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); > > > if (r) > > > return r; > > > @@ -152,7 +139,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 +159,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_cmpxchg(&drm_minors_xa, minor->index, NULL, &minor, GFP_KERNEL); > > I believe we should pass in "minor", without the &, as &minor will > > give you the address of the local pointer. > > > > Oded > > > > > + 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 +177,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_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); > > > > > > device_del(minor->kdev); > > > dev_set_drvdata(minor->kdev, NULL); /* safety belt */ > > > @@ -215,13 +202,12 @@ 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); > > > + xa_lock(&drm_minors_xa); > > > + minor = xa_load(&drm_minors_xa, minor_id); > Did you check this part ? Because this always gives me NULL... > > I tried executing the following code in a dummy driver I wrote: > > static DEFINE_XARRAY_ALLOC(xa_dummy); > void check_xa(void *pdev) > { > void *entry; > int ret, index; > > ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT); > if (ret < 0) > return ret; > > entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL); > if (xa_is_err(entry)) > return; > > xa_lock(&xa_dummy); > xa_dev = xa_load(&xa_dummy, index); > xa_unlock(&xa_dummy); > } > > And to my surprise xa_dev is always NULL, when it should be pdev. > I believe that because we first allocate the entry with NULL, it is > considered a "zero" entry in the XA. > And when we replace it, this attribute doesn't change so when we do > xa_load, the xa code thinks the entry is a "zero" entry and returns > NULL. > If that's correct, you need to either fix xarray code or change the > flow of allocating this in drm. > > If I send a real pointer (just a dummy object I allocated) instead of > NULL in xa_alloc, and then do xa_cmpxchg with pdev, xa_load returns > pdev successfully. > That points to the NULL being problematic in allocating an entry. Yeah, this was just using xa_store in previous revision. https://lore.kernel.org/dri-devel/20220906201629.419160-2-michal.winiarski@intel.com/ And I didn't notice the CI failures: https://patchwork.freedesktop.org/series/108206/ since I was originally hoping for comments on extending DRM minor limit, XArray conversion was a bit of an extra. Sorry about that - I just sent a next version using xa_store as a separate patch (without extending DRM device limit). Thanks! -Michał > > Oded > > > > > 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); > > > @@ -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); > > > + WARN_ON(!xa_empty(&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(); > > > -- > > > 2.37.3 > > >
On Mon, Nov 7, 2022 at 6:20 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote: > > I tried executing the following code in a dummy driver I wrote: > > You don't need to write a dummy driver; you can write test-cases > entirely in userspace. Just add them to lib/test_xarray.c and then > make -C tools/testing/radix-tree/ > > > static DEFINE_XARRAY_ALLOC(xa_dummy); > > void check_xa(void *pdev) > > { > > void *entry; > > int ret, index; > > > > ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT); > > if (ret < 0) > > return ret; > > > > entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL); > > if (xa_is_err(entry)) > > return; > > > > xa_lock(&xa_dummy); > > xa_dev = xa_load(&xa_dummy, index); > > xa_unlock(&xa_dummy); > > } > > > > And to my surprise xa_dev is always NULL, when it should be pdev. > > I believe that because we first allocate the entry with NULL, it is > > considered a "zero" entry in the XA. > > And when we replace it, this attribute doesn't change so when we do > > xa_load, the xa code thinks the entry is a "zero" entry and returns > > NULL. > > There's no "attribute" to mark a zero entry. It's just a zero entry. > The problem is that you're cmpxchg'ing against NULL, and it's not NULL, > it's the ZERO entry. This is even documented in the test code: > > /* cmpxchg sees a reserved entry as ZERO */ > XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0); > XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY, > xa_mk_value(12345678), GFP_NOWAIT) != NULL); > > I'm not quite sure why you're using xa_cmpxchg() here anyway; if you > allocated it, it's yours and you can just xa_store() to it. > Hi Matthew, Thanks for the explanation. I believe Michal's will fix his original patch and I'll take that code. Oded
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8214a0b1ab7f..61d24cdcd0f8 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,19 @@ 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_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, unsigned int type) { struct drm_minor *minor; - unsigned long flags; int r; minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); @@ -122,21 +120,10 @@ 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, &minor->index, NULL, DRM_MINOR_LIMIT(type), GFP_KERNEL); if (r < 0) return r; - minor->index = r; - r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); if (r) return r; @@ -152,7 +139,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 +159,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_cmpxchg(&drm_minors_xa, minor->index, NULL, &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 +177,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_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); device_del(minor->kdev); dev_set_drvdata(minor->kdev, NULL); /* safety belt */ @@ -215,13 +202,12 @@ 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); + 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); @@ -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); + WARN_ON(!xa_empty(&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 | 51 ++++++++++++++------------------------- 1 file changed, 18 insertions(+), 33 deletions(-)