[PATCH] libata: PIO bounce copies to avoid long IRQ off runs

[PATCH] libata: PIO bounce copies to avoid long IRQ off runs

[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 <alan@redhat.com>

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);
-
-		/* 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);
 	}

Re: [PATCH] libata: PIO bounce copies to avoid long IRQ off runs

[Grant Grundler]

On Tue, Apr 29, 2008 at 6:47 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 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 <alan@redhat.com>
>
>  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);
>  -
>  -               /* 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

>  +               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 majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] libata: PIO bounce copies to avoid long IRQ off runs

[Alan Cox]

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

Re: [PATCH] Re: libata: PIO bounce copies to avoid long IRQ off runs

[Alan Cox]

On Tue, 29 Apr 2008 18:01:28 -0400
Jeff Garzik <jeff@garzik.org> 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.

[PATCH] Re: libata: PIO bounce copies to avoid long IRQ off runs

[Jeff Garzik]

I would tend to prefer this version...

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

Original description:

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)

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/ata/libata-sff.c |   82 ++++++++++++++++++++++++++++++-----------------
 include/linux/libata.h   |    6 +++
 2 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 2ec65a8..7e0651c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -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);
 	}
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d1dfe87..ab43fad 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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