memory allocation in sg_io()

memory allocation in sg_io()

[Oliver Neukum]

Hi,

could you explain to me why this code can get away with allocating the
sense buffer on the stack?

static int sg_io(struct file *file, struct request_queue *q,
		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
{
	unsigned long start_time;
	int writing = 0, ret = 0, has_write_perm = 0;
	struct request *rq;
	char sense[SCSI_SENSE_BUFFERSIZE];

	Regards
		Oliver

Re: memory allocation in sg_io()

[Boaz Harrosh]

On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
> Hi,
> 
> could you explain to me why this code can get away with allocating the
> sense buffer on the stack?
> 
> static int sg_io(struct file *file, struct request_queue *q,
> 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
> {
> 	unsigned long start_time;
> 	int writing = 0, ret = 0, has_write_perm = 0;
> 	struct request *rq;
> 	char sense[SCSI_SENSE_BUFFERSIZE];
> 
> 	Regards
> 		Oliver
> -
where? what? do you mean in scsi_ioctl.c?
why not it's a synchronous call?
Do you mean 96 bytes is too big?
Do you mean DMA alignment and cache coherency? I'm working
on that for scsi devices.

Boaz

Re: memory allocation in sg_io()

[Jens Axboe]

On Thu, Jan 10 2008, Boaz Harrosh wrote:
> On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
> > Hi,
> > 
> > could you explain to me why this code can get away with allocating the
> > sense buffer on the stack?
> > 
> > static int sg_io(struct file *file, struct request_queue *q,
> > 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
> > {
> > 	unsigned long start_time;
> > 	int writing = 0, ret = 0, has_write_perm = 0;
> > 	struct request *rq;
> > 	char sense[SCSI_SENSE_BUFFERSIZE];
> > 
> > 	Regards
> > 		Oliver
> > -
> where? what? do you mean in scsi_ioctl.c?
> why not it's a synchronous call?
> Do you mean 96 bytes is too big?
> Do you mean DMA alignment and cache coherency? I'm working
> on that for scsi devices.

I'm guessing aligment - so the answer is that 'it cannot get away with
it'.

-- 
Jens Axboe

Re: memory allocation in sg_io()

[Boaz Harrosh]

On Thu, Jan 10 2008 at 15:06 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Thu, Jan 10 2008, Boaz Harrosh wrote:
>> On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
>>> Hi,
>>>
>>> could you explain to me why this code can get away with allocating the
>>> sense buffer on the stack?
>>>
>>> static int sg_io(struct file *file, struct request_queue *q,
>>> 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
>>> {
>>> 	unsigned long start_time;
>>> 	int writing = 0, ret = 0, has_write_perm = 0;
>>> 	struct request *rq;
>>> 	char sense[SCSI_SENSE_BUFFERSIZE];
>>>
>>> 	Regards
>>> 		Oliver
>>> -
>> where? what? do you mean in scsi_ioctl.c?
>> why not it's a synchronous call?
>> Do you mean 96 bytes is too big?
>> Do you mean DMA alignment and cache coherency? I'm working
>> on that for scsi devices.
> 
> I'm guessing aligment - so the answer is that 'it cannot get away with
> it'.
> 
I'm working on something for scsi.
Other block devices I don't know.

Boaz

Re: memory allocation in sg_io()

[Oliver Neukum]

Am Donnerstag, 10. Januar 2008 14:05:25 schrieb Boaz Harrosh:
> On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
> > Hi,
> > 
> > could you explain to me why this code can get away with allocating the
> > sense buffer on the stack?
> > 
> > static int sg_io(struct file *file, struct request_queue *q,
> > 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
> > {
> > 	unsigned long start_time;
> > 	int writing = 0, ret = 0, has_write_perm = 0;
> > 	struct request *rq;
> > 	char sense[SCSI_SENSE_BUFFERSIZE];
> > 
> > 	Regards
> > 		Oliver
> > -
> where? what? do you mean in scsi_ioctl.c?
> why not it's a synchronous call?
> Do you mean 96 bytes is too big?
> Do you mean DMA alignment and cache coherency? I'm working
> on that for scsi devices.

Yes, you are doing DMA on the stack.

	Regards
		Oliver

Re: memory allocation in sg_io()

[Jens Axboe]

On Thu, Jan 10 2008, Oliver Neukum wrote:
> Am Donnerstag, 10. Januar 2008 14:05:25 schrieb Boaz Harrosh:
> > On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
> > > Hi,
> > > 
> > > could you explain to me why this code can get away with allocating the
> > > sense buffer on the stack?
> > > 
> > > static int sg_io(struct file *file, struct request_queue *q,
> > > 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
> > > {
> > > 	unsigned long start_time;
> > > 	int writing = 0, ret = 0, has_write_perm = 0;
> > > 	struct request *rq;
> > > 	char sense[SCSI_SENSE_BUFFERSIZE];
> > > 
> > > 	Regards
> > > 		Oliver
> > > -
> > where? what? do you mean in scsi_ioctl.c?
> > why not it's a synchronous call?
> > Do you mean 96 bytes is too big?
> > Do you mean DMA alignment and cache coherency? I'm working
> > on that for scsi devices.
> 
> Yes, you are doing DMA on the stack.

There are many offenders like that, just take a peek in cdrom.c for
instance.

-- 
Jens Axboe

Re: memory allocation in sg_io()

[Matthew Wilcox]

On Thu, Jan 10, 2008 at 02:19:08PM +0100, Oliver Neukum wrote:
> Am Donnerstag, 10. Januar 2008 14:05:25 schrieb Boaz Harrosh:
> > On Thu, Jan 10 2008 at 14:33 +0200, Oliver Neukum <oliver@neukum.org> wrote:
> > > Hi,
> > > 
> > > could you explain to me why this code can get away with allocating the
> > > sense buffer on the stack?
> > > 
> > > static int sg_io(struct file *file, struct request_queue *q,
> > > 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
> > > {
> > > 	unsigned long start_time;
> > > 	int writing = 0, ret = 0, has_write_perm = 0;
> > > 	struct request *rq;
> > > 	char sense[SCSI_SENSE_BUFFERSIZE];
> 
> Yes, you are doing DMA on the stack.

Nobody does DMA to rq->sense.  ub does a memcpy to it.  cciss does a
memcpy to it.  scsi_lib assigns its own buffer to req->sense, ignoring
the one passed down.

That's how this code gets away with it.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."