Skip to contentSkip to conversation

libata: implement 32-bit transfers for PIO mode

PATCH In review Willy Tarreau 3 participants 9 messages Feb 17, 2008 – Apr 17, 2008 libata

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw

PATCH 1/2 libata: implement ATA_IOC_GET_IO32/ATA_IOC_SET_IO32 ioctls

Quoted 2 lines

From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon 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
index 1cea18f..8b4ca45 100644
@@ -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
index bc5a8d0..ea9e3fc 100644
@@ -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
#2 lore raw

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

Quoted 11 lines · Willy Tarreau
Quoted 1 line

From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon 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.

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

#3 lore raw

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

Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Willy Tarreau
Quoted 1 line

From 40a8174d27cb9d93b859bc073c8f075b9ff71578 Mon 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.

Quoted 4 lines
Quoted 1 line

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

#4 lore raw

PATCH 1/1 libata: implement support for 32-bit PIO transfers

In reply to Jeff Garzik

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 734d62ad5b63108bfc244433aedaf6190543e279 Mon 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
index be95fdb..22c2643 100644
@@ -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
index 37ee881..1f1a9be 100644
@@ -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
#5 lore raw

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

In reply to Willy Tarreau
Quoted 2 lines

From 4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9 Mon 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
index beaa3a9..2651130 100644
@@ -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
Acked-by on v1 2/2
#6 lore raw

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

Quoted 9 lines · On Sun, 17 Feb 2008 22:20:04 +0100 Willy Tarreau <[email protected]> wrote:
Quoted 2 lines

From 4ea313fe6a3c46a90226cf40d0e3ece4b36b48f9 Mon 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.

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

#7 lore raw
In reply to Willy Tarreau
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

#8 lore raw
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_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 :-/

Quoted 1 line

Alan

Willy