libata: implement 32-bit transfers for PIO mode
cover libata: implement 32-bit transfers for PIO mode
Hi Jeff,
For several times I tried libata on small machines equipped with either CompactFlash or IDE DOM (Disk-On-Module). All those machines with small flashes (<= 256 MB) were about 35-40% slower under libata than under the plain old IDE driver. I realized that all the slower ones were running PIO only.
Today I had time to investigate the issue on a Geode LX board. At first I thought it was the pata_cs5536 driver which would have incorrect timings, but this was not the case. I finally noticed that under IDE, my flashes were running in 32-bit mode while I could not enable 32-bit with libata.
Reading ata_data_xfer() made it obvious that transfers were only 16-bit wide.
Thus, I have implemented the 32-bit mode to bring the performance back to the level of the old IDE driver. I jumped from 1.5 MB/s to 2.5 MB/s, which is an important difference at this level of performance, especially when large files are read. The 32-bit mode is enabled using the ioctl which is already implemented but only accepts a null value.
I'm joining two patches which I hope you'll consider for inclusion. I've updated them to latest Linus' git.
Best regards,
Willy
PATCH 1/2 libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls
Quoted 2 lines
From
40a8174d27cb9d93b859bc073c8f075b9ff71578Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sun, 17 Feb 2008 21:11:46 +0100
Subject: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls
This patch implements the aforementionned ioctls and get/set the new ATA_DFLAG_32BIT_PIO ata flag accordingly for the device. It only supports values 0 and 1 (disable/enable), as I have no plan to implement the sync VLB transfers.
--- drivers/ata/libata-scsi.c | 70 +++++++++++++++++++++++++++++++++++++++----- include/linux/libata.h | 1 + 2 files changed, 63 insertions(+), 8 deletions(-)
drivers/ata/libata-scsi.c +62 -8
@@ -448,22 +448,76 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)return rc; } +/** +* ata_get_io32 - Handler for ATA_IOC_GET_IO32 ioctl +* @sdev: SCSI device to get identify data for +* @arg: User buffer area for return value +* +* LOCKING: +* Defined by the SCSI layer. We don't really care. +* +* RETURNS: +* Zero on success, negative errno on error. +*/ +static int ata_get_io32 (struct scsi_device *sdev,void __user *arg) +{ +struct ata_port *ap =ata_shost_to_port (sdev->host); +struct ata_device *dev =ata_scsi_find_dev (ap, sdev); +int val; + +if (!dev) +return -ENOMSG ; + + val = (dev->flags &ATA_DFLAG_32BIT_PIO ) !=0 ; +if (copy_to_user (arg, &val,1 )) +return -EFAULT ; + +return 0 ; +} + +/** +* ata_set_io32 - Handler for ATA_IOC_SET_IO32 ioctl +* @sdev: SCSI device to get identify data for +* @arg: boolean integer value passed by user +* +* LOCKING: +* Defined by the SCSI layer. We don't really care. +* +* RETURNS: +* Zero on success, negative errno on error. +*/ +static int ata_set_io32 (struct scsi_device *sdev,void __user *arg) +{ +struct ata_port *ap =ata_shost_to_port (sdev->host); +struct ata_device *dev =ata_scsi_find_dev (ap, sdev); + +if (!dev) +return -ENOMSG ; + +switch ((unsigned long ) arg) { +case 0 : + dev->flags &= ~ATA_DFLAG_32BIT_PIO ; +return 0 ; +case 1 : + dev->flags |=ATA_DFLAG_32BIT_PIO ; +return 0 ; +default : +break ; + } + +return -EFAULT ; +} +int ata_scsi_ioctl (struct scsi_device *scsidev,int cmd,void __user *arg) {int val = -EINVAL , rc = -EINVAL ;switch (cmd) {case ATA_IOC_GET_IO32 : - val =0 ; -if (copy_to_user (arg, &val,1 )) -return -EFAULT ; -return 0 ; +return ata_get_io32 (scsidev, arg);case ATA_IOC_SET_IO32 : - val = (unsigned long ) arg; -if (val !=0 ) -return -EINVAL ; -return 0 ; +return ata_set_io32 (scsidev, arg);case HDIO_GET_IDENTITY :return ata_get_identity (scsidev, arg);
include/linux/libata.h +1 -0
@@ -138,6 +138,7 @@ enum {ATA_DFLAG_AN = (1 <<7 ),/* AN configured */ ATA_DFLAG_HIPM = (1 <<8 ),/* device supports HIPM */ ATA_DFLAG_DIPM = (1 <<9 ),/* device supports DIPM */ +ATA_DFLAG_32BIT_PIO = (1 <<10 ),/* device supports 32-bit in PIO mode */ ATA_DFLAG_CFG_MASK = (1 <<12 ) -1 , ATA_DFLAG_PIO = (1 << 12), /* device limited to PIO mode */
Signature · 1 line
1.5.3.8
PATCH 2/2 libata: implement support for 32-bit PIO transfers
In reply to Willy TarreauQuoted 2 lines
From
4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sun, 17 Feb 2008 21:28:25 +0100
Subject: [PATCH 2/2] libata: implement support for 32-bit PIO transfers
When ATA_DFLAG_32BIT_PIO is set in ata flags, PIO transfers will be performed in 32-bit, just like with the plain old IDE drivers.
16-bit transfers in libata are as slow as 16-bit transfers in plain old IDE drivers, which are about 35% slower than 32-bit transfers with various CF and DOM. This patch restores the performance to pre-libata level, which is a nice improvement for many small systems relying on such hardware.
This patch relies on previous patch which implements the ATA_DFLAG_32BIT_PIO ioctl.
To enable 32-bit transfers, simply proceed like before with IDE :
# hdparm -c 1 /dev/sda
--- drivers/ata/libata-core.c | 46 +++++++++++++++++++++++++++++++------------- 1 files changed, 32 insertions(+), 14 deletions(-)
drivers/ata/libata-core.c +32 -14
@@ -4936,7 +4936,7 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) }/** -* ata_data_xfer - Transfer data by PIO +* ata_data_xfer - Transfer data by PIO (16 or 32-bit) * @dev: device to target * @buf: data buffer * @buflen: buffer length
@@ -4955,30 +4955,48 @@ unsigned int ata_data_xfer(struct ata_device *dev, unsigned char *buf, {struct ata_port *ap = dev->link->ap;void __iomem *data_addr = ap->ioaddr.data_addr; -unsigned int words = buflen >>1 ; +unsigned int words; -/* Transfer multiple of 2 bytes */ -if (rw ==READ ) -ioread16_rep (data_addr, buf, words); -else -iowrite16_rep (data_addr, buf, words); +if (dev->flags &ATA_DFLAG_32BIT_PIO ) { +/* Transfer in 32-bit words */ + words = buflen >>2 ; +if (rw ==READ ) +ioread32_rep (data_addr, buf, words); +else +iowrite32_rep (data_addr, buf, words); + words <<=2 ; + }else { +/* Transfer in 16-bit words */ + words = buflen >>1 ; +if (rw ==READ ) +ioread16_rep (data_addr, buf, words); +else +iowrite16_rep (data_addr, buf, words); + words <<=1 ; + } -/* Transfer trailing 1 byte, if any. */ -if (unlikely (buflen &0x01 )) { +/* Transfer trailing 1, 2 or 3 bytes, if any. We know how many bytes +* remain because <words> now contains the number of bytes transferred. +*/ +while (buflen > words) {__le16 align_buf[1 ] = {0 }; -unsigned char *trailing_buf = buf + buflen -1 ; +unsigned char *trailing_buf = buf + words; +int len = buflen - words; + +if (len >2 ) + len =2 ;if (rw ==READ ) { align_buf[0 ] =cpu_to_le16 (ioread16 (data_addr)); -memcpy (trailing_buf, align_buf,1 ); +memcpy (trailing_buf, align_buf, len); }else { -memcpy (align_buf, trailing_buf,1 ); +memcpy (align_buf, trailing_buf, len);iowrite16 (le16_to_cpu (align_buf[0 ]), data_addr); } - words++; + words += len; } -return words <<1 ; +return words; }/**
Signature · 1 line
1.5.3.8
Quoted 5 lines
Thus, I have implemented the 32-bit mode to bring the performance back to the level of the old IDE driver. I jumped from 1.5 MB/s to 2.5 MB/s, which is an important difference at this level of performance, especially when large files are read. The 32-bit mode is enabled using the ioctl which is already implemented but only accepts a null value.
Excellent, that has been on my TODO list for some time and I'd only gotten as far as putting into the ISA/VLB drivers rather than generally testing.
I'm not however sure this should be a DFLAG but should be an alernative ata_data_xfer method - I say that because VLB needs to wrap it and some controllers have quirky rules for 32bit xfers. (Also some small number of pre ATA disks can't handle the different timing cycles from a 32bit ISA I/O being redirected their way).
Alan
Re: [PATCH 2/2] libata: implement support for 32-bit PIO transfers
In reply to Willy TarreauQuoted 9 lines · On Sun, 17 Feb 2008 22:20:04 +0100 Willy Tarreau <[email protected]> wrote:
Quoted 2 lines
From
4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9Mon Sep 17 00:00:00 2001From: Willy Tarreau <[email protected]>
Date: Sun, 17 Feb 2008 21:28:25 +0100
Subject: [PATCH 2/2] libata: implement support for 32-bit PIO transfersWhen
ATA_DFLAG_32BIT_PIOis set in ata flags, PIO transfers will be performed in 32-bit, just like with the plain old IDE drivers.
Although I think ata_data_xfer_32() would be safer and easier to add to controllers as we check the docs. But that can be a follow up improvement.
Alan
Quoted 10 lines · Alan Cox
Quoted 5 lines
Thus, I have implemented the 32-bit mode to bring the performance back to the level of the old IDE driver. I jumped from 1.5 MB/s to 2.5 MB/s, which is an important difference at this level of performance, especially when large files are read. The 32-bit mode is enabled using the ioctl which is already implemented but only accepts a null value.
Excellent, that has been on my TODO list for some time and I'd only gotten as far as putting into the ISA/VLB drivers rather than generally testing.
Quoted 5 lines
I'm not however sure this should be a DFLAG but should be an alernative
ata_data_xfermethod - I say that because VLB needs to wrap it and some controllers have quirky rules for 32bit xfers. (Also some small number of pre ATA disks can't handle the different timing cycles from a 32bit ISA I/O being redirected their way).
Do you think this can cause any trouble considering that default setting is not changed ? However, I agree that an alternative ata_data_xfer may make it easier to always enable it on some controllers. Or maybe we should keep it that way (since this function checks the ioctl value) and add a pure 32-bit function for 32-bit enabled controllers ? I would say I have no idea, it's clearly not my domain of expertise :-/
Quoted 1 line
Alan
Willy
Re: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls
In reply to Jeff GarzikQuoted 12 lines · Jeff Garzik
Quoted 11 lines · Willy Tarreau
Quoted 1 line
From
40a8174d27cb9d93b859bc073c8f075b9ff71578Mon Sep 17 00:00:00 2001From: Willy Tarreau <[email protected]>
Date: Sun, 17 Feb 2008 21:11:46 +0100
Subject: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctlsThis patch implements the aforementionned ioctls and get/set the new
ATA_DFLAG_32BIT_PIOata flag accordingly for the device. It only supports values 0 and 1 (disable/enable), as I have no plan to implement the sync VLB transfers.
Quoted 4 lines
Quoted 1 line
Signed-off-by Willy Tarreau <[email protected]>this patch should be combined into the second patch, because it's not much use without both
OK will do and resend. I will also add Alan's Acked line.
Regards,
Willy
Re: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls
In reply to Willy TarreauQuoted 11 lines · Willy Tarreau
Quoted 1 line
From
40a8174d27cb9d93b859bc073c8f075b9ff71578Mon Sep 17 00:00:00 2001From: Willy Tarreau <[email protected]>
Date: Sun, 17 Feb 2008 21:11:46 +0100
Subject: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctlsThis patch implements the aforementionned ioctls and get/set the new
ATA_DFLAG_32BIT_PIOata flag accordingly for the device. It only supports values 0 and 1 (disable/enable), as I have no plan to implement the sync VLB transfers.Signed-off-by Willy Tarreau <[email protected]>
this patch should be combined into the second patch, because it's not much use without both
PATCH 1/1 libata: implement support for 32-bit PIO transfers
Jeff,
here's an update of the patch. Rebuilt and re-tested against 2.6.25. I've added Alan's previous Acked-by too btw.
Regards,
Willy
Quoted 1 line
From
734d62ad5b63108bfc244433aedaf6190543e279Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Thu, 17 Apr 2008 23:59:41 +0200
Subject: libata: implement support for 32-bit PIO transfers
This patch implements ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls and gets/sets the new ATA_DFLAG_32BIT_PIO ata flag accordingly for the device. It only supports values 0 and 1 (disable/enable), as I have no plan to implement the sync VLB transfers.
When ATA_DFLAG_32BIT_PIO is set in ata flags, PIO transfers will be performed in 32-bit, just like with the plain old IDE drivers.
16-bit transfers in libata are as slow as 16-bit transfers in plain old IDE drivers, which are about 35% slower than 32-bit transfers with various CF and DOM. This patch restores the performance to pre-libata level, which is a nice improvement for many small systems relying on such hardware. On an ALIX board equipped with a 64 MB CF card, transfer speed went up from 1.49 MB/sec to 2.41 MB/s, which represents a net gain of 61%.
To enable 32-bit transfers, simply proceed like before with IDE :
# hdparm -c 1 /dev/sda
--- drivers/ata/libata-core.c | 46 ++++++++++++++++++-------- drivers/ata/libata-scsi.c | 77 +++++++++++++++++++++++++++++++++++++------- include/linux/libata.h | 1 + 3 files changed, 97 insertions(+), 27 deletions(-)
drivers/ata/libata-core.c +32 -14
@@ -5036,7 +5036,7 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) }/** -* ata_data_xfer - Transfer data by PIO +* ata_data_xfer - Transfer data by PIO (16 or 32-bit) * @dev: device to target * @buf: data buffer * @buflen: buffer length
@@ -5055,30 +5055,48 @@ unsigned int ata_data_xfer(struct ata_device *dev, unsigned char *buf, {struct ata_port *ap = dev->link->ap;void __iomem *data_addr = ap->ioaddr.data_addr; -unsigned int words = buflen >>1 ; +unsigned int words; -/* Transfer multiple of 2 bytes */ -if (rw ==READ ) -ioread16_rep (data_addr, buf, words); -else -iowrite16_rep (data_addr, buf, words); +if (dev->flags &ATA_DFLAG_32BIT_PIO ) { +/* Transfer in 32-bit words */ + words = buflen >>2 ; +if (rw ==READ ) +ioread32_rep (data_addr, buf, words); +else +iowrite32_rep (data_addr, buf, words); + words <<=2 ; + }else { +/* Transfer in 16-bit words */ + words = buflen >>1 ; +if (rw ==READ ) +ioread16_rep (data_addr, buf, words); +else +iowrite16_rep (data_addr, buf, words); + words <<=1 ; + } -/* Transfer trailing 1 byte, if any. */ -if (unlikely (buflen &0x01 )) { +/* Transfer trailing 1, 2 or 3 bytes, if any. We know how many bytes +* remain because <words> now contains the number of bytes transferred. +*/ +while (buflen > words) {__le16 align_buf[1 ] = {0 }; -unsigned char *trailing_buf = buf + buflen -1 ; +unsigned char *trailing_buf = buf + words; +int len = buflen - words; + +if (len >2 ) + len =2 ;if (rw ==READ ) { align_buf[0 ] =cpu_to_le16 (ioread16 (data_addr)); -memcpy (trailing_buf, align_buf,1 ); +memcpy (trailing_buf, align_buf, len); }else { -memcpy (align_buf, trailing_buf,1 ); +memcpy (align_buf, trailing_buf, len);iowrite16 (le16_to_cpu (align_buf[0 ]), data_addr); } - words++; + words += len; } -return words <<1 ; +return words; }/**
drivers/ata/libata-scsi.c +64 -13
97 lines not on this page · raw message
include/linux/libata.h +1 -0
@@ -139,6 +139,7 @@ enum {ATA_DFLAG_HIPM = (1 <<8 ),/* device supports HIPM */ ATA_DFLAG_DIPM = (1 <<9 ),/* device supports DIPM */ ATA_DFLAG_DMADIR = (1 <<10 ),/* device requires DMADIR */ +ATA_DFLAG_32BIT_PIO = (1 <<11 ),/* device supports 32-bit in PIO mode */ ATA_DFLAG_CFG_MASK = (1 <<12 ) -1 , ATA_DFLAG_PIO = (1 << 12), /* device limited to PIO mode */
Signature · 1 line
1.5.3.8