[PATCH] reduce stack in cpqarray.c::ida_ioctl()

[PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Randy.Dunlap]

Hi,

This patch to 2.5.66 reduces stack usage in ida_ioctl() by about
0x500 bytes (on x86).

There is a possibility that the allocation here should be done one time
only and the buffer pointer saved for re-use instead of allocating it
on each call to ida_ioctl.  If that's desirable, I'll have a few
questions.

Comments on the patch?

Thanks,
--
~Randy


patch_name:	cpqarray-stack.patch
patch_version:	2003-04-02.21:10:01
author:		Randy.Dunlap <rddunlap@osdl.org>
description:	reduce stack in block/cqparray.c::ida_ioctl()
		from 0x550 to 0x48;
product:	Linux
product_versions: linux-2566
changelog:	allocate and free <ida_ioctl_t>
maintainer:	Steve Cameron: arrays@hp.com, steve.cameron@hp.com
diffstat:	=
 drivers/block/cpqarray.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)


diff -Naur ./drivers/block/cpqarray.c%IDAIOC ./drivers/block/cpqarray.c
--- ./drivers/block/cpqarray.c%IDAIOC	2003-03-24 14:00:03.000000000 -0800
+++ ./drivers/block/cpqarray.c	2003-04-02 20:05:57.000000000 -0800
@@ -1021,7 +1021,7 @@
 	int diskinfo[4];
 	struct hd_geometry *geo = (struct hd_geometry *)arg;
 	ida_ioctl_t *io = (ida_ioctl_t*)arg;
-	ida_ioctl_t my_io;
+	ida_ioctl_t *my_io;
 
 	switch(cmd) {
 	case HDIO_GETGEO:
@@ -1046,11 +1046,19 @@
 		return 0;
 	case IDAPASSTHRU:
 		if (!capable(CAP_SYS_RAWIO)) return -EPERM;
-		if (copy_from_user(&my_io, io, sizeof(my_io)))
-			return -EFAULT;
-		error = ida_ctlr_ioctl(ctlr, dsk, &my_io);
-		if (error) return error;
-		return copy_to_user(io, &my_io, sizeof(my_io)) ? -EFAULT : 0;
+		my_io = kmalloc(sizeof(ida_ioctl_t), GFP_KERNEL);
+		if (!my_io)
+			return -ENOMEM;
+		if (copy_from_user(my_io, io, sizeof(*my_io))) {
+			error = -EFAULT;
+			goto iocfree;
+		}
+		error = ida_ctlr_ioctl(ctlr, dsk, my_io);
+		if (error) goto iocfree;
+		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
+  iocfree:
+		kfree(my_io);
+		return error;
 	case IDAGETCTLRSIG:
 		if (!arg) return -EINVAL;
 		put_user(hba[ctlr]->ctlr_sig, (int*)arg);

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Randy.Dunlap]

On Thu, 3 Apr 2003 14:18:57 -0600 "Cameron, Steve" <Steve.Cameron@hp.com> wrote:

| 
| > This patch to 2.5.66 reduces stack usage in ida_ioctl() by about
| > 0x500 bytes (on x86).
| 
| Looks ok to me, (but i haven't tried it.)

Me either (-ENOHARDWARE).
 
| > There is a possibility that the allocation here should be done one time
| > only and the buffer pointer saved for re-use instead of allocating it
| > on each call to ida_ioctl.  If that's desirable, I'll have a few
| > questions.
| 
| No, I don't think so.  I think we should
| allow for the possibllity of concurrent calls 
| to this ioctl.  (whether linux allows it is 
| another question.  I think it used to be the case
| that only one ioctl could get in at a time... I can't
| recall if it's still that way.)

Well, my questions were along those lines, like:
. allocate a buffer per hba or per disk?
. how much is ida_ioctl() [passthru] used?
. it's not on a normal(!?!) read path, is it?
  if so, using kmalloc() that could fail would be a bad idea.

--
~Randy

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Randy.Dunlap]

On Fri, 4 Apr 2003 02:30:44 +0200 Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:

| On Thu, 3 April 2003 12:03:08 +0000, Randy.Dunlap wrote:
| > 
| > Comments on the patch?
| 
| Your patch looks just fine. The original code is a bit odd, though.
| If you want to change that in one go, read on. If not, please ignore
| this.
| 
| > +		error = ida_ctlr_ioctl(ctlr, dsk, my_io);
| > +		if (error) goto iocfree;
| 
| Wouldn't an extra line be nicer?

Probably.  I would normally do that.  In this case I was just
maintaining the style that is already used in that source file.

| > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
| 
| copy_to_user returns the bytes successfully copied.
| error is set to -EFAULT, if there was actually data transferred?

Did you verify that?


| How about:
| +		error = copy_to_user(io, my_io, sizeof(*my_io)) < sizeof(*my_io) ? -EFAULT : 0;


--
~Randy

RE: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Cameron, Steve]

> This patch to 2.5.66 reduces stack usage in ida_ioctl() by about
> 0x500 bytes (on x86).

Looks ok to me, (but i haven't tried it.)

>
> There is a possibility that the allocation here should be done one time
> only and the buffer pointer saved for re-use instead of allocating it
> on each call to ida_ioctl.  If that's desirable, I'll have a few
> questions.

No, I don't think so.  I think we should
allow for the possibllity of concurrent calls 
to this ioctl.  (whether linux allows it is 
another question.  I think it used to be the case
that only one ioctl could get in at a time... I can't
recall if it's still that way.)

-- steve

RE: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Cameron, Steve]

> Well, my questions were along those lines, like:
> . allocate a buffer per hba or per disk?

I don't think the number of concurrent ioctls 
apps might try to send is governed by number of
hba's or number of disks (which are virtualized)
or number of physical disks (which the driver doesn't
know.)  It's whatever the apps want to try to send
down.

> . how much is ida_ioctl() [passthru] used?

Not much by most people, I expect. It's not in
the main i/o path, definitely.

it's used by things like our online Array Config Utility
(for configuring new volumes after adding new hardware
such as hot plugging some disks into a storage box...)

Also used by Compaq Insight Manager agents for 
monitoring.

Flashing HBA firmware...

> . it's not on a normal(!?!) read path, is it?

no.

>  if so, using kmalloc() that could fail would be a bad idea.

--
~Randy

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Jörn Engel]

On Thu, 3 April 2003 12:03:08 +0000, Randy.Dunlap wrote:
> 
> Comments on the patch?

Your patch looks just fine. The original code is a bit odd, though.
If you want to change that in one go, read on. If not, please ignore
this.

> diff -Naur ./drivers/block/cpqarray.c%IDAIOC ./drivers/block/cpqarray.c
> --- ./drivers/block/cpqarray.c%IDAIOC	2003-03-24 14:00:03.000000000 -0800
> +++ ./drivers/block/cpqarray.c	2003-04-02 20:05:57.000000000 -0800
> @@ -1021,7 +1021,7 @@
>  	int diskinfo[4];
>  	struct hd_geometry *geo = (struct hd_geometry *)arg;
>  	ida_ioctl_t *io = (ida_ioctl_t*)arg;
> -	ida_ioctl_t my_io;
> +	ida_ioctl_t *my_io;
>  
>  	switch(cmd) {
>  	case HDIO_GETGEO:
> @@ -1046,11 +1046,19 @@
>  		return 0;
>  	case IDAPASSTHRU:
>  		if (!capable(CAP_SYS_RAWIO)) return -EPERM;
> -		if (copy_from_user(&my_io, io, sizeof(my_io)))
> -			return -EFAULT;
> -		error = ida_ctlr_ioctl(ctlr, dsk, &my_io);
> -		if (error) return error;
> -		return copy_to_user(io, &my_io, sizeof(my_io)) ? -EFAULT : 0;
> +		my_io = kmalloc(sizeof(ida_ioctl_t), GFP_KERNEL);
> +		if (!my_io)
> +			return -ENOMEM;
> +		if (copy_from_user(my_io, io, sizeof(*my_io))) {
> +			error = -EFAULT;
> +			goto iocfree;
> +		}
> +		error = ida_ctlr_ioctl(ctlr, dsk, my_io);
> +		if (error) goto iocfree;

Wouldn't an extra line be nicer?

> +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;

copy_to_user returns the bytes successfully copied.
error is set to -EFAULT, if there was actually data transferred?

How about:
+		error = copy_to_user(io, my_io, sizeof(*my_io)) < sizeof(*my_io) ? -EFAULT : 0;

> +  iocfree:
> +		kfree(my_io);
> +		return error;
>  	case IDAGETCTLRSIG:
>  		if (!arg) return -EINVAL;
>  		put_user(hba[ctlr]->ctlr_sig, (int*)arg);

Jörn

-- 
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Jörn Engel]

On Thu, 3 April 2003 17:33:52 +0000, Randy.Dunlap wrote:
> 
> | > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
> | 
> | copy_to_user returns the bytes successfully copied.
> | error is set to -EFAULT, if there was actually data transferred?
> 
> Did you verify that?

Yes, but I do make mistakes. Better double-check it yourself.

Jörn

-- 
"Error protection by error detection and correction."
-- from a university class

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Jens Axboe]

On Fri, Apr 04 2003, Jörn Engel wrote:
> > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
> 
> copy_to_user returns the bytes successfully copied.
> error is set to -EFAULT, if there was actually data transferred?
> 
> How about:
> +		error = copy_to_user(io, my_io, sizeof(*my_io)) < sizeof(*my_io) ? -EFAULT : 0;

Pure nonsense! Correct logic, and much nicer to read IMO is:

	if (copy_to_user(io, my_io, sizeof(*my_io))
		error = -EFAULT;

-- 
Jens Axboe

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Randy.Dunlap]

On Fri, 4 Apr 2003 09:56:52 +0200 Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:

| On Thu, 3 April 2003 17:33:52 +0000, Randy.Dunlap wrote:
| > 
| > | > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
| > | 
| > | copy_to_user returns the bytes successfully copied.
| > | error is set to -EFAULT, if there was actually data transferred?
| > 
| > Did you verify that?
| 
| Yes, but I do make mistakes. Better double-check it yourself.

Oh, I did.  That's why I am asking you.

--
~Randy

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Jörn Engel]

On Fri, 4 April 2003 10:09:37 +0200, Jens Axboe wrote:
> On Fri, Apr 04 2003, Jörn Engel wrote:
> > > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
> > 
> > copy_to_user returns the bytes successfully copied.
> > error is set to -EFAULT, if there was actually data transferred?
> > 
> > How about:
> > +		error = copy_to_user(io, my_io, sizeof(*my_io)) < sizeof(*my_io) ? -EFAULT : 0;
> 
> Pure nonsense! Correct logic, and much nicer to read IMO is:
> 
> 	if (copy_to_user(io, my_io, sizeof(*my_io))
> 		error = -EFAULT;

Yes, you are right. I just couldn't read inline assembler properly.

Jörn

-- 
With a PC, I always felt limited by the software available. On Unix, 
I am limited only by my knowledge.
-- Peter J. Schoenster

Re: [PATCH] reduce stack in cpqarray.c::ida_ioctl()

[Jörn Engel]

On Fri, 4 April 2003 08:44:09 +0000, Randy.Dunlap wrote:
> On Fri, 4 Apr 2003 09:56:52 +0200 Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
> 
> | On Thu, 3 April 2003 17:33:52 +0000, Randy.Dunlap wrote:
> | > 
> | > | > +		error = copy_to_user(io, my_io, sizeof(*my_io)) ? -EFAULT : 0;
> | > | 
> | > | copy_to_user returns the bytes successfully copied.
> | > | error is set to -EFAULT, if there was actually data transferred?
> | > 
> | > Did you verify that?
> | 
> | Yes, but I do make mistakes. Better double-check it yourself.
> 
> Oh, I did.  That's why I am asking you.

It was my mistake. I got confused reading uaccess.h. Sorry about that.

Jörn

-- 
Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt