RE: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

RE: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Timothy Thelin]

> On Tue, Nov 01 2005, Timothy Thelin wrote:
> > Changes:
> > Increases the taskfile data transfer limit from 128K to a
> > theoretical 2-4MB (arch dependant) by using __get_free_pages
> > instead of kmalloc.  Note that larger requests have a lower
> > success rate because of needing to find a larger amount of
> > free consecutive memory.
> 
> I don't think this is a good way to do it. IDE already supports sg for
> regular fs requests, the correct solution would be to make 
> sure the user
> driven taskfile submission is sg based as well. That would 
> enable large
> transfers without risking allocation failures.

Agreed. I looked into doing sg first, but the changes
required would be more invasive and have a higher risk of
breaking existing things. This was targeted as an interim
solution by someone without lots of ide core experience
until someone came up with a proper sg solution.

> An interface that doesn't
> work for anything but a freshly booted kernel is pretty 
> worthless, imho.
> User interfaces must work predictably.
> 

A question though: is __get_free_pages less successful than
kmalloc in returning the same sized chunk of memory?  If
it's the same success rate (and I thought it was, is it not?),
then up to 128K the interface acts just like before, but now it
can possibly do larger transfers rather than having no
ability to do larger transfers.

My personal interests are in transfering data in the
130K-140K range, just over what kmalloc can do, and 
this interim solution has been great for that.

It might be possible for me to begin work on an sg solution
in a couple of weeks (design advice anyone?).


Regards,
Tim Thelin

Re: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Jens Axboe]

On Tue, Nov 01 2005, Timothy Thelin wrote:
> 
> > On Tue, Nov 01 2005, Timothy Thelin wrote:
> > > Changes:
> > > Increases the taskfile data transfer limit from 128K to a
> > > theoretical 2-4MB (arch dependant) by using __get_free_pages
> > > instead of kmalloc.  Note that larger requests have a lower
> > > success rate because of needing to find a larger amount of
> > > free consecutive memory.
> > 
> > I don't think this is a good way to do it. IDE already supports sg for
> > regular fs requests, the correct solution would be to make 
> > sure the user
> > driven taskfile submission is sg based as well. That would 
> > enable large
> > transfers without risking allocation failures.
> 
> Agreed. I looked into doing sg first, but the changes
> required would be more invasive and have a higher risk of
> breaking existing things. This was targeted as an interim
> solution by someone without lots of ide core experience
> until someone came up with a proper sg solution.

That's understandable.

> > An interface that doesn't
> > work for anything but a freshly booted kernel is pretty 
> > worthless, imho.
> > User interfaces must work predictably.
> > 
> 
> A question though: is __get_free_pages less successful than
> kmalloc in returning the same sized chunk of memory?  If
> it's the same success rate (and I thought it was, is it not?),

Hmm I'm not sure. kmalloc() will go to the size-128k slab pool for the
memory, while __get_free_pages() will just try and find 128k/page_size
consecutive free pages in memory. So it's different paths. You can try
it though, if you can think of a good way to fragment your memory :-)

> then up to 128K the interface acts just like before, but now it
> can possibly do larger transfers rather than having no
> ability to do larger transfers.
> 
> My personal interests are in transfering data in the
> 130K-140K range, just over what kmalloc can do, and 
> this interim solution has been great for that.
> 
> It might be possible for me to begin work on an sg solution
> in a couple of weeks (design advice anyone?).

Since the general fs io path already deals with sg requests all the
time, you should be able to take advantage of that. Basically it's a
taskfile requests with a fully bio mapped request. Some nice examples of
that can be found in drivers/block/scsi_ioctl.c, you want to be using
blk_rq_map_user() for setting up the request data parts. Since the API
didn't have any alignment restrictions before (it always copied data,
blk_rq_map_user() will set up your request for zero-copy dma), you
probably also have to deal with bouncing the memory to a kernel
allocated buffer (which can be sg as well, so just single page
allocations are needed).

-- 
Jens Axboe

Re: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Martin Murray]

    I looked into this yesterday, and it appears that sglists are
hardcoded to a maximum of 256 sectors or (256*512) 128kb. Specifically, 
ide_build_sglist() in ide-dma.c the lines are as follows:

    if ((rq->flags & REQ_DRIVE_TASKFILE) && rq->nr_sectors > 256)
        BUG();

    Other than that, as far as I could tell, modifying Mr. Thelin's
patch to use SG would be straightforward. ide_map_sg() in ide-io.c would
have to be modified to allocate the scatter gather list, and the
ide_taskfile_ioctl() in ide-taskfile.c would have to be modified to copy
to userspace the scatter/gather list instead of the buffer that is 
allocated. I'll happily try this after class unless its incorrect.

    One thing I don't understand is, if Mr. Thelin's code that requests,
as said, a little over 128k is a DMA command, then shouldn't the kernel
have bugged? And if it didn't bug, was the data transferred via PIO? Or
does the chipset handle the PIO to the drive and dma the result?

Regards, Martin

* Jens Axboe <axboe@suse.de> [051102 03:48]:
> On Tue, Nov 01 2005, Timothy Thelin wrote:
> > 
> > > On Tue, Nov 01 2005, Timothy Thelin wrote:
> > > > Changes:
> > > > Increases the taskfile data transfer limit from 128K to a
> > > > theoretical 2-4MB (arch dependant) by using __get_free_pages
> > > > instead of kmalloc.  Note that larger requests have a lower
> > > > success rate because of needing to find a larger amount of
> > > > free consecutive memory.
> > > 
> > > I don't think this is a good way to do it. IDE already supports sg for
> > > regular fs requests, the correct solution would be to make 
> > > sure the user
> > > driven taskfile submission is sg based as well. That would 
> > > enable large
> > > transfers without risking allocation failures.
> > 
> > Agreed. I looked into doing sg first, but the changes
> > required would be more invasive and have a higher risk of
> > breaking existing things. This was targeted as an interim
> > solution by someone without lots of ide core experience
> > until someone came up with a proper sg solution.
> 
> That's understandable.
> 
> > > An interface that doesn't
> > > work for anything but a freshly booted kernel is pretty 
> > > worthless, imho.
> > > User interfaces must work predictably.
> > > 
> > 
> > A question though: is __get_free_pages less successful than
> > kmalloc in returning the same sized chunk of memory?  If
> > it's the same success rate (and I thought it was, is it not?),
> 
> Hmm I'm not sure. kmalloc() will go to the size-128k slab pool for the
> memory, while __get_free_pages() will just try and find 128k/page_size
> consecutive free pages in memory. So it's different paths. You can try
> it though, if you can think of a good way to fragment your memory :-)
> 
> > then up to 128K the interface acts just like before, but now it
> > can possibly do larger transfers rather than having no
> > ability to do larger transfers.
> > 
> > My personal interests are in transfering data in the
> > 130K-140K range, just over what kmalloc can do, and 
> > this interim solution has been great for that.
> > 
> > It might be possible for me to begin work on an sg solution
> > in a couple of weeks (design advice anyone?).
> 
> Since the general fs io path already deals with sg requests all the
> time, you should be able to take advantage of that. Basically it's a
> taskfile requests with a fully bio mapped request. Some nice examples of
> that can be found in drivers/block/scsi_ioctl.c, you want to be using
> blk_rq_map_user() for setting up the request data parts. Since the API
> didn't have any alignment restrictions before (it always copied data,
> blk_rq_map_user() will set up your request for zero-copy dma), you
> probably also have to deal with bouncing the memory to a kernel
> allocated buffer (which can be sg as well, so just single page
> allocations are needed).
> 
> -- 
> Jens Axboe
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Martin Murray, <mmurray@deepthought.org>, :\\//\\//:
- What is Industrial Music? It's the sound of angry
  Belgians having a fight with a washing machine.
Fear: Seeing B8:00:4C:CD:21, and knowing what it means.

Re: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Bartlomiej Zolnierkiewicz]

Hi,

On 11/2/05, Martin Murray <mmurray@deepthought.org> wrote:
>
>     I looked into this yesterday, and it appears that sglists are
> hardcoded to a maximum of 256 sectors or (256*512) 128kb. Specifically,
> ide_build_sglist() in ide-dma.c the lines are as follows:
>
>     if ((rq->flags & REQ_DRIVE_TASKFILE) && rq->nr_sectors > 256)
>         BUG();

Yep.

>     Other than that, as far as I could tell, modifying Mr. Thelin's
> patch to use SG would be straightforward. ide_map_sg() in ide-io.c would
> have to be modified to allocate the scatter gather list, and the
> ide_taskfile_ioctl() in ide-taskfile.c would have to be modified to copy
> to userspace the scatter/gather list instead of the buffer that is

Won't this break existing users of HDIO_DRIVE_TASKFILE ioctl?

Long-term I want to add SG_IO ioctl + "ATA pass-through" support
to IDE driver which will be based on libata code.

A lot of work has already been done by Tejun Heo and me:

* unification of ioctl code-paths
* conversion to use struct ata_taskfile
* conversion to use taskfiles for normal I/O

Unfortunately this work stalled because of lack of
time so patches need refreshing for current kernels.

Next step should be addition of host state machine to IDE driver.

This will be basically enough to start adding "ATA pass-through"
support to IDE driver and deprecating HDIO_DRIVE_* ioctls...

> allocated. I'll happily try this after class unless its incorrect.
>
>     One thing I don't understand is, if Mr. Thelin's code that requests,
> as said, a little over 128k is a DMA command, then shouldn't the kernel
> have bugged? And if it didn't bug, was the data transferred via PIO? Or

I suppose so.

> does the chipset handle the PIO to the drive and dma the result?

This is Virtual DMA mode and is independent
of HDIO_DRIVE_TASKFILE ioctl.

Thanks,
Bartlomiej

Re: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Martin Murray]

Just a few quick questions:

regarding HDIO_DRIVE_TASKFILE:
    I don't see any exact information, however, from what I can infer
from the code, it appears that while the user can specify an
input buffer and/or an output buffer, only one of the buffers would be
filled with useful information. Is it reasonable to assume that
req_task->out_size == 0 req_task->in_size == 0? Or, more specifically,
is it reasonable to return -EINVAL if both are non zero?

    If I were to do zero_copy IO using blk_rq_map_user, should I verify
the user buffer is valid with access_ok() ?

    Why does ide_diag_taskfile() allocate a custom request structure
instead of using blk_get_request(drive->queue, ...) ?

    Given HDIO_DRIVE_TASKFILE, what is the purpose of HDIO_DRIVE_TASK
and HDIO_DRIVE_CMD?

Thank you, Martin

RE: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Timothy Thelin]

Hi,

> 
>  <snip>
> >
> >     One thing I don't understand is, if Mr. Thelin's code 
> that requests,
> > as said, a little over 128k is a DMA command, then 
> shouldn't the kernel
> > have bugged? And if it didn't bug, was the data transferred 
> via PIO? Or
> 
> I suppose so.
> 

Yes, the transfers were all PIO

Regards,
Tim Thelin

RE: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Timothy Thelin]

Hi again,

I had a brain malfunction and gave the wrong email
address on the signed-off line, sorry about that.

Here is the correct one:
Signed-off-by: Timothy Thelin <timothy.thelin@wdc.com>

Regards,
Tim Thelin

> -----Original Message-----
> From: linux-ide-owner@vger.kernel.org
> [mailto:linux-ide-owner@vger.kernel.org]On Behalf Of Timothy Thelin
> Sent: Tuesday, November 01, 2005 11:06 AM
> To: Bartlomiej Zolnierkiewicz; linux-ide@vger.kernel.org
> Subject: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile 
> transfers
> 
> 
> Hi all,
> 
> Here is for review the cleaned up version of the patch
> that allows ide taskfile transfers to be larger than 128K.
> 
> I've validated on my x86 that its possible to do 4MB
> read sectors extended and write sectors extended, smaller
> transfers with smart read log and smart write log, and
> some non-data commands.
> 
> It's attached as well, since what's below is going to get
> line wrapped.
> 
> Thanks,
> Tim Thelin
> 
> Changes:
> Increases the taskfile data transfer limit from 128K to a
> theoretical 2-4MB (arch dependant) by using __get_free_pages
> instead of kmalloc.  Note that larger requests have a lower
> success rate because of needing to find a larger amount of
> free consecutive memory.
> 
> Signed-off-by: Timothy Thelin <tthelin@wdc.com>
> 
> ============
> 
> diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
> --- a/drivers/ide/ide-taskfile.c
> +++ b/drivers/ide/ide-taskfile.c
> @@ -511,6 +511,27 @@ int ide_raw_taskfile (ide_drive_t *drive
>  
>  EXPORT_SYMBOL(ide_raw_taskfile);
>  
> +static int ide_xfrsize_to_pageorder(int size)
> +{
> +	int pageorder, x, num;
> +
> +	num = size / PAGE_SIZE;
> +	if (size % PAGE_SIZE)
> +		++num;
> +
> +	pageorder = 0;
> +	for (x = sizeof(num) * 8 - 1; x >= 0; --x) {
> +		if ((num >> x) & 1) {
> +			if (pageorder) {
> +				++pageorder;
> +				break;
> +			} else
> +				pageorder = x;
> +		}
> +	}
> +	return pageorder;
> +}
> +
>  int ide_taskfile_ioctl (ide_drive_t *drive, unsigned int 
> cmd, unsigned long
> arg)
>  {
>  	ide_task_request_t	*req_task;
> @@ -523,6 +544,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
>  	int tasksize		= sizeof(struct ide_task_request_s);
>  	int taskin		= 0;
>  	int taskout		= 0;
> +	int inorder		= 0;
> +	int outorder    	= 0;
>  	u8 io_32bit		= drive->io_32bit;
>  	char __user *buf = (char __user *)arg;
>  
> @@ -541,7 +564,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
>  
>  	if (taskout) {
>  		int outtotal = tasksize;
> -		outbuf = kmalloc(taskout, GFP_KERNEL);
> +		outorder = ide_xfrsize_to_pageorder(taskout);
> +		outbuf = (u8*)__get_free_pages(GFP_KERNEL, outorder);
>  		if (outbuf == NULL) {
>  			err = -ENOMEM;
>  			goto abort;
> @@ -555,7 +579,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
>  
>  	if (taskin) {
>  		int intotal = tasksize + taskout;
> -		inbuf = kmalloc(taskin, GFP_KERNEL);
> +		inorder = ide_xfrsize_to_pageorder(taskin);
> +		inbuf = (u8*)__get_free_pages(GFP_KERNEL, inorder);
>  		if (inbuf == NULL) {
>  			err = -ENOMEM;
>  			goto abort;
> @@ -650,9 +675,9 @@ int ide_taskfile_ioctl (ide_drive_t *dri
>  abort:
>  	kfree(req_task);
>  	if (outbuf != NULL)
> -		kfree(outbuf);
> +		free_pages((unsigned long)outbuf, outorder);
>  	if (inbuf != NULL)
> -		kfree(inbuf);
> +		free_pages((unsigned long)inbuf, inorder);
>  
>  //	printk("IDE Taskfile ioctl ended. rc = %i\n", err);
>  
> 

[UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Timothy Thelin]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]

Hi all,

Here is for review the cleaned up version of the patch
that allows ide taskfile transfers to be larger than 128K.

I've validated on my x86 that its possible to do 4MB
read sectors extended and write sectors extended, smaller
transfers with smart read log and smart write log, and
some non-data commands.

It's attached as well, since what's below is going to get
line wrapped.

Thanks,
Tim Thelin

Changes:
Increases the taskfile data transfer limit from 128K to a
theoretical 2-4MB (arch dependant) by using __get_free_pages
instead of kmalloc.  Note that larger requests have a lower
success rate because of needing to find a larger amount of
free consecutive memory.

Signed-off-by: Timothy Thelin <tthelin@wdc.com>

============

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -511,6 +511,27 @@ int ide_raw_taskfile (ide_drive_t *drive
 
 EXPORT_SYMBOL(ide_raw_taskfile);
 
+static int ide_xfrsize_to_pageorder(int size)
+{
+	int pageorder, x, num;
+
+	num = size / PAGE_SIZE;
+	if (size % PAGE_SIZE)
+		++num;
+
+	pageorder = 0;
+	for (x = sizeof(num) * 8 - 1; x >= 0; --x) {
+		if ((num >> x) & 1) {
+			if (pageorder) {
+				++pageorder;
+				break;
+			} else
+				pageorder = x;
+		}
+	}
+	return pageorder;
+}
+
 int ide_taskfile_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long
arg)
 {
 	ide_task_request_t	*req_task;
@@ -523,6 +544,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 	int tasksize		= sizeof(struct ide_task_request_s);
 	int taskin		= 0;
 	int taskout		= 0;
+	int inorder		= 0;
+	int outorder    	= 0;
 	u8 io_32bit		= drive->io_32bit;
 	char __user *buf = (char __user *)arg;
 
@@ -541,7 +564,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 
 	if (taskout) {
 		int outtotal = tasksize;
-		outbuf = kmalloc(taskout, GFP_KERNEL);
+		outorder = ide_xfrsize_to_pageorder(taskout);
+		outbuf = (u8*)__get_free_pages(GFP_KERNEL, outorder);
 		if (outbuf == NULL) {
 			err = -ENOMEM;
 			goto abort;
@@ -555,7 +579,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 
 	if (taskin) {
 		int intotal = tasksize + taskout;
-		inbuf = kmalloc(taskin, GFP_KERNEL);
+		inorder = ide_xfrsize_to_pageorder(taskin);
+		inbuf = (u8*)__get_free_pages(GFP_KERNEL, inorder);
 		if (inbuf == NULL) {
 			err = -ENOMEM;
 			goto abort;
@@ -650,9 +675,9 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 abort:
 	kfree(req_task);
 	if (outbuf != NULL)
-		kfree(outbuf);
+		free_pages((unsigned long)outbuf, outorder);
 	if (inbuf != NULL)
-		kfree(inbuf);
+		free_pages((unsigned long)inbuf, inorder);
 
 //	printk("IDE Taskfile ioctl ended. rc = %i\n", err);
 

[-- Attachment #2: ide-enable-larger-taskfile-transfers.patch --]
[-- Type: application/octet-stream, Size: 1923 bytes --]

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -511,6 +511,27 @@ int ide_raw_taskfile (ide_drive_t *drive
 
 EXPORT_SYMBOL(ide_raw_taskfile);
 
+static int ide_xfrsize_to_pageorder(int size)
+{
+	int pageorder, x, num;
+
+	num = size / PAGE_SIZE;
+	if (size % PAGE_SIZE)
+		++num;
+
+	pageorder = 0;
+	for (x = sizeof(num) * 8 - 1; x >= 0; --x) {
+		if ((num >> x) & 1) {
+			if (pageorder) {
+				++pageorder;
+				break;
+			} else
+				pageorder = x;
+		}
+	}
+	return pageorder;
+}
+
 int ide_taskfile_ioctl (ide_drive_t *drive, unsigned int cmd, unsigned long arg)
 {
 	ide_task_request_t	*req_task;
@@ -523,6 +544,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 	int tasksize		= sizeof(struct ide_task_request_s);
 	int taskin		= 0;
 	int taskout		= 0;
+	int inorder		= 0;
+	int outorder    	= 0;
 	u8 io_32bit		= drive->io_32bit;
 	char __user *buf = (char __user *)arg;
 
@@ -541,7 +564,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 
 	if (taskout) {
 		int outtotal = tasksize;
-		outbuf = kmalloc(taskout, GFP_KERNEL);
+		outorder = ide_xfrsize_to_pageorder(taskout);
+		outbuf = (u8*)__get_free_pages(GFP_KERNEL, outorder);
 		if (outbuf == NULL) {
 			err = -ENOMEM;
 			goto abort;
@@ -555,7 +579,8 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 
 	if (taskin) {
 		int intotal = tasksize + taskout;
-		inbuf = kmalloc(taskin, GFP_KERNEL);
+		inorder = ide_xfrsize_to_pageorder(taskin);
+		inbuf = (u8*)__get_free_pages(GFP_KERNEL, inorder);
 		if (inbuf == NULL) {
 			err = -ENOMEM;
 			goto abort;
@@ -650,9 +675,9 @@ int ide_taskfile_ioctl (ide_drive_t *dri
 abort:
 	kfree(req_task);
 	if (outbuf != NULL)
-		kfree(outbuf);
+		free_pages((unsigned long)outbuf, outorder);
 	if (inbuf != NULL)
-		kfree(inbuf);
+		free_pages((unsigned long)inbuf, inorder);
 
 //	printk("IDE Taskfile ioctl ended. rc = %i\n", err);
 

Re: [UPDATED][PATCH 2.6.14]: ide: Enable larger taskfile transfers

[Jens Axboe]

On Tue, Nov 01 2005, Timothy Thelin wrote:
> Changes:
> Increases the taskfile data transfer limit from 128K to a
> theoretical 2-4MB (arch dependant) by using __get_free_pages
> instead of kmalloc.  Note that larger requests have a lower
> success rate because of needing to find a larger amount of
> free consecutive memory.

I don't think this is a good way to do it. IDE already supports sg for
regular fs requests, the correct solution would be to make sure the user
driven taskfile submission is sg based as well. That would enable large
transfers without risking allocation failures. An interface that doesn't
work for anything but a freshly booted kernel is pretty worthless, imho.
User interfaces must work predictably.

-- 
Jens Axboe