highmem pages

highmem pages

[Jeff Garzik]

Here is an interesting code snippet from ide-scsi, that could be useful 
in libata:

>                 count = min(pc->sg->length - pc->b_count, bcount);
>                 if (PageHighMem(pc->sg->page)) {
>                         unsigned long flags;
> 
>                         local_irq_save(flags);
>                         buf = kmap_atomic(pc->sg->page, KM_IRQ0) + pc->sg->offse
> t;
>                         drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
>  count);
>                         kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
>                         local_irq_restore(flags);
>                 } else {
>                         buf = page_address(pc->sg->page) + pc->sg->offset;
>                         drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
>  count);
>                }

Re: highmem pages

[Albert Lee]

Jeff Garzik wrote:
> 
> Here is an interesting code snippet from ide-scsi, that could be useful 
> in libata:
> 
>>                 count = min(pc->sg->length - pc->b_count, bcount);
>>                 if (PageHighMem(pc->sg->page)) {
>>                         unsigned long flags;
>>
>>                         local_irq_save(flags);
>>                         buf = kmap_atomic(pc->sg->page, KM_IRQ0) + 
>> pc->sg->offse
>> t;
>>                         drive->hwif->atapi_input_bytes(drive, buf + 
>> pc->b_count,
>>  count);
>>                         kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
>>                         local_irq_restore(flags);
>>                 } else {
>>                         buf = page_address(pc->sg->page) + 
>> pc->sg->offset;
>>                         drive->hwif->atapi_input_bytes(drive, buf + 
>> pc->b_count,
>>  count);
>>                }
> 

Thanks for the advice. The kmap_atomic() usage optimization patches are ready:

   patch 1/3: reorganize ata_pio_sector() and __atapi_pio_bytes()
   patch 2/3: reorganize "buf + offset"
   patch 3/3: use PageHighMem() to optimize the kmap_atomic() usage

Patch against libata-dev irq-pio branch (c71c18576d0d8aa4db876c737c3c597c724cf02f).
For your review and advice, thanks.

Albert

[PATCH/RFC 1/3] irq-pio: reorganize ata_pio_sector() and __atapi_pio_bytes()

[Albert Lee]

Patch 1/3:
   reorganize ata_pio_sector() and __atapi_pio_bytes()

Changes:
   - move some code out of the kmap_atomic() / kunmap_atomic() zone
   - remove the redundant "do_write = (qc->tf.flags & ATA_TFLAG_WRITE);"


For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

===========

--- irq-pio/drivers/scsi/libata-core.c	2005-10-06 16:47:05.000000000 +0800
+++ himem1/drivers/scsi/libata-core.c	2005-10-06 15:59:35.000000000 +0800
@@ -2763,22 +2763,21 @@ static void ata_pio_sector(struct ata_qu
  	local_irq_save(flags);
  	buf = kmap_atomic(page, KM_IRQ0) + offset;

-	qc->cursect++;
-	qc->cursg_ofs++;
-
-	if ((qc->cursg_ofs * ATA_SECT_SIZE) == (&sg[qc->cursg])->length) {
-		qc->cursg++;
-		qc->cursg_ofs = 0;
-	}
-
  	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

  	/* do the actual data transfer */
-	do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
  	ata_data_xfer(ap, buf, ATA_SECT_SIZE, do_write);

  	kunmap_atomic(buf - offset, KM_IRQ0);
  	local_irq_restore(flags);
+
+	qc->cursect++;
+	qc->cursg_ofs++;
+
+	if ((qc->cursg_ofs * ATA_SECT_SIZE) == (&sg[qc->cursg])->length) {
+		qc->cursg++;
+		qc->cursg_ofs = 0;
+	}
  }

  /**
@@ -2956,6 +2955,14 @@ next_sg:
  	local_irq_save(flags);
  	buf = kmap_atomic(page, KM_IRQ0) + offset;

+	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
+
+	/* do the actual data transfer */
+	ata_data_xfer(ap, buf, count, do_write);
+
+	kunmap_atomic(buf - offset, KM_IRQ0);
+	local_irq_restore(flags);
+
  	bytes -= count;
  	qc->curbytes += count;
  	qc->cursg_ofs += count;
@@ -2965,14 +2972,6 @@ next_sg:
  		qc->cursg_ofs = 0;
  	}

-	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
-
-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, count, do_write);
-
-	kunmap_atomic(buf - offset, KM_IRQ0);
-	local_irq_restore(flags);
-
  	if (bytes)
  		goto next_sg;
  }

[PATCH/RFC 2/3] irq-pio: reorganize "buf + offset"

[Albert Lee]

Patch 2/3:
     reorganize "buf + offset" in ata_pio_sector() and __atapi_pio_bytes()


Changes:
   - relocate DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
   - buf + offset, buf - offset tidy up


For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

===========

--- himem1/drivers/scsi/libata-core.c	2005-10-06 15:59:35.000000000 +0800
+++ himem2/drivers/scsi/libata-core.c	2005-10-06 16:10:33.000000000 +0800
@@ -2760,15 +2760,15 @@ static void ata_pio_sector(struct ata_qu
  	page = nth_page(page, (offset >> PAGE_SHIFT));
  	offset %= PAGE_SIZE;

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0) + offset;
-
  	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

+	local_irq_save(flags);
+	buf = kmap_atomic(page, KM_IRQ0);
+
  	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, ATA_SECT_SIZE, do_write);
+	ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);

-	kunmap_atomic(buf - offset, KM_IRQ0);
+	kunmap_atomic(buf, KM_IRQ0);
  	local_irq_restore(flags);

  	qc->cursect++;
@@ -2952,15 +2952,15 @@ next_sg:
  	/* don't cross page boundaries */
  	count = min(count, (unsigned int)PAGE_SIZE - offset);

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0) + offset;
-
  	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

+	local_irq_save(flags);
+	buf = kmap_atomic(page, KM_IRQ0);
+
  	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, count, do_write);
+	ata_data_xfer(ap, buf + offset, count, do_write);

-	kunmap_atomic(buf - offset, KM_IRQ0);
+	kunmap_atomic(buf, KM_IRQ0);
  	local_irq_restore(flags);

  	bytes -= count;

[PATCH/RFC 3/3] irq-pio: reorganize "buf + offset"

[Albert Lee]

Patch 3/3:
   use PageHighMem() to optimize the kmap_atomic() usage, as done in ide-scsi.c

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

==========

--- himem2/drivers/scsi/libata-core.c	2005-10-06 16:10:33.000000000 +0800
+++ himem3/drivers/scsi/libata-core.c	2005-10-06 16:08:43.000000000 +0800
@@ -2748,7 +2748,6 @@ static void ata_pio_sector(struct ata_qu
  	struct page *page;
  	unsigned int offset;
  	unsigned char *buf;
-	unsigned long flags;

  	if (qc->cursect == (qc->nsect - 1))
  		ap->hsm_task_state = HSM_ST_LAST;
@@ -2762,14 +2761,21 @@ static void ata_pio_sector(struct ata_qu

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

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0);
+	if (PageHighMem(page)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ0);

-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);
+		/* do the actual data transfer */
+		ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);

-	kunmap_atomic(buf, KM_IRQ0);
-	local_irq_restore(flags);
+		kunmap_atomic(buf, KM_IRQ0);
+		local_irq_restore(flags);
+	} else {
+		buf = page_address(page);
+		ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);
+	}

  	qc->cursect++;
  	qc->cursg_ofs++;
@@ -2908,7 +2914,6 @@ static void __atapi_pio_bytes(struct ata
  	struct page *page;
  	unsigned char *buf;
  	unsigned int offset, count;
-	unsigned long flags;

  	if (qc->curbytes + bytes >= qc->nbytes)
  		ap->hsm_task_state = HSM_ST_LAST;
@@ -2954,14 +2959,21 @@ next_sg:

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

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0);
+	if (PageHighMem(page)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ0);

-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf + offset, count, do_write);
+		/* do the actual data transfer */
+		ata_data_xfer(ap, buf + offset, count, do_write);

-	kunmap_atomic(buf, KM_IRQ0);
-	local_irq_restore(flags);
+		kunmap_atomic(buf, KM_IRQ0);
+		local_irq_restore(flags);
+	} else {
+		buf = page_address(page);
+		ata_data_xfer(ap, buf + offset, count, do_write);
+	}

  	bytes -= count;
  	qc->curbytes += count;

Re: highmem pages

[Jeff Garzik]

Albert Lee wrote:
> Thanks for the advice. The kmap_atomic() usage optimization patches are 
> ready:
> 
>   patch 1/3: reorganize ata_pio_sector() and __atapi_pio_bytes()
>   patch 2/3: reorganize "buf + offset"
>   patch 3/3: use PageHighMem() to optimize the kmap_atomic() usage

Thanks for the patches.  All three patches look OK, but did not apply 
(probably due to mailer problems):

> [jgarzik@pretzel libata-dev]$ patch -sp1 < /g/tmp/mbox 
> 3 out of 3 hunks FAILED -- saving rejects to file drivers/scsi/libata-core.c.rej

FWIW the most reliable email solution is to use a script to generate an 
RFC822 header and email message:

	To: jgarzik@pobox.com (Jeff Garzik)
	CC: linux-ide@vger.kernel.org (Linux IDE mailing list)
	Subject: [patch 1/3] irq-pio: ...
	[blank line, as required by RFC822]
	[patch description]
	[patch]

and then send that to '/usr/bin/sendmail -t < msg.txt' for delivery.

	Jeff

Re: highmem pages

[Albert Lee]

Jeff Garzik wrote:
> Albert Lee wrote:
> 
>> Thanks for the advice. The kmap_atomic() usage optimization patches
>> are ready:
>>
>>   patch 1/3: reorganize ata_pio_sector() and __atapi_pio_bytes()
>>   patch 2/3: reorganize "buf + offset"
>>   patch 3/3: use PageHighMem() to optimize the kmap_atomic() usage
> 
> 
> Thanks for the patches.  All three patches look OK, but did not apply
> (probably due to mailer problems):
> 
>> [jgarzik@pretzel libata-dev]$ patch -sp1 < /g/tmp/mbox 3 out of 3
>> hunks FAILED -- saving rejects to file drivers/scsi/libata-core.c.rej
> 
> 

Sorry for the mangled patch. :(

It seems Thunderbird add an extra space to the beginning of the line.
e.g.
[space][tab]local_irq_save(flags);
was changed to
[space][space][tab]local_irq_save(flags);

I've reconfigured the Thunderbird according to

http://www.gelato.unsw.edu.au/archives/git/0508/7750.html

Will resubmit the patches later.

Albert

[PATCH 0/3] libata-dev: use PageHighMem() to optimize kmap_atomic() usage

[Albert Lee]

(Resend due to white space mangled in the previous patches)

   patch 1/3: reorganize ata_pio_sector() and __atapi_pio_bytes()
   patch 2/3: reorganize "buf + offset"
   patch 3/3: use PageHighMem() to optimize the kmap_atomic() usage

Patch against libata-dev irq-pio branch (c71c18576d0d8aa4db876c737c3c597c724cf02f).
Thanks,

Albert

[PATCH 1/3] irq-pio: reorganize ata_pio_sector() and __atapi_pio_bytes()

[Albert Lee]

Patch 1/3:
   reorganize ata_pio_sector() and __atapi_pio_bytes()

Changes:
   - move some code out of the kmap_atomic() / kunmap_atomic() zone
   - remove the redundant "do_write = (qc->tf.flags & ATA_TFLAG_WRITE);"


Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
===========

--- irq-pio/drivers/scsi/libata-core.c	2005-10-06 16:47:05.000000000 +0800
+++ himem1/drivers/scsi/libata-core.c	2005-10-06 15:59:35.000000000 +0800
@@ -2763,22 +2763,21 @@ static void ata_pio_sector(struct ata_qu
 	local_irq_save(flags);
 	buf = kmap_atomic(page, KM_IRQ0) + offset;

-	qc->cursect++;
-	qc->cursg_ofs++;
-
-	if ((qc->cursg_ofs * ATA_SECT_SIZE) == (&sg[qc->cursg])->length) {
-		qc->cursg++;
-		qc->cursg_ofs = 0;
-	}
-
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

 	/* do the actual data transfer */
-	do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	ata_data_xfer(ap, buf, ATA_SECT_SIZE, do_write);

 	kunmap_atomic(buf - offset, KM_IRQ0);
 	local_irq_restore(flags);
+
+	qc->cursect++;
+	qc->cursg_ofs++;
+
+	if ((qc->cursg_ofs * ATA_SECT_SIZE) == (&sg[qc->cursg])->length) {
+		qc->cursg++;
+		qc->cursg_ofs = 0;
+	}
 }

 /**
@@ -2956,6 +2955,14 @@ next_sg:
 	local_irq_save(flags);
 	buf = kmap_atomic(page, KM_IRQ0) + offset;

+	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
+
+	/* do the actual data transfer */
+	ata_data_xfer(ap, buf, count, do_write);
+
+	kunmap_atomic(buf - offset, KM_IRQ0);
+	local_irq_restore(flags);
+
 	bytes -= count;
 	qc->curbytes += count;
 	qc->cursg_ofs += count;
@@ -2965,14 +2972,6 @@ next_sg:
 		qc->cursg_ofs = 0;
 	}

-	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
-
-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, count, do_write);
-
-	kunmap_atomic(buf - offset, KM_IRQ0);
-	local_irq_restore(flags);
-
 	if (bytes)
 		goto next_sg;
 }

[PATCH 2/3] irq-pio: reorganize "buf + offset"

[Albert Lee]

(Resend)
Patch 2/3:
     reorganize "buf + offset" in ata_pio_sector() and __atapi_pio_bytes()


Changes:
   - relocate DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
   - buf + offset, buf - offset tidy up


Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

===========

--- himem1/drivers/scsi/libata-core.c	2005-10-06 15:59:35.000000000 +0800
+++ himem2/drivers/scsi/libata-core.c	2005-10-06 16:10:33.000000000 +0800
@@ -2760,15 +2760,15 @@ static void ata_pio_sector(struct ata_qu
 	page = nth_page(page, (offset >> PAGE_SHIFT));
 	offset %= PAGE_SIZE;

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0) + offset;
-
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

+	local_irq_save(flags);
+	buf = kmap_atomic(page, KM_IRQ0);
+
 	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, ATA_SECT_SIZE, do_write);
+	ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);

-	kunmap_atomic(buf - offset, KM_IRQ0);
+	kunmap_atomic(buf, KM_IRQ0);
 	local_irq_restore(flags);

 	qc->cursect++;
@@ -2952,15 +2952,15 @@ next_sg:
 	/* don't cross page boundaries */
 	count = min(count, (unsigned int)PAGE_SIZE - offset);

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0) + offset;
-
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");

+	local_irq_save(flags);
+	buf = kmap_atomic(page, KM_IRQ0);
+
 	/* do the actual data transfer */
-	ata_data_xfer(ap, buf, count, do_write);
+	ata_data_xfer(ap, buf + offset, count, do_write);

-	kunmap_atomic(buf - offset, KM_IRQ0);
+	kunmap_atomic(buf, KM_IRQ0);
 	local_irq_restore(flags);

 	bytes -= count;

[PATCH 3/3] irq-pio: use PageHighMem() to optimize the kmap_atomic() usage

[Albert Lee]

(resend)
Patch 3/3:
   use PageHighMem() to optimize the kmap_atomic() usage, as done in ide-scsi.c

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

==========

--- himem2/drivers/scsi/libata-core.c	2005-10-06 16:10:33.000000000 +0800
+++ himem3/drivers/scsi/libata-core.c	2005-10-06 16:08:43.000000000 +0800
@@ -2748,7 +2748,6 @@ static void ata_pio_sector(struct ata_qu
 	struct page *page;
 	unsigned int offset;
 	unsigned char *buf;
-	unsigned long flags;

 	if (qc->cursect == (qc->nsect - 1))
 		ap->hsm_task_state = HSM_ST_LAST;
@@ -2762,14 +2761,21 @@ static void ata_pio_sector(struct ata_qu

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

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0);
+	if (PageHighMem(page)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ0);

-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);
+		/* do the actual data transfer */
+		ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);

-	kunmap_atomic(buf, KM_IRQ0);
-	local_irq_restore(flags);
+		kunmap_atomic(buf, KM_IRQ0);
+		local_irq_restore(flags);
+	} else {
+		buf = page_address(page);
+		ata_data_xfer(ap, buf + offset, ATA_SECT_SIZE, do_write);
+	}

 	qc->cursect++;
 	qc->cursg_ofs++;
@@ -2908,7 +2914,6 @@ static void __atapi_pio_bytes(struct ata
 	struct page *page;
 	unsigned char *buf;
 	unsigned int offset, count;
-	unsigned long flags;

 	if (qc->curbytes + bytes >= qc->nbytes)
 		ap->hsm_task_state = HSM_ST_LAST;
@@ -2954,14 +2959,21 @@ next_sg:

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

-	local_irq_save(flags);
-	buf = kmap_atomic(page, KM_IRQ0);
+	if (PageHighMem(page)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		buf = kmap_atomic(page, KM_IRQ0);

-	/* do the actual data transfer */
-	ata_data_xfer(ap, buf + offset, count, do_write);
+		/* do the actual data transfer */
+		ata_data_xfer(ap, buf + offset, count, do_write);

-	kunmap_atomic(buf, KM_IRQ0);
-	local_irq_restore(flags);
+		kunmap_atomic(buf, KM_IRQ0);
+		local_irq_restore(flags);
+	} else {
+		buf = page_address(page);
+		ata_data_xfer(ap, buf + offset, count, do_write);
+	}

 	bytes -= count;
 	qc->curbytes += count;

Re: [PATCH 3/3] irq-pio: use PageHighMem() to optimize the kmap_atomic() usage

[Jeff Garzik]

applied patches 1-3, after updating the irq-pio branch to the latest 
upstream branch.