[PATCH] char: misc: Allow minors values up to MINORMASK

[PATCH] char: misc: Allow minors values up to MINORMASK

[D Scott Phillips]

For per-cpu coresight_tmc devices, we can end up with hundreds of devices
on large systems that all want a dynamic minor number.  Switch the dynamic
minors allocator to an ida and add logic to allocate in the ranges [0..127]
and [256..MINORMASK]. Allocations start from 127 growing downwards and then
increasing from 256, so device numbering for the first 128 devices should
be the same as before.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
---
 drivers/char/misc.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index cba19bfdc44d..05727f0daa6b 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -61,7 +61,29 @@ static DEFINE_MUTEX(misc_mtx);
  * Assigned numbers, used for dynamic minors
  */
 #define DYNAMIC_MINORS 128 /* like dynamic majors */
-static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS);
+static DEFINE_IDA(misc_minors_ida);
+
+static int misc_minor_alloc(void)
+{
+	int ret;
+
+	ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+	if (ret >= 0) {
+		ret = DYNAMIC_MINORS - ret - 1;
+	} else {
+		ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+				      MINORMASK, GFP_KERNEL);
+	}
+	return ret;
+}
+
+static void misc_minor_free(int minor)
+{
+	if (minor < DYNAMIC_MINORS)
+		ida_free(&misc_minors_ida, DYNAMIC_MINORS - minor - 1);
+	else if (minor > MISC_DYNAMIC_MINOR)
+		ida_free(&misc_minors_ida, minor);
+}
 
 #ifdef CONFIG_PROC_FS
 static void *misc_seq_start(struct seq_file *seq, loff_t *pos)
@@ -183,14 +205,13 @@ int misc_register(struct miscdevice *misc)
 	mutex_lock(&misc_mtx);
 
 	if (is_dynamic) {
-		int i = find_first_zero_bit(misc_minors, DYNAMIC_MINORS);
+		int i = misc_minor_alloc();
 
-		if (i >= DYNAMIC_MINORS) {
+		if (i < 0) {
 			err = -EBUSY;
 			goto out;
 		}
-		misc->minor = DYNAMIC_MINORS - i - 1;
-		set_bit(i, misc_minors);
+		misc->minor = i;
 	} else {
 		struct miscdevice *c;
 
@@ -209,10 +230,7 @@ int misc_register(struct miscdevice *misc)
 					  misc, misc->groups, "%s", misc->name);
 	if (IS_ERR(misc->this_device)) {
 		if (is_dynamic) {
-			int i = DYNAMIC_MINORS - misc->minor - 1;
-
-			if (i < DYNAMIC_MINORS && i >= 0)
-				clear_bit(i, misc_minors);
+			misc_minor_free(misc->minor);
 			misc->minor = MISC_DYNAMIC_MINOR;
 		}
 		err = PTR_ERR(misc->this_device);
@@ -240,16 +258,13 @@ EXPORT_SYMBOL(misc_register);
 
 void misc_deregister(struct miscdevice *misc)
 {
-	int i = DYNAMIC_MINORS - misc->minor - 1;
-
 	if (WARN_ON(list_empty(&misc->list)))
 		return;
 
 	mutex_lock(&misc_mtx);
 	list_del(&misc->list);
 	device_destroy(misc_class, MKDEV(MISC_MAJOR, misc->minor));
-	if (i < DYNAMIC_MINORS && i >= 0)
-		clear_bit(i, misc_minors);
+	misc_minor_free(misc->minor);
 	mutex_unlock(&misc_mtx);
 }
 EXPORT_SYMBOL(misc_deregister);
-- 
2.37.2

Re: [PATCH] char: misc: Allow minors values up to MINORMASK

[Greg Kroah-Hartman]

On Tue, Sep 06, 2022 at 12:52:13PM -0700, D Scott Phillips wrote:
> For per-cpu coresight_tmc devices, we can end up with hundreds of devices
> on large systems that all want a dynamic minor number.  Switch the dynamic
> minors allocator to an ida and add logic to allocate in the ranges [0..127]
> and [256..MINORMASK]. Allocations start from 127 growing downwards and then
> increasing from 256, so device numbering for the first 128 devices should
> be the same as before.
> 
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> ---
>  drivers/char/misc.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)

So you are adding more logic to the kernel for no change at all?

Why is this needed?  What changed here except the underlying data
structure being used?

thanks,

greg k-h

Re: [PATCH] char: misc: Allow minors values up to MINORMASK

[D Scott Phillips]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, Sep 06, 2022 at 12:52:13PM -0700, D Scott Phillips wrote:
>> For per-cpu coresight_tmc devices, we can end up with hundreds of devices
>> on large systems that all want a dynamic minor number.  Switch the dynamic
>> minors allocator to an ida and add logic to allocate in the ranges [0..127]
>> and [256..MINORMASK]. Allocations start from 127 growing downwards and then
>> increasing from 256, so device numbering for the first 128 devices should
>> be the same as before.
>> 
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> ---
>>  drivers/char/misc.c | 41 ++++++++++++++++++++++++++++-------------
>>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> So you are adding more logic to the kernel for no change at all?
>
> Why is this needed?  What changed here except the underlying data
> structure being used?

Hi Greg, the goal of the change I'm proposing here is to increase the
maximum number of dynamic misc devices from 128 to 1048448. And then the
motivation is that 128 isn't enough for all the coresight_tmc devices we
have on the AmpereOne processor, where there are independent coresight
devices for each cpu. Maybe a clearer commit message to convey that
would be?:

 char: misc: Increase the maximum number of dynamic misc devices from 128 to 1048448

 On AmpereOne, 128 dynamic misc devices is not enough for the per-cpu
 coresight_tmc devices.  Switch the dynamic minors allocator to an ida
 and add logic to allocate in the ranges [0..127] and
 [256..MINORMASK]. Allocations start from 127 growing downwards and then
 increasing from 256, so device numbering for the first 128 devices
 remain the same as before.

Re: [PATCH] char: misc: Allow minors values up to MINORMASK

[Greg Kroah-Hartman]

On Fri, Sep 09, 2022 at 11:18:54AM -0700, D Scott Phillips wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, Sep 06, 2022 at 12:52:13PM -0700, D Scott Phillips wrote:
> >> For per-cpu coresight_tmc devices, we can end up with hundreds of devices
> >> on large systems that all want a dynamic minor number.  Switch the dynamic
> >> minors allocator to an ida and add logic to allocate in the ranges [0..127]
> >> and [256..MINORMASK]. Allocations start from 127 growing downwards and then
> >> increasing from 256, so device numbering for the first 128 devices should
> >> be the same as before.
> >> 
> >> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> >> ---
> >>  drivers/char/misc.c | 41 ++++++++++++++++++++++++++++-------------
> >>  1 file changed, 28 insertions(+), 13 deletions(-)
> >
> > So you are adding more logic to the kernel for no change at all?
> >
> > Why is this needed?  What changed here except the underlying data
> > structure being used?
> 
> Hi Greg, the goal of the change I'm proposing here is to increase the
> maximum number of dynamic misc devices from 128 to 1048448.

You never said that anywhere in the changelog text :(

What would you like to see if you had to review such a change?  Please
fix up the changelog text to make sense when having to describe it to
someone who has no prior knowledge of what you were trying to do.

Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what is needed in order
to properly describe the change when you resend this.

thanks,

greg k-h