Allow setting of number of raw devices as a module parameter

PATCH Jan Kara 7 participants 19 messages Apr 28, 2011 – May 3, 2011 Char and misc drivers

Share & tools

Jan Kara

author

2011-04-28 22:24 UTC #0 lore raw

Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).

Signed-off-by Jan Kara <[email protected]>

---
 drivers/char/Kconfig |    2 +-
 drivers/char/raw.c   |   33 +++++++++++++++++++++++++++------
 2 files changed, 28 insertions(+), 7 deletions(-)

We are carrying this patch in SUSE kernels for a few years already (some
customer requested this feature). Can it be merged upstream? I'm not sure who'd be the right maintainer though...

drivers/char/Kconfig +1 -1

@@ -1059,7 +1059,7 @@ config RAW_DRIVER
           with the O_DIRECT flag.
 
 config MAX_RAW_DEVS
-	int "Maximum number of RAW devices to support (1-8192)"
+	int "Maximum number of RAW devices to support (1-65536)"
 	depends on RAW_DRIVER
 	default "256"
 	help

drivers/char/raw.c +27 -6

@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/gfp.h>
 #include <linux/compat.h>
+#include <linux/vmalloc.h>
 
 #include <asm/uaccess.h>

@@ -30,10 +31,15 @@ struct raw_device_data {
 };
 
 static struct class *raw_class;
-static struct raw_device_data raw_devices[MAX_RAW_MINORS];
+static struct raw_device_data *raw_devices;
 static DEFINE_MUTEX(raw_mutex);
 static const struct file_operations raw_ctl_fops; /* forward declaration */
 
+static int max_raw_minors = MAX_RAW_MINORS;
+
+module_param(max_raw_minors, int, 0);
+MODULE_PARM_DESC(max_raw_minors, "Maximum number of raw devices (1-65536)");
+
 /*
  * Open/close code for raw IO.
  *

@@ -131,7 +137,7 @@ static int bind_set(int number, u64 majo
 	struct raw_device_data *rawdev;
 	int err = 0;
 
-	if (number <= 0 || number >= MAX_RAW_MINORS)
+	if (number <= 0 || number >= max_raw_minors)
 		return -EINVAL;
 
 	if (MAJOR(dev) != major || MINOR(dev) != minor)

@@ -318,12 +324,26 @@ static int __init raw_init(void)
 	dev_t dev = MKDEV(RAW_MAJOR, 0);
 	int ret;
 
-	ret = register_chrdev_region(dev, MAX_RAW_MINORS, "raw");
+	if (max_raw_minors < 1 || max_raw_minors > 65536) {
+		printk(KERN_WARNING "raw: invalid max_raw_minors (must be"
+			" between 1 and 65536), using %d\n", MAX_RAW_MINORS);
+		max_raw_minors = MAX_RAW_MINORS;
+	}
+
+	raw_devices = vmalloc(sizeof(struct raw_device_data) * max_raw_minors);
+	if (!raw_devices) {
+		printk(KERN_ERR "Not enough memory for raw device structures\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+	memset(raw_devices, 0, sizeof(struct raw_device_data) * max_raw_minors);
+
+	ret = register_chrdev_region(dev, max_raw_minors, "raw");
 	if (ret)
 		goto error;
 
 	cdev_init(&raw_cdev, &raw_fops);
-	ret = cdev_add(&raw_cdev, dev, MAX_RAW_MINORS);
+	ret = cdev_add(&raw_cdev, dev, max_raw_minors);
 	if (ret) {
 		kobject_put(&raw_cdev.kobj);
 		goto error_region;

@@ -342,8 +362,9 @@ static int __init raw_init(void)
 	return 0;
 
 error_region:
-	unregister_chrdev_region(dev, MAX_RAW_MINORS);
+	unregister_chrdev_region(dev, max_raw_minors);
 error:
+	vfree(raw_devices);
 	return ret;
 }

@@ -352,7 +373,7 @@ static void __exit raw_exit(void)
 	device_destroy(raw_class, MKDEV(RAW_MAJOR, 0));
 	class_destroy(raw_class);
 	cdev_del(&raw_cdev);
-	unregister_chrdev_region(MKDEV(RAW_MAJOR, 0), MAX_RAW_MINORS);
+	unregister_chrdev_region(MKDEV(RAW_MAJOR, 0), max_raw_minors);
 }
 
 module_init(raw_init);

Greg KH

2011-04-29 23:19 UTC #1 lore raw

Quoted 6 lines · Jan Kara

Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Signed-off-by Jan Kara <[email protected]>

Quoted 8 lines

---
 drivers/char/Kconfig |    2 +-
 drivers/char/raw.c   |   33 +++++++++++++++++++++++++++------
 2 files changed, 28 insertions(+), 7 deletions(-)
We are carrying this patch in SUSE kernels for a few years already (some
customer requested this feature). Can it be merged upstream? I'm not sure who'd be the right maintainer though...

I've touched the raw driver a number of times over the years. I tried to get rid of it in the past, and was told that it had to stay.

So, as the driver isn't going anywhere any year soon we should apply this patch, I'll queue it up and take it through my tree unless someone strongly objects?

thanks,

greg k-h

Greg KH

2011-04-30 00:07 UTC #3 lore raw

Quoted 10 lines · Andrew Morton

Quoted 4 lines · On Fri, 29 Apr 2011 00:24:29 +0200 Jan Kara <[email protected]> wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Changelog failed to describe why the patch is needed. Lacking that information, we have no reason to apply it.

Good point, how about:
People like running their machines with thousands of raw devices, yet distros don't want to take up that much memory for all users. Provide a way for everyone to be happy without forcing the module to be rebuilt by providing the ability to specify the number of raw devices as a module parameter option.

thanks,

greg k-h

Dave Jones

2011-04-30 05:10 UTC #4 lore raw

Quoted 12 lines · Greg KH

Quoted 10 lines · Andrew Morton
Quoted 4 lines · On Fri, 29 Apr 2011 00:24:29 +0200 Jan Kara <[email protected]> wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Changelog failed to describe why the patch is needed. Lacking that information, we have no reason to apply it.

Quoted 6 lines

Good point, how about:
People like running their machines with thousands of raw devices, yet distros don't want to take up that much memory for all users. Provide a way for everyone to be happy without forcing the module to be rebuilt by providing the ability to specify the number of raw devices as a module parameter option.

I remember when this was made a config option, thinking "8192 raw devices? who would need that many?". I have to wonder just how someone manages a system with an insane number of these devices.

Dave

Greg KH

2011-04-30 05:41 UTC #5 lore raw

Quoted 20 lines · Dave Jones

Quoted 12 lines · Greg KH
Quoted 10 lines · Andrew Morton
Quoted 4 lines · On Fri, 29 Apr 2011 00:24:29 +0200 Jan Kara <[email protected]> wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Changelog failed to describe why the patch is needed. Lacking that information, we have no reason to apply it.
Quoted 6 lines
Good point, how about:
People like running their machines with thousands of raw devices, yet distros don't want to take up that much memory for all users. Provide a way for everyone to be happy without forcing the module to be rebuilt by providing the ability to specify the number of raw devices as a module parameter option.

Quoted 3 lines

I remember when this was made a config option, thinking "8192 raw devices? who would need that many?". I have to wonder just how someone manages a system with an insane number of these devices.

Think of a very large database benchmark with many thousands of "disks" that are used to get very fast throughput numbers...

As for how they manage them, there's tools for that that companies are very happy to sell you to make it easier (IBM, CA, etc.)

Bizarre, yes, unrealistic, no :(

thanks,

greg k-h

Alan Cox

2011-04-30 10:28 UTC #6 lore raw

In reply to Andrew Morton

Quoted 6 lines · On Fri, 29 Apr 2011 16:28:17 -0700 Andrew Morton <[email protected]>…

Quoted 3 lines · On Fri, 29 Apr 2011 00:24:29 +0200 Jan Kara <[email protected]> wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).

A large vmalloc array is very antisocial on a 32bit x86 box. It looks like almost all of it would become sane if there was an array of pointers to raw devices and the devices were initially allocated on need (even if for now only recovered on rmmod)

Greg KH

2011-04-30 15:34 UTC #7 lore raw

Quoted 6 lines · Alan Cox

Quoted 3 lines · On Fri, 29 Apr 2011 16:28:17 -0700 Andrew Morton <[email protected]>…
On Fri, 29 Apr 2011 00:24:29 +0200
Jan Kara <[email protected]> wrote:

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
A large vmalloc array is very antisocial on a 32bit x86 box. It looks like almost all of it would become sane if there was an array of pointers to raw devices and the devices were initially allocated on need (even if for now only recovered on rmmod)

In practice, we've never seen a problem with this[1]. Machines that want thousands of raw devices have plenty of memory to handle this situation.

I doubt adding the complexity of dynamically allocating the devices as-needed is worth it for the very few systems that ever use this driver, compounded with the fact that we keep saying that this code isn't to be used by "normal" people anyway.

thanks,

greg k-h

[1] Again, this has been shipping for years in SLES kernels with no reported bug reports. I know that's not really a valid reason to do things, but it is a data point for the simplicity of this patch.

Alan Cox

2011-04-30 15:40 UTC #8 lore raw

Quoted 5 lines

Quoted 4 lines
A large vmalloc array is very antisocial on a 32bit x86 box. It looks like almost all of it would become sane if there was an array of pointers to raw devices and the devices were initially allocated on need (even if for now only recovered on rmmod)

Quoted 8 lines

In practice, we've never seen a problem with this[1]. Machines that want thousands of raw devices have plenty of memory to handle this situation.
I doubt adding the complexity of dynamically allocating the devices as-needed is worth it for the very few systems that ever use this driver, compounded with the fact that we keep saying that this code isn't to be used by "normal" people anyway.

That's no excuse for sloppy code. Making it an array populated on demand is an improvement for everyone, making it vmalloc hurts every access (in TLB misses for one as well as vmalloc overhead).

I note two of us immediately made the same observation. Doing it basically right (array of pointers) is easy. Doing the full works with a hash for the lookups is a bit harder and that I would agree is overkill.

Alan

Greg KH

2011-04-30 15:46 UTC #9 lore raw

Quoted 14 lines · Alan Cox

Quoted 5 lines
Quoted 4 lines
A large vmalloc array is very antisocial on a 32bit x86 box. It looks like almost all of it would become sane if there was an array of pointers to raw devices and the devices were initially allocated on need (even if for now only recovered on rmmod)
Quoted 8 lines
In practice, we've never seen a problem with this[1]. Machines that want thousands of raw devices have plenty of memory to handle this situation.
I doubt adding the complexity of dynamically allocating the devices as-needed is worth it for the very few systems that ever use this driver, compounded with the fact that we keep saying that this code isn't to be used by "normal" people anyway.

Quoted 7 lines

That's no excuse for sloppy code. Making it an array populated on demand is an improvement for everyone, making it vmalloc hurts every access (in TLB misses for one as well as vmalloc overhead).
I note two of us immediately made the same observation. Doing it basically right (array of pointers) is easy. Doing the full works with a hash for the lookups is a bit harder and that I would agree is overkill.

Ah, ok, I was thinking of the latter, I'll work on implementing the former next week, unless Jan wants to do it instead?

thanks for the review,

greg k-h

Jan Kara

author

2011-05-02 19:23 UTC #10 lore raw

In reply to Alan Cox

Quoted 14 lines · Alan Cox

Quoted 5 lines
Quoted 4 lines
A large vmalloc array is very antisocial on a 32bit x86 box. It looks like almost all of it would become sane if there was an array of pointers to raw devices and the devices were initially allocated on need (even if for now only recovered on rmmod)
Quoted 8 lines
In practice, we've never seen a problem with this[1]. Machines that want thousands of raw devices have plenty of memory to handle this situation.
I doubt adding the complexity of dynamically allocating the devices as-needed is worth it for the very few systems that ever use this driver, compounded with the fact that we keep saying that this code isn't to be used by "normal" people anyway.

Quoted 7 lines

That's no excuse for sloppy code. Making it an array populated on demand is an improvement for everyone, making it vmalloc hurts every access (in TLB misses for one as well as vmalloc overhead).
I note two of us immediately made the same observation. Doing it basically right (array of pointers) is easy. Doing the full works with a hash for the lookups is a bit harder and that I would agree is overkill.

Alan, I might be missing something but is there a significant advantage
in creating the array of pointers? Note that struct raw_device_data which is what the array is from is:

struct raw_device_data {
        struct block_device *binding;
        int inuse;
};

So 16 bytes on 64-bit arch. You can make it 8 bytes by the array-of-pointers idea but people requesting this feature want to run with 8192 or even 16384 raw devices in the system (yes, they might be insane). That gives you 16 to 32 pages for array of pointers which seems too much to me for kmalloc() anyway.

So if people don't like vmalloc() (which I can understand), then I see no other option than dynamically allocating struct raw_device_data.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Alan Cox

2011-05-03 09:42 UTC #11 lore raw

Quoted 2 lines

So if people don't like vmalloc() (which I can understand), then I see no other option than dynamically allocating struct raw_device_data.

That gets rather more complicated.

So yes I agree your question is a good one - for smaller sizes I'd rather see it from the kmalloc/static pool but for small sizes it also doesn't matter much anyway.

Objection rescinded - I think you've proved convincingly that the only way to sort it properly is horribly complicated

Arnd Bergmann

maintainer

2011-04-30 12:15 UTC #12 lore raw

In reply to Jan Kara

Quoted 5 lines · On Friday 29 April 2011 00:24:29 Jan Kara wrote:

Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Signed-off-by Jan Kara <[email protected]>

Why don't you get rid of the global raw_devices array instead?

I suppose if you embed the struct cdev in raw_device_data and register it during RAW_SETBIND separately for each raw device instead of globally at raw_init time, you can support any number of devices while making the code smaller and simpler. I suppose you can even remove the raw_mutex then.

Arnd

Jan Kara

author

2011-05-02 19:39 UTC #13 lore raw

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · On Friday 29 April 2011 00:24:29 Jan Kara wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Signed-off-by Jan Kara <[email protected]>

Quoted 7 lines

Why don't you get rid of the global raw_devices array instead?
I suppose if you embed the struct cdev in raw_device_data and register it during RAW_SETBIND separately for each raw device instead of globally at raw_init time, you can support any number of devices while making the code smaller and simpler. I suppose you can even remove the raw_mutex then.

OK, I see. This might be a reasonable option. I'd then use kobj_lookup()
to find my structure where I need it instead of array indexing, right? I have really zero experience with writing device drivers so I'm asking simple questions :).

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Arnd Bergmann

maintainer

2011-05-02 19:45 UTC #14 lore raw

Quoted 15 lines · Jan Kara

Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · On Friday 29 April 2011 00:24:29 Jan Kara wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Signed-off-by Jan Kara <[email protected]>
Quoted 7 lines
Why don't you get rid of the global raw_devices array instead?
I suppose if you embed the struct cdev in raw_device_data and register it during RAW_SETBIND separately for each raw device instead of globally at raw_init time, you can support any number of devices while making the code smaller and simpler. I suppose you can even remove the raw_mutex then.

Quoted 4 lines

OK, I see. This might be a reasonable option. I'd then use kobj_lookup()
to find my structure where I need it instead of array indexing, right? I have really zero experience with writing device drivers so I'm asking simple questions :).

The character device layer uses kobj_lookup internally when opening the device. When you are inside the driver, you can access it through file->f_path.dentry->d_inode->i_cdev, from where you go to your own data structure using container_of.

Arnd

Jan Kara

author

2011-05-02 21:11 UTC #15 lore raw

Quoted 21 lines · Arnd Bergmann

Quoted 15 lines · Jan Kara
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · On Friday 29 April 2011 00:24:29 Jan Kara wrote:
Allow setting of maximal number of raw devices as a module parameter. This requires changing of static array into a vmalloced one (the array is going to be too large for kmalloc).
Signed-off-by Jan Kara <[email protected]>
Quoted 7 lines
Why don't you get rid of the global raw_devices array instead?
I suppose if you embed the struct cdev in raw_device_data and register it during RAW_SETBIND separately for each raw device instead of globally at raw_init time, you can support any number of devices while making the code smaller and simpler. I suppose you can even remove the raw_mutex then.
Quoted 4 lines
OK, I see. This might be a reasonable option. I'd then use kobj_lookup()
to find my structure where I need it instead of array indexing, right? I have really zero experience with writing device drivers so I'm asking simple questions :).

Quoted 4 lines

The character device layer uses kobj_lookup internally when opening the device. When you are inside the driver, you can access it through file->f_path.dentry->d_inode->i_cdev, from where you go to your own data structure using container_of.

OK, but if I don't have the inode of the character device (like in
RAW_GETBIND case)?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Arnd Bergmann

maintainer

2011-05-03 10:55 UTC #16 lore raw

Quoted 8 lines · Jan Kara

Quoted 5 lines
The character device layer uses kobj_lookup internally when opening the device. When you are inside the driver, you can access it through file->f_path.dentry->d_inode->i_cdev, from where you go to your own data structure using container_of.
OK, but if I don't have the inode of the character device (like in
RAW_GETBIND case)?

Oh, good point. That doesn't work as easily then. You'd have to use kobj_lookup, but you can only do that from fs/char_dev.c because otherwise the cdev_map is not visible.

So you could in theory add a cdev_lookup() function to fs/char_dev.c, but I'm not sure if that is a good idea if the only user is raw.c. It's probably not any cleaner than the approach you were suggesting.

Another idea would be to make the array dynamically sized, using some version of realloc (which doesn't exist for vmalloc today), or to use a more complex data structure for the lookup, like idr or radix_tree.

Arnd

Jan Kara

author

2011-05-03 17:04 UTC #17 lore raw

Quoted 14 lines · Arnd Bergmann

Quoted 8 lines · Jan Kara
Quoted 5 lines
The character device layer uses kobj_lookup internally when opening the device. When you are inside the driver, you can access it through file->f_path.dentry->d_inode->i_cdev, from where you go to your own data structure using container_of.
OK, but if I don't have the inode of the character device (like in
RAW_GETBIND case)?
Oh, good point. That doesn't work as easily then. You'd have to use kobj_lookup, but you can only do that from fs/char_dev.c because otherwise the cdev_map is not visible.

Quoted 7 lines

So you could in theory add a cdev_lookup() function to fs/char_dev.c, but I'm not sure if that is a good idea if the only user is raw.c. It's probably not any cleaner than the approach you were suggesting.
Another idea would be to make the array dynamically sized, using some version of realloc (which doesn't exist for vmalloc today), or to use a more complex data structure for the lookup, like idr or radix_tree.

Yes, but in that case, I find vmalloc() the smaller pain given how obscure
the driver is. Alan seems to have agreed as well so if you don't object Greg can merge the patch as is (modulo the changelog improvement from Greg)...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Arnd Bergmann

maintainer Acked-by

2011-05-03 17:30 UTC #18 lore raw

Quoted 3 lines · Jan Kara

Yes, but in that case, I find vmalloc() the smaller pain given how obscure
the driver is. Alan seems to have agreed as well so if you don't object Greg can merge the patch as is (modulo the changelog improvement from Greg)...

Yes, that's probably the best then.

Acked-by Arnd Bergmann <[email protected]>

Arnd