LinuxLists.cc - Allow setting of number of raw devices as a module parameter

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Fri, Apr 29, 2011 at 12:24:29AM +0200, 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]>
>
> ---
> 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

2011-04-29 23:34:33

by Andrew Morton

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

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.

> +static int max_raw_minors = MAX_RAW_MINORS;

I suppose we could remove MAX_RAW_MINORS and use CONFIG_MAX_RAW_DEVS
directly.

> +
> +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);

vzalloc().

2011-04-30 00:07:54

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Fri, Apr 29, 2011 at 04:28:17PM -0700, Andrew Morton wrote:
> 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

2011-04-30 05:10:01

by Dave Jones

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Fri, Apr 29, 2011 at 05:07:04PM -0700, Greg KH wrote:
> On Fri, Apr 29, 2011 at 04:28:17PM -0700, Andrew Morton wrote:
> > 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.

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

2011-04-30 05:41:31

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Sat, Apr 30, 2011 at 01:09:51AM -0400, Dave Jones wrote:
> On Fri, Apr 29, 2011 at 05:07:04PM -0700, Greg KH wrote:
> > On Fri, Apr 29, 2011 at 04:28:17PM -0700, Andrew Morton wrote:
> > > 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.
>
> 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

2011-04-30 10:28:34

by Alan

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Fri, 29 Apr 2011 16:28:17 -0700
Andrew Morton <[email protected]> wrote:

> 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)

2011-04-30 12:15:16

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

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

2011-04-30 15:34:59

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Sat, Apr 30, 2011 at 11:29:37AM +0100, Alan Cox wrote:
> On Fri, 29 Apr 2011 16:28:17 -0700
> Andrew Morton <[email protected]> wrote:
>
> > 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)

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.

2011-04-30 15:40:37

by Alan

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

> > 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.

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

2011-04-30 15:46:34

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Sat, Apr 30, 2011 at 04:41:41PM +0100, Alan Cox wrote:
> > > 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.
>
> 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

2011-05-02 19:23:00

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Sat 30-04-11 16:41:41, Alan Cox wrote:
> > > 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.
>
> 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
--
Jan Kara <[email protected]>
SUSE Labs, CR

2011-05-02 19:39:40

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Sat 30-04-11 14:15:02, Arnd Bergmann wrote:
> 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.
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
--
Jan Kara <[email protected]>
SUSE Labs, CR

2011-05-02 19:45:06

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Monday 02 May 2011, Jan Kara wrote:
> On Sat 30-04-11 14:15:02, Arnd Bergmann wrote:
> > 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.
> 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

2011-05-02 21:11:44

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Mon 02-05-11 21:44:31, Arnd Bergmann wrote:
> On Monday 02 May 2011, Jan Kara wrote:
> > On Sat 30-04-11 14:15:02, Arnd Bergmann wrote:
> > > 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.
> > 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.
OK, but if I don't have the inode of the character device (like in
RAW_GETBIND case)?

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

2011-05-03 09:42:19

by Alan

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

> 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

2011-05-03 10:55:18

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Monday 02 May 2011, Jan Kara wrote:
> >
> > 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

2011-05-03 17:04:12

[permalink] [raw]

Subject: Re: Allow setting of number of raw devices as a module parameter

On Tue 03-05-11 12:55:06, Arnd Bergmann wrote:
> On Monday 02 May 2011, Jan Kara wrote:
> > >
> > > 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.
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
--
Jan Kara <[email protected]>
SUSE Labs, CR

2011-05-03 17:30:40