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@xxxxxxxxxx> wrote: > > > > On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski > > <michal.winiarski@xxxxxxxxx> 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@xxxxxxxxx> > > > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxx/ > > > > 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@xxxxxxxxx/ 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 > > >