[PATCH v3 0/4] drm: Use full allocated minor range for DRM

[PATCH v3 0/4] drm: Use full allocated minor range for DRM

[Michał Winiarski]

64 DRM device nodes is not enough for everyone.
Upgrade it to ~512K (which definitely is more than enough).

To allow testing userspace support for >64 devices, add additional DRM
modparam (skip_legacy_minors) which causes DRM to skip allocating minors
in 0-192 range.
Additionally - one minor tweak around minor DRM IDR locking and IDR lockdep
annotations.

v1 -> v2:
Don't touch DRM_MINOR_CONTROL and its range (Simon Ser)

v2 -> v3:
Don't use legacy scheme for >=192 minor range (Dave Airlie)
Add modparam for testing (Dave Airlie)
Add lockdep annotation for IDR (Daniel Vetter)

Michał Winiarski (4):
  drm: Expand max DRM device number to full MINORBITS
  drm: Introduce skip_legacy_minors modparam
  drm: Use mutex for minors
  idr: Add might_alloc() annotation

 drivers/gpu/drm/drm_drv.c | 55 ++++++++++++++++++++++-----------------
 lib/radix-tree.c          |  3 +++
 2 files changed, 34 insertions(+), 24 deletions(-)

-- 
2.37.3

[PATCH v3 1/4] drm: Expand max DRM device number to full MINORBITS

[Michał Winiarski]

Having a limit of 64 DRM devices is not good enough for modern world
where we have multi-GPU servers, SR-IOV virtual functions and virtual
devices used for testing.
Let's utilize full minor range for DRM devices.
To avoid regressing the existing userspace, we're still maintaining the
numbering scheme where 0-63 is used for primary, 64-127 is reserved
(formerly for control) and 128-191 is used for render.
For minors >= 192, we're allocating minors dynamically on a first-come,
first-served basis.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..9432b1619602 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -122,6 +122,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->type = type;
 	minor->dev = dev;
 
+	/*
+	 * DRM used to support 64 devices, for backwards compatibility we need to maintain the
+	 * minor allocation scheme where minors 0-63 are primary nodes, 64-127 are control nodes,
+	 * and 128-191 are render nodes.
+	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
+	 */
 	idr_preload(GFP_KERNEL);
 	spin_lock_irqsave(&drm_minor_lock, flags);
 	r = idr_alloc(&drm_minors_idr,
@@ -129,6 +135,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 		      64 * type,
 		      64 * (type + 1),
 		      GFP_NOWAIT);
+	if (r == -ENOSPC)
+		r = idr_alloc(&drm_minors_idr, NULL, 192, 1 << MINORBITS, GFP_NOWAIT);
 	spin_unlock_irqrestore(&drm_minor_lock, flags);
 	idr_preload_end();
 
-- 
2.37.3

[PATCH v3 2/4] drm: Introduce skip_legacy_minors modparam

[Michał Winiarski]

While there is support for >64 DRM devices on kernel side, existing
userspace may still have some hardcoded assumptions and it's possible
that it will require changes to be able to use more than 64 devices.
Add a modparam to simplify testing and development of >64 devices
support on userspace side by allocating minors from the >=192 range
(without the need of having >64 physical devices connected).

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9432b1619602..0bdcca0db611 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -56,6 +56,11 @@ MODULE_LICENSE("GPL and additional rights");
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
+static bool skip_legacy_minors;
+module_param_unsafe(skip_legacy_minors, bool, 0400);
+MODULE_PARM_DESC(skip_legacy_minors,
+		 "Don't allocate minors in 0-192 range. This can be used for testing userspace support for >64 drm devices (default: false)");
+
 /*
  * If the drm core fails to init for whatever reason,
  * we should prevent any drivers from registering with it.
@@ -113,7 +118,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
 	unsigned long flags;
-	int r;
+	int r = -ENOSPC;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
 	if (!minor)
@@ -130,11 +135,12 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int 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);
+	if (!skip_legacy_minors)
+		r = idr_alloc(&drm_minors_idr,
+			      NULL,
+			      64 * type,
+			      64 * (type + 1),
+			      GFP_NOWAIT);
 	if (r == -ENOSPC)
 		r = idr_alloc(&drm_minors_idr, NULL, 192, 1 << MINORBITS, GFP_NOWAIT);
 	spin_unlock_irqrestore(&drm_minor_lock, flags);
-- 
2.37.3

[PATCH v3 3/4] drm: Use mutex for minors

[Michał Winiarski]

Operating on drm minor is not done in IRQ context, which means that we
could safely downgrade to regular non-irq spinlock.
But we can also go further and drop the idr_preload tricks by just using
a mutex.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_drv.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0bdcca0db611..f66904527256 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -53,7 +53,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 DEFINE_MUTEX(drm_minor_lock);
 static struct idr drm_minors_idr;
 
 static bool skip_legacy_minors;
@@ -103,21 +103,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);
+	mutex_lock(&drm_minor_lock);
 	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 }
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 	int r = -ENOSPC;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -133,18 +131,16 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	 * and 128-191 are render nodes.
 	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
 	 */
-	idr_preload(GFP_KERNEL);
-	spin_lock_irqsave(&drm_minor_lock, flags);
+	mutex_lock(&drm_minor_lock);
 	if (!skip_legacy_minors)
 		r = idr_alloc(&drm_minors_idr,
 			      NULL,
 			      64 * type,
 			      64 * (type + 1),
-			      GFP_NOWAIT);
+			      GFP_KERNEL);
 	if (r == -ENOSPC)
-		r = idr_alloc(&drm_minors_idr, NULL, 192, 1 << MINORBITS, GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
-	idr_preload_end();
+		r = idr_alloc(&drm_minors_idr, NULL, 192, 1 << MINORBITS, GFP_KERNEL);
+	mutex_unlock(&drm_minor_lock);
 
 	if (r < 0)
 		return r;
@@ -166,7 +162,6 @@ 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;
 	int ret;
 
 	DRM_DEBUG("\n");
@@ -186,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);
+	mutex_lock(&drm_minor_lock);
 	idr_replace(&drm_minors_idr, minor, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -201,16 +196,15 @@ 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);
+	mutex_lock(&drm_minor_lock);
 	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -229,13 +223,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);
+	mutex_lock(&drm_minor_lock);
 	minor = idr_find(&drm_minors_idr, minor_id);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
-- 
2.37.3

[PATCH v3 4/4] idr: Add might_alloc() annotation

[Michał Winiarski]

Using might_alloc() lets us catch problems in a deterministic manner,
even if we end up not allocating anything.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 lib/radix-tree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 3c78e1e8b2ad..787ab01001de 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -23,6 +23,7 @@
 #include <linux/preempt.h>		/* in_interrupt() */
 #include <linux/radix-tree.h>
 #include <linux/rcupdate.h>
+#include <linux/sched/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/xarray.h>
@@ -1481,6 +1482,8 @@ void __rcu **idr_get_free(struct radix_tree_root *root,
 	unsigned long maxindex, start = iter->next_index;
 	unsigned int shift, offset = 0;
 
+	might_alloc(gfp);
+
  grow:
 	shift = radix_tree_load_root(root, &child, &maxindex);
 	if (!radix_tree_tagged(root, IDR_FREE))
-- 
2.37.3

Re: [PATCH v3 0/4] drm: Use full allocated minor range for DRM

[Matthew Wilcox]

On Tue, Sep 06, 2022 at 04:01:13PM +0200, Michał Winiarski wrote:
> 64 DRM device nodes is not enough for everyone.
> Upgrade it to ~512K (which definitely is more than enough).
> 
> To allow testing userspace support for >64 devices, add additional DRM
> modparam (skip_legacy_minors) which causes DRM to skip allocating minors
> in 0-192 range.
> Additionally - one minor tweak around minor DRM IDR locking and IDR lockdep
> annotations.

The IDR is deprecated; rather than making all these changes around
the IDR, could you convert it to use the XArray instead?  I did it
once before, but those patches bounced off the submissions process.

Re: [PATCH v3 0/4] drm: Use full allocated minor range for DRM

[Michał Winiarski]

On Tue, Sep 06, 2022 at 03:21:25PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 06, 2022 at 04:01:13PM +0200, Michał Winiarski wrote:
> > 64 DRM device nodes is not enough for everyone.
> > Upgrade it to ~512K (which definitely is more than enough).
> > 
> > To allow testing userspace support for >64 devices, add additional DRM
> > modparam (skip_legacy_minors) which causes DRM to skip allocating minors
> > in 0-192 range.
> > Additionally - one minor tweak around minor DRM IDR locking and IDR lockdep
> > annotations.
> 
> The IDR is deprecated; rather than making all these changes around
> the IDR, could you convert it to use the XArray instead?  I did it
> once before, but those patches bounced off the submissions process.

Sure. The IDR annotation can still be useful for existing users though,
are you saying I should drop it as well?

-Michał