[01/34] drm: Convert drm_minors_idr to XArray

Message ID	20190221184226.2149-2-willy@infradead.org (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> From: Matthew Wilcox <willy@infradead.org> To: dri-devel@lists.freedesktop.org Subject: [PATCH 01/34] drm: Convert drm_minors_idr to XArray Date: Thu, 21 Feb 2019 10:41:21 -0800 Message-Id: <20190221184226.2149-2-willy@infradead.org> In-Reply-To: <20190221184226.2149-1-willy@infradead.org> References: <20190221184226.2149-1-willy@infradead.org> Precedence: list Cc: Matthew Wilcox <willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
Series	Convert DRM to XArray \| expand [00/34] Convert DRM to XArray [01/34] drm: Convert drm_minors_idr to XArray [02/34] drm: Convert aux_idr to XArray [03/34] drm: Convert object_name_idr to XArray [04/34] drm: Convert object_idr to XArray [05/34] drm: Convert syncobj_idr to XArray [06/34] drm: Convert magic_map to XArray [07/34] drm: Convert lessee_idr to XArray [08/34] drm: Remove linked lists for lessees [09/34] drm: Convert ctx_idr to XArray [10/34] drm: Convert tile_idr to XArray [11/34] drm: Convert crtc_idr to XArray [12/34] drm/agp: Convert bo_list_handles to XArray [13/34] drm/amdgpu: Convert ctx_handles to XArray [14/34] drm/amdgpu: Convert pasid_idr to XArray [15/34] drm/amdkfd: Convert event_idr to XArray [16/34] drm/amdkfd: Convert alloc_idr to XArray [17/34] drm/etnaviv: Convert fence_idr to XArray [18/34] drm/i915: Convert handles_vma to XArray [19/34] drm/i915: Convert spt_tree to XArray [20/34] drm/i915: Convert page_track_tree to XArray [21/34] drm/i915: Convert get_page to XArray [22/34] drm/i915: Convert object_idr to IDA [23/34] drm/i915: Convert context_idr to XArray [24/34] drm/i915: Convert metrics_idr to XArray [25/34] drm/i915: Convert vgpus_idr to XArray [26/34] drm/qxl: Convert release_idr to XArray [27/34] drm/qxl: Convert surf_id_idr to XArray [28/34] drm/tegra: Convert contexts IDR to XArray [29/34] drm/vc4: Convert perfmon IDR to XArray [30/34] drm/sis: Convert object_idr to XArray [31/34] drm/vgem: Convert fence_idr to XArray [32/34] drm/via: Convert object_idr to XArray [33/34] drm/vmwgfx: Convert base IDR to XArray [34/34] drm/vmwgfx: Convert res_idr to XArray

Message ID

20190221184226.2149-2-willy@infradead.org (mailing list archive)

State

New, archived

Headers

From: Matthew Wilcox <willy@infradead.org>
To: dri-devel@lists.freedesktop.org
Subject: [PATCH 01/34] drm: Convert drm_minors_idr to XArray
Date: Thu, 21 Feb 2019 10:41:21 -0800
Message-Id: <20190221184226.2149-2-willy@infradead.org>
In-Reply-To: <20190221184226.2149-1-willy@infradead.org>
References: <20190221184226.2149-1-willy@infradead.org>
Precedence: list
Cc: Matthew Wilcox <willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: dri-devel-bounces@lists.freedesktop.org
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>

Series

Convert DRM to XArray | expand

Divide all the indices by 64 to save memory. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++------------------------- 1 file changed, 17 insertions(+), 32 deletions(-)

Comments

Daniel Vetter Feb. 22, 2019, 9:11 a.m. UTC | #1

On Thu, Feb 21, 2019 at 10:41:21AM -0800, Matthew Wilcox wrote:
> Divide all the indices by 64 to save memory.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>

Pretty sure this goes boom. Our char device minor allocation scheme is

device 0: card0=0, renderD0=64
device 1: card1=1, renderD1=65
...

I think your scheme aliases all devices with the first one.

And yes the minor(cardX) + 64 == minor(renderDX) is uapi :-)

If you want to save space we'd need to move the minor allocation from
drm_minor to drm_device (with a very strange allocation scheme of blocks
of 64 entries, every 128 entries). That would also solve the issue with
the current scheme potentially racing if you load multiple drivers at the
same time (except for drm_global_mutex, but that's more an accident than
intention). Not sure if worth the bother.

Or maybe coffee hasn't kicked in yet over here and I'm missing something?
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 12e5e2be7890..17ed29f49060 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -64,8 +64,7 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
>  "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
>  module_param_named(debug, drm_debug, int, 0600);
>  
> -static DEFINE_SPINLOCK(drm_minor_lock);
> -static struct idr drm_minors_idr;
> +static DEFINE_XARRAY_FLAGS(drm_minors, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>  
>  /*
>   * If the drm core fails to init for whatever reason,
> @@ -109,7 +108,6 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  {
>  	struct drm_minor *minor;
> -	unsigned long flags;
>  	int r;
>  
>  	minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> @@ -118,22 +116,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  
>  	minor->type = type;
>  	minor->dev = dev;
> +	minor->index = 64 * type;
>  
> -	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_insert_irq(&drm_minors, minor->index / 64, NULL, GFP_KERNEL);
>  	if (r < 0)
>  		goto err_free;
>  
> -	minor->index = r;
> -
>  	minor->kdev = drm_sysfs_minor_alloc(minor);
>  	if (IS_ERR(minor->kdev)) {
>  		r = PTR_ERR(minor->kdev);
> @@ -144,9 +132,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  	return 0;
>  
>  err_index:
> -	spin_lock_irqsave(&drm_minor_lock, flags);
> -	idr_remove(&drm_minors_idr, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_erase_irq(&drm_minors, minor->index / 64);
>  err_free:
>  	kfree(minor);
>  	return r;
> @@ -164,9 +150,9 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
>  
>  	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_lock_irqsave(&drm_minors, flags);
> +	__xa_erase(&drm_minors, minor->index / 64);
> +	xa_unlock_irqrestore(&drm_minors, flags);
>  
>  	kfree(minor);
>  	*slot = NULL;
> @@ -195,9 +181,9 @@ 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);
> +	xa_lock_irqsave(&drm_minors, flags);
> +	__xa_store(&drm_minors, minor->index / 64, minor, 0);
> +	xa_unlock_irqrestore(&drm_minors, flags);
>  
>  	DRM_DEBUG("new minor registered %d\n", minor->index);
>  	return 0;
> @@ -217,9 +203,9 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
>  		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_lock_irqsave(&drm_minors, flags);
> +	__xa_store(&drm_minors, minor->index / 64, NULL, 0);
> +	xa_unlock_irqrestore(&drm_minors, flags);
>  
>  	device_del(minor->kdev);
>  	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
> @@ -240,11 +226,11 @@ 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_irqsave(&drm_minors, flags);
> +	minor = xa_load(&drm_minors, minor_id / 64);
>  	if (minor)
>  		drm_dev_get(minor->dev);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_unlock_irqrestore(&drm_minors, flags);
>  
>  	if (!minor) {
>  		return ERR_PTR(-ENODEV);
> @@ -958,7 +944,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));
>  	drm_connector_ida_destroy();
>  }
>  
> @@ -967,7 +953,6 @@ static int __init drm_core_init(void)
>  	int ret;
>  
>  	drm_connector_ida_init();
> -	idr_init(&drm_minors_idr);
>  
>  	ret = drm_sysfs_init();
>  	if (ret < 0) {
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter Feb. 22, 2019, 9:55 a.m. UTC | #2

On Fri, Feb 22, 2019 at 10:11:14AM +0100, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 10:41:21AM -0800, Matthew Wilcox wrote:
> > Divide all the indices by 64 to save memory.
> > 
> > Signed-off-by: Matthew Wilcox <willy@infradead.org>
> 
> Pretty sure this goes boom. Our char device minor allocation scheme is
> 
> device 0: card0=0, renderD0=64
> device 1: card1=1, renderD1=65
> ...
> 
> I think your scheme aliases all devices with the first one.
> 
> And yes the minor(cardX) + 64 == minor(renderDX) is uapi :-)
> 
> If you want to save space we'd need to move the minor allocation from
> drm_minor to drm_device (with a very strange allocation scheme of blocks
> of 64 entries, every 128 entries). That would also solve the issue with
> the current scheme potentially racing if you load multiple drivers at the
> same time (except for drm_global_mutex, but that's more an accident than
> intention). Not sure if worth the bother.
> 
> Or maybe coffee hasn't kicked in yet over here and I'm missing something?

btw if you want to test locally, enable vkms.ko and vgem.ko and load them
both, for at least 2 drm drivers.
-Daniel

Matthew Wilcox Feb. 22, 2019, 3:13 p.m. UTC | #3

On Fri, Feb 22, 2019 at 10:11:14AM +0100, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 10:41:21AM -0800, Matthew Wilcox wrote:
> > Divide all the indices by 64 to save memory.
> > 
> > Signed-off-by: Matthew Wilcox <willy@infradead.org>
> 
> Pretty sure this goes boom. Our char device minor allocation scheme is
> 
> device 0: card0=0, renderD0=64
> device 1: card1=1, renderD1=65
> ...
> 
> I think your scheme aliases all devices with the first one.
> 
> And yes the minor(cardX) + 64 == minor(renderDX) is uapi :-)
> 
> If you want to save space we'd need to move the minor allocation from
> drm_minor to drm_device (with a very strange allocation scheme of blocks
> of 64 entries, every 128 entries). That would also solve the issue with
> the current scheme potentially racing if you load multiple drivers at the
> same time (except for drm_global_mutex, but that's more an accident than
> intention). Not sure if worth the bother.
> 
> Or maybe coffee hasn't kicked in yet over here and I'm missing something?

I'm the one who needed moar coffee.  I misread:

> > -	r = idr_alloc(&drm_minors_idr,
> > -		      NULL,
> > -		      64 * type,
> > -		      64 * (type + 1),
> > -		      GFP_NOWAIT);

As (64 * type) + 1.  So I'll redo this patch.

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 12e5e2be7890..17ed29f49060 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -64,8 +64,7 @@  MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
 "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
 module_param_named(debug, drm_debug, int, 0600);
 
-static DEFINE_SPINLOCK(drm_minor_lock);
-static struct idr drm_minors_idr;
+static DEFINE_XARRAY_FLAGS(drm_minors, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
 
 /*
  * If the drm core fails to init for whatever reason,
@@ -109,7 +108,6 @@  static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 	int r;
 
 	minor = kzalloc(sizeof(*minor), GFP_KERNEL);
@@ -118,22 +116,12 @@  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 
 	minor->type = type;
 	minor->dev = dev;
+	minor->index = 64 * type;
 
-	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_insert_irq(&drm_minors, minor->index / 64, NULL, GFP_KERNEL);
 	if (r < 0)
 		goto err_free;
 
-	minor->index = r;
-
 	minor->kdev = drm_sysfs_minor_alloc(minor);
 	if (IS_ERR(minor->kdev)) {
 		r = PTR_ERR(minor->kdev);
@@ -144,9 +132,7 @@  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	return 0;
 
 err_index:
-	spin_lock_irqsave(&drm_minor_lock, flags);
-	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_erase_irq(&drm_minors, minor->index / 64);
 err_free:
 	kfree(minor);
 	return r;
@@ -164,9 +150,9 @@  static void drm_minor_free(struct drm_device *dev, unsigned int type)
 
 	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_lock_irqsave(&drm_minors, flags);
+	__xa_erase(&drm_minors, minor->index / 64);
+	xa_unlock_irqrestore(&drm_minors, flags);
 
 	kfree(minor);
 	*slot = NULL;
@@ -195,9 +181,9 @@  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);
+	xa_lock_irqsave(&drm_minors, flags);
+	__xa_store(&drm_minors, minor->index / 64, minor, 0);
+	xa_unlock_irqrestore(&drm_minors, flags);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -217,9 +203,9 @@  static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 		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_lock_irqsave(&drm_minors, flags);
+	__xa_store(&drm_minors, minor->index / 64, NULL, 0);
+	xa_unlock_irqrestore(&drm_minors, flags);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -240,11 +226,11 @@  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_irqsave(&drm_minors, flags);
+	minor = xa_load(&drm_minors, minor_id / 64);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_unlock_irqrestore(&drm_minors, flags);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
@@ -958,7 +944,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));
 	drm_connector_ida_destroy();
 }
 
@@ -967,7 +953,6 @@  static int __init drm_core_init(void)
 	int ret;
 
 	drm_connector_ida_init();
-	idr_init(&drm_minors_idr);
 
 	ret = drm_sysfs_init();
 	if (ret < 0) {

[01/34] drm: Convert drm_minors_idr to XArray

Commit Message

Comments

Patch