[PATCH 0/2] libata: implement 32-bit transfers for PIO mode

[PATCH 0/2] libata: implement 32-bit transfers for PIO mode

[Willy Tarreau]

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

[Willy Tarreau]

>From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
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.

Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 drivers/ata/libata-scsi.c |   70 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/libata.h    |    1 +
 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1cea18f..8b4ca45 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..ea9e3fc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */
-- 
1.5.3.8

Re: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls

[Jeff Garzik]

Willy Tarreau wrote:
>>From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <w@1wt.eu>
> 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.
> 
> Signed-off-by: Willy Tarreau <w@1wt.eu>

this patch should be combined into the second patch, because it's not 
much use without both

Re: [PATCH 1/2] libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls

[Willy Tarreau]

On Thu, Apr 17, 2008 at 03:49:04PM -0400, Jeff Garzik wrote:
> Willy Tarreau wrote:
> >>From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon Sep 17 00:00:00 2001
> >From: Willy Tarreau <w@1wt.eu>
> >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.
> >
> >Signed-off-by: Willy Tarreau <w@1wt.eu>
> 
> 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

[PATCH] libata: implement support for 32-bit PIO transfers

[Willy Tarreau]

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


>From 734d62ad5b63108bfc244433aedaf6190543e279 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
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

Signed-off-by: Willy Tarreau <w@1wt.eu>
Acked-by: Alan Cox <alan@redhat.com>
---
 drivers/ata/libata-core.c |   46 ++++++++++++++++++--------
 drivers/ata/libata-scsi.c |   77 +++++++++++++++++++++++++++++++++++++-------
 include/linux/libata.h    |    1 +
 3 files changed, 97 insertions(+), 27 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index be95fdb..22c2643 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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;
 }
 
 /**
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1579539..c0205de 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -448,22 +448,74 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 	return rc;
 }
 
-int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
+/**
+ *	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)
 {
-	int val = -EINVAL, rc = -EINVAL;
+	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)
+{
 	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);
@@ -479,11 +531,10 @@ int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
 		return ata_task_ioctl(scsidev, arg);
 
 	default:
-		rc = -ENOTTY;
-		break;
+		return -ENOTTY;
 	}
 
-	return rc;
+	return -EINVAL;
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 37ee881..1f1a9be 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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 */
-- 
1.5.3.8

[PATCH 2/2] libata: implement support for 32-bit PIO transfers

[Willy Tarreau]

>From 4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
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

Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 drivers/ata/libata-core.c |   46 +++++++++++++++++++++++++++++++-------------
 1 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index beaa3a9..2651130 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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;
 }
 
 /**
-- 
1.5.3.8

Re: [PATCH 2/2] libata: implement support for 32-bit PIO transfers

[Alan Cox]

On Sun, 17 Feb 2008 22:20:04 +0100
Willy Tarreau <w@1wt.eu> wrote:

> 
> >From 4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9 Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <w@1wt.eu>
> 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.

Acked-by: Alan Cox <alan@redhat.com>

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

Re: [PATCH 0/2] libata: implement 32-bit transfers for PIO mode

[Alan Cox]

> 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 0/2] libata: implement 32-bit transfers for PIO mode

[Willy Tarreau]

On Sun, Feb 17, 2008 at 10:31:34PM +0000, Alan Cox wrote:
> > 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).

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 :-/

> Alan

Willy