libata: PIO bounce copies to avoid long IRQ off runs

PATCH In review Alan Cox 3 participants 5 messages Apr 29, 2008 libata

Share & tools

Alan Cox

author

2008-04-29 13:55 UTC #0 lore raw

PATCH 1/1 libata: PIO bounce copies to avoid long IRQ off runs

Right now with PIO transfers we can jam the box up horribly as lock IRQs off when the transfer is from high memory. We still have other things we need to fix in this area to really make a difference but this is the buffering side of the fix (the state machine enable/disable irq stuff is also needed)

Currently uses kmalloc - which seems to work best, but trivial to allocate at init time if wanted

Signed-off-by Alan Cox <[email protected]>

drivers/ata/libata-sff.c +63 ~~-27~~

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-mm1/drivers/ata/libata-sff.c linux-2.6.25-mm1/drivers/ata/libata-sff.c
@@ -665,6 +665,62 @@
 }
 
 /**
+ *	ata_bounce_pio_xfer	-	Transfer a block by PIO from high
+ *	@dev: target device
+ *	@page: highmem page
+ *	@offset: offset in page
+ *	@count: bytes to transfer
+ *	@do_write: writing if set, reading if not
+ *
+ *	Transfer a page of high memory via PIO. Whenever possible use a bounce
+ *	buffer to avoid transfers occuring with local IRQ disable
+ */
+
+static unsigned int ata_bounce_pio_xfer(struct ata_device *dev, struct page *page, 
+				  unsigned int offset, int count, int rw)
+{
+	struct ata_port *ap = dev->link->ap;
+	unsigned long flags;
+	unsigned char *zebedee;
+	unsigned char *buf;
+	unsigned int consumed;
+
+	BUG_ON(offset + count > PAGE_SIZE);
+
+	zebedee = kmalloc(count, GFP_ATOMIC);
+	if (likely(zebedee)) {
+		if (rw == WRITE) {
+			local_irq_save(flags);
+			buf = kmap_atomic(page, KM_IRQ0);
+			memcpy(zebedee, buf + offset, count);
+			kunmap_atomic(buf, KM_IRQ0);
+			local_irq_restore(flags);
+		}
+		/* do the actual data transfer */
+		consumed = ap->ops->sff_data_xfer(dev, zebedee, count, rw);
+		if (rw == READ) {
+			/* Read so bounce  data upwards */
+			local_irq_save(flags);
+			buf = kmap_atomic(page, KM_IRQ0);
+			memcpy(buf + offset, zebedee, count);
+			kunmap_atomic(buf, KM_IRQ0);
+			local_irq_restore(flags);
+		}
+		kfree(zebedee);
+	} else {
+		/* Only used when we are out of buffer memory
+		   as a last last resort */
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ0);
+		/* do the actual data transfer */
+		consumed = ap->ops->sff_data_xfer(dev, buf + offset, count, rw);
+		kunmap_atomic(buf, KM_IRQ0);
+		local_irq_restore(flags);
+	}
+	return consumed;
+}
+
+/**
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
  *

@@ -675,7 +731,7 @@
  */
 static void ata_pio_sector(struct ata_queued_cmd *qc)
 {
-	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
+	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
 	struct ata_port *ap = qc->ap;
 	struct page *page;
 	unsigned int offset;

@@ -693,19 +749,9 @@
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
-	if (PageHighMem(page)) {
-		unsigned long flags;
-
-		/* FIXME: use a bounce buffer */
-		local_irq_save(flags);
-		buf = kmap_atomic(page, KM_IRQ0);
-
-		/* do the actual data transfer */
-		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
-				       do_write);
-
-		kunmap_atomic(buf, KM_IRQ0);
-		local_irq_restore(flags);
+	if (PageHighMem(page) || 1 /* TEST FIXME */) {
+		ata_bounce_pio_xfer(qc->dev, page, offset, qc->sect_size,
+								do_write);
 	} else {
 		buf = page_address(page);
 		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,

@@ -830,19 +876,9 @@
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
-	if (PageHighMem(page)) {
-		unsigned long flags;
-
-		/* FIXME: use bounce buffer */
-		local_irq_save(flags);
-		buf = kmap_atomic(page, KM_IRQ0);
-
-		/* do the actual data transfer */
-		consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
-
-		kunmap_atomic(buf, KM_IRQ0);
-		local_irq_restore(flags);
-	} else {
+	if (PageHighMem(page))
+		consumed = ata_bounce_pio_xfer(qc->dev, page, offset, count, rw);
+	else {
 		buf = page_address(page);
 		consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
 	}

Grant Grundler

2008-04-29 15:51 UTC #1 lore raw

Quoted 96 lines · Alan Cox

Right now with PIO transfers we can jam the box up horribly as lock IRQs
off when the transfer is from high memory. We still have other things we need to fix in this area to really make a difference but this is the buffering side of the fix (the state machine enable/disable irq stuff is also needed)

Currently uses kmalloc - which seems to work best, but trivial to allocate at init time if wanted

Signed-off-by Alan Cox <[email protected]>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-mm1/drivers/ata/libata-sff.c linux-2.6.25-mm1/drivers/ata/libata-sff.c
--- linux.vanilla-2.6.25-mm1/drivers/ata/libata-sff.c 2008-04-28 11:36:48.000000000 +0100
+++ linux-2.6.25-mm1/drivers/ata/libata-sff.c 2008-04-28 11:42:05.000000000 +0100
@@ -665,6 +665,62 @@
}

/**

 + *     ata_bounce_pio_xfer     -       Transfer a block by PIO from high
 + *     @dev: target device
 + *     @page: highmem page
 + *     @offset: offset in page
 + *     @count: bytes to transfer
 + *     @do_write: writing if set, reading if not
 + *
 + *     Transfer a page of high memory via PIO. Whenever possible use a bounce
 + *     buffer to avoid transfers occuring with local IRQ disable
 + */
 +
 +static unsigned int ata_bounce_pio_xfer(struct ata_device *dev, struct page *page,
 +                                 unsigned int offset, int count, int rw)
 +{
 +       struct ata_port *ap = dev->link->ap;
 +       unsigned long flags;
 +       unsigned char *zebedee;
 +       unsigned char *buf;
 +       unsigned int consumed;
 +
 +       BUG_ON(offset + count > PAGE_SIZE);
 +
 +       zebedee = kmalloc(count, GFP_ATOMIC);
 +       if (likely(zebedee)) {
 +               if (rw == WRITE) {
 +                       local_irq_save(flags);
 +                       buf = kmap_atomic(page, KM_IRQ0);
 +                       memcpy(zebedee, buf + offset, count);
 +                       kunmap_atomic(buf, KM_IRQ0);
 +                       local_irq_restore(flags);
 +               }
 +               /* do the actual data transfer */
 +               consumed = ap->ops->sff_data_xfer(dev, zebedee, count, rw);
 +               if (rw == READ) {
 +                       /* Read so bounce  data upwards */
 +                       local_irq_save(flags);
 +                       buf = kmap_atomic(page, KM_IRQ0);
 +                       memcpy(buf + offset, zebedee, count);
 +                       kunmap_atomic(buf, KM_IRQ0);
 +                       local_irq_restore(flags);
 +               }
 +               kfree(zebedee);
 +       } else {
 +               /* Only used when we are out of buffer memory
 +                  as a last last resort */
 +               local_irq_save(flags);
 +               buf = kmap_atomic(page, KM_IRQ0);
 +               /* do the actual data transfer */
 +               consumed = ap->ops->sff_data_xfer(dev, buf + offset, count, rw);
 +               kunmap_atomic(buf, KM_IRQ0);
 +               local_irq_restore(flags);
 +       }
 +       return consumed;
 +}
 +
 +/**
  *     ata_pio_sector - Transfer a sector of data.
  *     @qc: Command on going
  *
 @@ -675,7 +731,7 @@
  */
  static void ata_pio_sector(struct ata_queued_cmd *qc)
  {
 -       int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 +       int do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
        struct ata_port *ap = qc->ap;
        struct page *page;
        unsigned int offset;

@@ -693,19 +749,9 @@

        DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

- if (PageHighMem(page)) {
- unsigned long flags;
-

 -               /* FIXME: use a bounce buffer */
 -               local_irq_save(flags);
 -               buf = kmap_atomic(page, KM_IRQ0);

Quoted 8 lines

-
- /* do the actual data transfer */
- ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
- do_write);
-
- kunmap_atomic(buf, KM_IRQ0);
- local_irq_restore(flags);
+ if (PageHighMem(page) || 1 /* TEST FIXME */) {

Did you want the "|| 1" code committed as well?

thanks,
grant

Quoted 33 lines

+ ata_bounce_pio_xfer(qc->dev, page, offset, qc->sect_size,

 +                                                               do_write);
        } else {
                buf = page_address(page);
                ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,

@@ -830,19 +876,9 @@

        DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

- if (PageHighMem(page)) {
- unsigned long flags;
-

 -               /* FIXME: use bounce buffer */
 -               local_irq_save(flags);
 -               buf = kmap_atomic(page, KM_IRQ0);
 -
 -               /* do the actual data transfer */
 -               consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
 -
 -               kunmap_atomic(buf, KM_IRQ0);
 -               local_irq_restore(flags);
 -       } else {
 +       if (PageHighMem(page))
 +               consumed = ata_bounce_pio_xfer(qc->dev, page, offset, count, rw);
 +       else {
                buf = page_address(page);
                consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
        }

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alan Cox

author

2008-04-29 16:29 UTC #2 lore raw

Quoted 5 lines

Quoted 3 lines
- kunmap_atomic(buf, KM_IRQ0);
- local_irq_restore(flags);
+ if (PageHighMem(page) || 1 /* TEST FIXME */) {
Did you want the "|| 1" code committed as well?

Well it was more for discussion but no I didn't. Well spotted 8)

Alan

Jeff Garzik

2008-04-29 22:01 UTC #3 lore raw

In reply to Alan Cox

I would tend to prefer this version...

- allocate buffer at init time
- use lesser-used KM_IRQ1

Original description:

Signed-off-by Jeff Garzik <[email protected]>

---
 drivers/ata/libata-sff.c |   82 ++++++++++++++++++++++++++++++-----------------
 include/linux/libata.h   |    6 +++
 2 files changed, 60 insertions(+), 28 deletions(-)

drivers/ata/libata-sff.c +54 ~~-28~~

index 2ec65a8..7e0651c 100644
@@ -665,6 +665,52 @@ unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
 }
 
 /**
+ *	ata_bounce_pio_xfer	-	Transfer a block by PIO from high
+ *	@dev: target device
+ *	@page: highmem page
+ *	@offset: offset in page
+ *	@count: bytes to transfer
+ *	@do_write: writing if set, reading if not
+ *
+ *	Transfer a page of high memory via PIO. Whenever possible use a bounce
+ *	buffer to avoid transfers occuring with local IRQ disable
+ */
+
+static unsigned int ata_bounce_pio_xfer(struct ata_device *dev,
+					struct page *page, unsigned int offset,
+					int count, int rw)
+{
+	struct ata_port *ap = dev->link->ap;
+	unsigned long flags;
+	unsigned char *buf;
+	unsigned int consumed;
+
+	BUG_ON(offset + count > PAGE_SIZE);
+
+	if (rw == WRITE) {
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ1);
+		memcpy(dev->pio_bounce, buf + offset, count);
+		kunmap_atomic(buf, KM_IRQ1);
+		local_irq_restore(flags);
+	}
+
+	/* do the actual data transfer */
+	consumed = ap->ops->sff_data_xfer(dev, dev->pio_bounce, count, rw);
+
+	if (rw == READ) {
+		/* Read so bounce  data upwards */
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ1);
+		memcpy(buf + offset, dev->pio_bounce, count);
+		kunmap_atomic(buf, KM_IRQ1);
+		local_irq_restore(flags);
+	}
+
+	return consumed;
+}
+
+/**
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
  *

@@ -675,7 +721,7 @@ unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
  */
 static void ata_pio_sector(struct ata_queued_cmd *qc)
 {
-	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
+	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
 	struct ata_port *ap = qc->ap;
 	struct page *page;
 	unsigned int offset;

@@ -693,20 +739,10 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
-	if (PageHighMem(page)) {
-		unsigned long flags;
-
-		/* FIXME: use a bounce buffer */
-		local_irq_save(flags);
-		buf = kmap_atomic(page, KM_IRQ0);
-
-		/* do the actual data transfer */
-		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
-				       do_write);
-
-		kunmap_atomic(buf, KM_IRQ0);
-		local_irq_restore(flags);
-	} else {
+	if (PageHighMem(page))
+		ata_bounce_pio_xfer(qc->dev, page, offset, qc->sect_size,
+								do_write);
+	else {
 		buf = page_address(page);
 		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
 				       do_write);

@@ -830,19 +866,9 @@ next_sg:
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
-	if (PageHighMem(page)) {
-		unsigned long flags;
-
-		/* FIXME: use bounce buffer */
-		local_irq_save(flags);
-		buf = kmap_atomic(page, KM_IRQ0);
-
-		/* do the actual data transfer */
-		consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
-
-		kunmap_atomic(buf, KM_IRQ0);
-		local_irq_restore(flags);
-	} else {
+	if (PageHighMem(page))
+		consumed = ata_bounce_pio_xfer(qc->dev, page, offset, count, rw);
+	else {
 		buf = page_address(page);
 		consumed = ap->ops->sff_data_xfer(dev,  buf + offset, count, rw);
 	}

include/linux/libata.h +6 -0

index d1dfe87..ab43fad 100644
@@ -576,6 +576,12 @@ struct ata_device {
 		u16		id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
 		u32		gscr[SATA_PMP_GSCR_DWORDS]; /* PMP GSCR block */
 	};
+
+	/* TODO: if bounce buffer is guaranteed never to be used on a
+	 * system, elide it at compile time.  It is only needed to
+	 * bounce PageHighMem() pages.
+	 */
+	u8			pio_bounce[PAGE_SIZE];
 };
 
 /* Offset into struct ata_device.  Fields above it are maintained

Alan Cox

author

2008-04-29 22:08 UTC #4 lore raw

Quoted 4 lines · On Tue, 29 Apr 2008 18:01:28 -0400 Jeff Garzik <[email protected]> wrote:

I would tend to prefer this version...
- allocate buffer at init time
- use lesser-used KM_IRQ1

Seems reasonable enough. You actually want to deal with the buffer at run time however because you care about two cases

1. Box with no highmem - easy to do compile time
2. Box with no high pages allocated - kind of hairy with gcc as
the crystal ball feature doesn't work.