Re: [PATCH] Reduce stack usage in sg.c

["Randy.Dunlap" <rddunlap@osdl.org>]

Yum Rayan wrote:
> Attempt to reduce stack usage in sg.c.
> 
> Used checkstack.pl script to measure stack usage and noted follows:
> 
> Before patch:
> sg_ioctl - 412
> sg_read - 200
> 
> After patch:
> sg_ioctl - 92
> sg_read -  96
> 
> Thanks,
> Rayan
> 
> Signed-off-by: Yum Rayan <yum.rayan@gmail.com>

Mostly looks OK to me.  However, I would rather see patches
against the current -bk instead of 2.6.x.y.
This one does not apply to 2.6.12-rc1-bk1.
AFAIK, development is still primarily done against the linus-bk
tree and not the x.y release tree.

Hunk #1 FAILED at 335.
Hunk #2 succeeded at 971 with fuzz 2 (offset -6 lines).
Hunk #3 succeeded at 997 (offset -6 lines).
1 out of 3 hunks FAILED -- saving rejects to file drivers/scsi/sg.c.rej


> diff -Nur linux-2.6.11.5.orig/drivers/scsi/sg.c linux-2.6.11.5/drivers/scsi/sg.c
> --- linux-2.6.11.5.orig/drivers/scsi/sg.c	2005-03-18 22:34:56.000000000 -0800
> +++ linux-2.6.11.5/drivers/scsi/sg.c	2005-03-21 23:48:52.000000000 -0800
> @@ -335,109 +335,143 @@
>  	Sg_fd *sfp;
>  	Sg_request *srp;
>  	int req_pack_id = -1;
> -	struct sg_header old_hdr;
> -	sg_io_hdr_t new_hdr;
> +	struct sg_header *old_hdr;
>  	sg_io_hdr_t *hp;
> +	int retval = 0;
> +	
>  
>  	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
>  		return -ENXIO;
>  	SCSI_LOG_TIMEOUT(3, printk("sg_read: %s, count=%d\n",
>  				   sdp->disk->disk_name, (int) count));
> -	if ((k = verify_area(VERIFY_WRITE, buf, count)))
> -		return k;
> +	old_hdr = kmalloc(SZ_SG_HEADER, GFP_KERNEL);
> +	if (!old_hdr)
> +		return -ENOMEM;
> +	if ((k = verify_area(VERIFY_WRITE, buf, count))) {
> +		retval = k;
> +		goto free_old_hdr;
> +	}
>  	if (sfp->force_packid && (count >= SZ_SG_HEADER)) {
> -		if (__copy_from_user(&old_hdr, buf, SZ_SG_HEADER))
> -			return -EFAULT;
> -		if (old_hdr.reply_len < 0) {
> +		if (__copy_from_user(old_hdr, buf, SZ_SG_HEADER)){
> +			retval = -EFAULT;
> +			goto free_old_hdr;
> +		}
> +		if (old_hdr->reply_len < 0) {
>  			if (count >= SZ_SG_IO_HDR) {
> -				if (__copy_from_user
> -				    (&new_hdr, buf, SZ_SG_IO_HDR))
> -					return -EFAULT;
> -				req_pack_id = new_hdr.pack_id;
> +				sg_io_hdr_t *new_hdr;
> +				new_hdr = kmalloc(SZ_SG_IO_HDR,	GFP_KERNEL);
> +				if (!new_hdr) {
> +					retval = -ENOMEM;
> +					goto free_old_hdr;
> +				}
> +				retval = __copy_from_user(new_hdr, buf,
> +								SZ_SG_IO_HDR);
> +				req_pack_id = new_hdr->pack_id;
> +				kfree(new_hdr);
> +				if (retval) {
> +					retval = -EFAULT;
> +					goto free_old_hdr;
> +				}
>  			}
>  		} else
> -			req_pack_id = old_hdr.pack_id;
> +			req_pack_id = old_hdr->pack_id;
>  	}
>  	srp = sg_get_rq_mark(sfp, req_pack_id);
>  	if (!srp) {		/* now wait on packet to arrive */
> -		if (sdp->detached)
> -			return -ENODEV;
> -		if (filp->f_flags & O_NONBLOCK)
> -			return -EAGAIN;
> +		if (sdp->detached) {
> +			retval = -ENODEV;
> +			goto free_old_hdr;
> +		}
> +		if (filp->f_flags & O_NONBLOCK) {
> +			retval = -EAGAIN;
> +			goto free_old_hdr;
> +		}

This patch section looks suspect to me:
Notice that the second '+' sign begins an illegal construct,
probably missing an 'if' line or 2.
Or the '+' should be '-'.

>  		while (1) {
> -			res = 0;	/* following is a macro that beats race condition */
> +			res = 0; /* following macro beats race condition */
>  			__wait_event_interruptible(sfp->read_wait,
> -				(sdp->detached || (srp = sg_get_rq_mark(sfp, req_pack_id))), 
> -						   res);
> -			if (sdp->detached)
> -				return -ENODEV;
> +				(sdp->detached ||
> +				(srp = sg_get_rq_mark(sfp, req_pack_id))), 
> +				 res);
> +			if (sdp->detached) {
> +				retval = -ENODEV;
> +				goto free_old_hdr;
> +			}
>  			if (0 == res)
>  				break;
> -			return res;	/* -ERESTARTSYS because signal hit process */
> +			retval = res; /* -ERESTARTSYS as signal hit process */
> +			goto free_old_hdr;
>  		}
>  	}
> -	if (srp->header.interface_id != '\0')
> -		return sg_new_read(sfp, buf, count, srp);
> +	if (srp->header.interface_id != '\0') {
> +		retval = sg_new_read(sfp, buf, count, srp);
> +		goto free_old_hdr;
> +	}
>  
>  	hp = &srp->header;
> -	memset(&old_hdr, 0, SZ_SG_HEADER);
> -	old_hdr.reply_len = (int) hp->timeout;
> -	old_hdr.pack_len = old_hdr.reply_len; /* very old, strange behaviour */
> -	old_hdr.pack_id = hp->pack_id;
> -	old_hdr.twelve_byte =
> +	memset(old_hdr, 0, SZ_SG_HEADER);
> +	old_hdr->reply_len = (int) hp->timeout;
> +	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
> +	old_hdr->pack_id = hp->pack_id;
> +	old_hdr->twelve_byte =
>  	    ((srp->data.cmd_opcode >= 0xc0) && (12 == hp->cmd_len)) ? 1 : 0;
> -	old_hdr.target_status = hp->masked_status;
> -	old_hdr.host_status = hp->host_status;
> -	old_hdr.driver_status = hp->driver_status;
> +	old_hdr->target_status = hp->masked_status;
> +	old_hdr->host_status = hp->host_status;
> +	old_hdr->driver_status = hp->driver_status;
>  	if ((CHECK_CONDITION & hp->masked_status) ||
>  	    (DRIVER_SENSE & hp->driver_status))
> -		memcpy(old_hdr.sense_buffer, srp->sense_b,
> -		       sizeof (old_hdr.sense_buffer));
> +		memcpy(old_hdr->sense_buffer, srp->sense_b,
> +		       sizeof (old_hdr->sense_buffer));
>  	switch (hp->host_status) {
>  	/* This setup of 'result' is for backward compatibility and is best
>  	   ignored by the user who should use target, host + driver status */
>  	case DID_OK:
>  	case DID_PASSTHROUGH:
>  	case DID_SOFT_ERROR:
> -		old_hdr.result = 0;
> +		old_hdr->result = 0;
>  		break;
>  	case DID_NO_CONNECT:
>  	case DID_BUS_BUSY:
>  	case DID_TIME_OUT:
> -		old_hdr.result = EBUSY;
> +		old_hdr->result = EBUSY;
>  		break;
>  	case DID_BAD_TARGET:
>  	case DID_ABORT:
>  	case DID_PARITY:
>  	case DID_RESET:
>  	case DID_BAD_INTR:
> -		old_hdr.result = EIO;
> +		old_hdr->result = EIO;
>  		break;
>  	case DID_ERROR:
> -		old_hdr.result = (srp->sense_b[0] == 0 && 
> +		old_hdr->result = (srp->sense_b[0] == 0 && 
>  				  hp->masked_status == GOOD) ? 0 : EIO;
>  		break;
>  	default:
> -		old_hdr.result = EIO;
> +		old_hdr->result = EIO;
>  		break;
>  	}
>  
>  	/* Now copy the result back to the user buffer.  */
>  	if (count >= SZ_SG_HEADER) {
> -		if (__copy_to_user(buf, &old_hdr, SZ_SG_HEADER))
> -			return -EFAULT;
> +		if (__copy_to_user(buf, old_hdr, SZ_SG_HEADER)) {
> +			retval = -EFAULT;
> +			goto free_old_hdr;
> +		}
>  		buf += SZ_SG_HEADER;
> -		if (count > old_hdr.reply_len)
> -			count = old_hdr.reply_len;
> +		if (count > old_hdr->reply_len)
> +			count = old_hdr->reply_len;
>  		if (count > SZ_SG_HEADER) {
>  			if ((res =
>  			     sg_read_oxfer(srp, buf, count - SZ_SG_HEADER)))
> -				return -EFAULT;
> +				retval = -EFAULT;
> +				goto free_old_hdr;
>  		}
>  	} else
> -		count = (old_hdr.result == 0) ? 0 : -EIO;
> +		count = (old_hdr->result == 0) ? 0 : -EIO;
>  	sg_finish_rem_req(srp);
> -	return count;
> +	retval = count;
> +free_old_hdr:
> +	kfree(old_hdr);
> +	return retval;
>  }
>  
>  static ssize_t
> @@ -943,8 +977,11 @@
>  		if (result)
>  			return result;
>  		else {
> -			sg_req_info_t rinfo[SG_MAX_QUEUE];
> -			Sg_request *srp;
> +			sg_req_info_t *rinfo;
> +			rinfo = kmalloc(SG_MAX_QUEUE * SZ_SG_REQ_INFO,
> +								GFP_KERNEL);
> +			if (!rinfo)
> +				return -ENOMEM;
>  			read_lock_irqsave(&sfp->rq_list_lock, iflags);
>  			for (srp = sfp->headrp, val = 0; val < SG_MAX_QUEUE;
>  			     ++val, srp = srp ? srp->nextrp : srp) {
> @@ -966,8 +1003,11 @@
>  				}
>  			}
>  			read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> -			return (__copy_to_user(p, rinfo,
> -			        SZ_SG_REQ_INFO * SG_MAX_QUEUE) ? -EFAULT : 0);
> +			result = __copy_to_user(p, rinfo, SZ_SG_REQ_INFO *
> +							SG_MAX_QUEUE);
> +			result = result ? -EFAULT : 0;
> +			kfree(rinfo);
> +			return result;
>  		}
>  	case SG_EMULATED_HOST:
>  		if (sdp->detached)
> -

-- 
~Randy