[PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Handling of the trailing byte in ata_sff_data_xfer() is suboptimal bacause:

- it always initializes the padding buffer to 0 which is not really needed in
  both the read and write cases;

- it has to use memcpy() to transfer a single byte from/to the padding buffer;

- it uses io{read|write}16() accessors which swap bytes on the big endian CPUs
  and so have to additionally convert the data from/to the little endian format
  instead of using io{read|write}16_rep() accessors which are not supposed to
  change the byte ordering.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
Oops, the subject got truncated -- resending...
The patch is against the recent Linus' tree but should apply to applicable
branch in the libata tree where it should probably wait for the next merge
window...

 drivers/ata/libata-sff.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/ata/libata-sff.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-sff.c
+++ linux-2.6/drivers/ata/libata-sff.c
@@ -723,17 +723,23 @@ unsigned int ata_sff_data_xfer(struct at
 	else
 		iowrite16_rep(data_addr, buf, words);
 
-	/* Transfer trailing 1 byte, if any. */
+	/* Transfer trailing byte, if any. */
 	if (unlikely(buflen & 0x01)) {
-		__le16 align_buf[1] = { 0 };
-		unsigned char *trailing_buf = buf + buflen - 1;
+		unsigned char pad[2];
 
+		/* Point buf to the tail of buffer */
+		buf += buflen - 1;
+
+		/*
+		 * Use io*16_rep() accessors here as well to avoid pointlessly
+		 * swapping bytes to and fro on the big endian machines...
+		 */
 		if (rw == READ) {
-			align_buf[0] = cpu_to_le16(ioread16(data_addr));
-			memcpy(trailing_buf, align_buf, 1);
+			ioread16_rep(data_addr, pad, 1);
+			*buf = pad[0];
 		} else {
-			memcpy(align_buf, trailing_buf, 1);
-			iowrite16(le16_to_cpu(align_buf[0]), data_addr);
+			pad[0] = *buf;
+			iowrite16_rep(data_addr, pad, 1);
 		}
 		words++;
 	}

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Jeff Garzik]

Sergei Shtylyov wrote:
> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal bacause:
> 
> - it always initializes the padding buffer to 0 which is not really needed in
>   both the read and write cases;
> 
> - it has to use memcpy() to transfer a single byte from/to the padding buffer;

Have you looked at the assembly, before deciding it is suboptiomal?

gcc optimizes tiny arrays and structures quite well, and is well capable 
of seeing one path where the initialization is clobbered without a 
single read, and another code path where it is used.

As for memcpy, for small and/or constant values that is quite often a 
compiler builtin.  It is rarely useful, these days, to convert a 
memcpy() to a hand-rolled version of same.

	Jeff

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Jeff Garzik wrote:

>> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal 
>> bacause:

>> - it always initializes the padding buffer to 0 which is not really 
>> needed in
>>   both the read and write cases;

>> - it has to use memcpy() to transfer a single byte from/to the padding 
>> buffer;

> Have you looked at the assembly, before deciding it is suboptiomal?

    I'm estimating the code itself, not what the compiler can do to fix it. :-)

> gcc optimizes tiny arrays and structures quite well, and is well capable 
> of seeing one path where the initialization is clobbered without a 
> single read, and another code path where it is used.

    The initialier just shouldn't have been there in the first place, 
clobbered or not. And let's looks at what gcc gave me:

.L504:
         .loc 1 727 0
         testb   $1, %bl #, buflen
         jne     .L511   #,
[...]
.L511:
.LBB635:
         .loc 1 731 0
         movl    8(%ebp), %eax   # rw,
         .loc 1 729 0
         leal    (%esi,%ebx), %ebx       #, tmp72
.LVL440:
         .loc 1 728 0
.LBB635:
         .loc 1 731 0
         movl    8(%ebp), %eax   # rw,
         .loc 1 729 0
         leal    (%esi,%ebx), %ebx       #, tmp72
.LVL440:
         .loc 1 728 0
         movw    $0, -14(%ebp)   #, align_buf
         .loc 1 731 0
         testl   %eax, %eax      #
         jne     .L507   #,
         .loc 1 732 0
         movl    -20(%ebp), %eax # data_addr, data_addr
         call    ioread16        #
         movw    %ax, -14(%ebp)  # D.29224, align_buf
.LBB629:
.LBB630:
         .loc 4 60 0
         movzbl  -14(%ebp), %eax #, tmp73
         movb    %al, -1(%ebx)   # tmp73,
.L509:
.LBE630:
.LBE629:
         .loc 1 738 0
         addl    $1, %edi        #, words
         jmp     .L505   #
.L507:
.LBB631:
.LBB632:
         .loc 4 60 0
         movzbl  -1(%ebx), %eax  #, tmp74
.LBE632:
.LBE631:
         .loc 1 736 0
         movzwl  -14(%ebp), %eax # align_buf, align_buf
         call    iowrite16       #
         jmp     .L509   #

    As you can see, it happily assigned 0 to align_buf[0] at .LVL440, 
regardless of the value of 'rw'.

> As for memcpy, for small and/or constant values that is quite often a 
> compiler builtin.  It is rarely useful, these days, to convert a memcpy() to a hand-rolled 
version of same.

    Here memcpy() just shouldn't have appeared in the first place. But indeed, 
gcc did optimize it away.

>     Jeff

MBR, Sergei

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Hello, I wrote:

>>> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal 
>>> bacause:

>>> - it always initializes the padding buffer to 0 which is not really 
>>> needed in
>>>   both the read and write cases;

>>> - it has to use memcpy() to transfer a single byte from/to the 
>>> padding buffer;

>> Have you looked at the assembly, before deciding it is suboptiomal?

>    I'm estimating the code itself, not what the compiler can do to fix 
> it. :-)

>> gcc optimizes tiny arrays and structures quite well, and is well 
>> capable of seeing one path where the initialization is clobbered 
>> without a single read, and another code path where it is used.

>    The initialier just shouldn't have been there in the first place, 
> clobbered or not. And let's looks at what gcc gave me:

[...]

>> As for memcpy, for small and/or constant values that is quite often a 
>> compiler builtin.  It is rarely useful, these days, to convert a 
>> memcpy() to a hand-rolled 

> version of same.

>    Here memcpy() just shouldn't have appeared in the first place. But 
> indeed, gcc did optimize it away.

    In fact, we could do without both memcpy and io*15_rep() I think:

  	if (unlikely(buflen & 0x01)) {
		u16 pad;

		/* Point buf to the tail of buffer */
		buf += buflen - 1;

		/*
		 * Copy from/to pad's LSB only (host order),
		 * dropping its MSB or zero-extending it...
		 */
  		if (rw == READ) {
			pad = ioread16(data_addr);
			*buf = (unsigned char)pad;
  		} else {
			pad = *buf;
			iowrite16(pad, data_addr);
		}
	}

    It should work -- that easy... although io{read|write}16() will still 
byte-swap.

>>     Jeff

MBR, Sergei

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Hello, I wrote:

> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal bacause:
>
> - it always initializes the padding buffer to 0 which is not really needed in
>   both the read and write cases;
>
> - it has to use memcpy() to transfer a single byte from/to the padding buffer;
>
> - it uses io{read|write}16() accessors which swap bytes on the big endian CPUs
>   and so have to additionally convert the data from/to the little endian format
>   instead of using io{read|write}16_rep() accessors which are not supposed to
>   change the byte ordering.
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

   Jeff, have you forgotten about this one?

MBR, Sergei

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Hello, I wrote:

>> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal 
>> bacause:

>> - it always initializes the padding buffer to 0 which is not really 
>> needed in
>>   both the read and write cases;

>> - it has to use memcpy() to transfer a single byte from/to the padding 
>> buffer;

>> - it uses io{read|write}16() accessors which swap bytes on the big 
>> endian CPUs
>>   and so have to additionally convert the data from/to the little 
>> endian format
>>   instead of using io{read|write}16_rep() accessors which are not 
>> supposed to
>>   change the byte ordering.
>>
>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>   Jeff, have you forgotten about this one?

    PING.

MBR, Sergei

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Jeff Garzik]

Sergei Shtylyov wrote:
> Hello, I wrote:
> 
>>> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal 
>>> bacause:
> 
>>> - it always initializes the padding buffer to 0 which is not really 
>>> needed in
>>>   both the read and write cases;
> 
>>> - it has to use memcpy() to transfer a single byte from/to the 
>>> padding buffer;
> 
>>> - it uses io{read|write}16() accessors which swap bytes on the big 
>>> endian CPUs
>>>   and so have to additionally convert the data from/to the little 
>>> endian format
>>>   instead of using io{read|write}16_rep() accessors which are not 
>>> supposed to
>>>   change the byte ordering.
>>>
>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>>   Jeff, have you forgotten about this one?
> 
>    PING.

This has been queued to libata-dev.git#upstream, i.e. linux-next queue, 
for a long time.  Apologies if I forgot the 'applied' reply.

Just sent this upstream, as linux-ide should show.

	Jeff

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>> Hello, I wrote:

>>>> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal 
>>>> bacause:

>>>> - it always initializes the padding buffer to 0 which is not really 
>>>> needed in
>>>>   both the read and write cases;

>>>> - it has to use memcpy() to transfer a single byte from/to the 
>>>> padding buffer;

>>>> - it uses io{read|write}16() accessors which swap bytes on the big 
>>>> endian CPUs
>>>>   and so have to additionally convert the data from/to the little 
>>>> endian format
>>>>   instead of using io{read|write}16_rep() accessors which are not 
>>>> supposed to
>>>>   change the byte ordering.
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>>   Jeff, have you forgotten about this one?

>>    PING.

> This has been queued to libata-dev.git#upstream, i.e. linux-next queue, 

    Ah, haven't looked there, so it appeared as forgotten since I know you 
usually reply with 'applied'.

> for a long time.  Apologies if I forgot the 'applied' reply.

> Just sent this upstream, as linux-ide should show.

    Thanks. I began to think of reworking it to avoid even io*_rep() as per 
my followup mail and resending... well, it's good enogh as is. :-)

>     Jeff

MBR, Sergei

Re: [PATCH 2/2 resend] libata-sff: avoid byte swapping in ata_sff_data_xfer()

[Jeff Garzik]

Sergei Shtylyov wrote:
> Handling of the trailing byte in ata_sff_data_xfer() is suboptimal bacause:
> 
> - it always initializes the padding buffer to 0 which is not really needed in
>   both the read and write cases;
> 
> - it has to use memcpy() to transfer a single byte from/to the padding buffer;
> 
> - it uses io{read|write}16() accessors which swap bytes on the big endian CPUs
>   and so have to additionally convert the data from/to the little endian format
>   instead of using io{read|write}16_rep() accessors which are not supposed to
>   change the byte ordering.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied