Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Montag 12 November 2007 08:54:52, you (Adrian Bunk) wrote: > On Sat, Nov 10, 2007 at 11:04:41PM +0100, Jiri Slaby wrote: > > Why? Anyway I think this is the case. The body of the then branch is > > executed at > > most once, while the else branch each time but last. If you write/read 1002 > > bytes, it means 250:1. ...and it's invoked from interrupt too... > > AFAIK there is no well defined semantics whether likely/unlikely means > 10:1 or 1:1 that is guaranteed to not change during the next > 10 years. > > Unless there's a measurable difference, it's best to write readable > C code and simply leave all optimizations to the compiler. >From my (totally) beginners point of view i would have guessed a chance of very well below one percent that this condition is true could be called "unlikely", but i have to admit this is most probably a much too naive way of thinking, especially in regards of compiler optimizations. And in this special case now there are already most calls (about 80 percent) caught by the switch-case-shortcuts anyway. So, i'll revert it. Thanks a lot for your feedback, Frank - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Sat, Nov 10, 2007 at 11:04:41PM +0100, Jiri Slaby wrote:
> On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> > On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
> >> ...
> >> --- a/drivers/char/nozomi.c
> >> +++ b/drivers/char/nozomi.c
> >> ...
> >> - if (size_bytes - i == 2) {
> >> + if (unlikely(size_bytes - i == 2)) {
> >> ...
> >
> > Please don't add likely/unlikely in drivers unless it brings a
> > measurable improvement.
>
> Why? Anyway I think this is the case. The body of the then branch is executed
> at
> most once, while the else branch each time but last. If you write/read 1002
> bytes, it means 250:1. ...and it's invoked from interrupt too...
AFAIK there is no well defined semantics whether likely/unlikely means
10:1 or 1:1 that is guaranteed to not change during the next
10 years.
Unless there's a measurable difference, it's best to write readable
C code and simply leave all optimizations to the compiler.
> regards,
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Sonntag 11 November 2007 03:37:28, you (Frank Seidel) wrote:
> While in the read_mem32 the unlikekly really seems to be of no use at all (the
> switch-case ahead seems to be hit nearly always), the unlikely in the
> write_mem32 seems to be fine.
> I compared after each 30 seconds and got median ratio of 1381:1 (for the
> likely path) after about 20 minutes, i see a range between 1046:1 and
> 3511:1. So i wouldn't call it a bad guess from my beginners point of view.
I even did some more tracking which pathes get used in there with what
size_bytes values for write_mem32. To what i could see i get about
50% of the calls with size_bytes == 4 (what gets handled in the
switch-case shortcut), about 30% of the calls with size_bytes == 1 (
so i also added a shortcut for this which was just one line) and the
remaining calls (not handled in the switch-case ahead) still reach
a ratio of about 800:1 for the 4-byte-case to the (unlikely) 2-byte-
case.
So i did a rework of that patch which is nearly as nice as Jiris,
but works here without problems and has the size_bytes 1 shortcut,
plus the "unlikely" for the remaining 2-bytes path.
I know the format of the patch isn't fully correct, but i'll integrate
it into the complete patch und polish it there before posting that
again.
Thanks a lot,
Frank
---
Index: linux/drivers/char/nozomi.c
===
--- linux.orig/drivers/char/nozomi.c
+++ linux/drivers/char/nozomi.c
@@ -104,6 +104,7 @@
#include
#include
#include
+#include
#include
@@ -265,7 +266,7 @@ enum port_type {
PORT_ERROR = -1,
};
-#ifdef __ARMEB__
+#ifdef __BIG_ENDIAN
/* Big endian */
struct toggles {
@@ -547,11 +548,7 @@ static void read_mem32(u32 *buf, const v
u32 size_bytes)
{
u32 i = 0;
-#ifdef __ARMEB__
- const u32 *ptr = (u32 *) mem_addr_start;
-#else
const u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
u16 *buf16;
if (unlikely(!ptr || !buf))
@@ -561,19 +558,11 @@ static void read_mem32(u32 *buf, const v
switch (size_bytes) {
case 2: /* 2 bytes */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- *buf16 = __le16_to_cpu(readw(ptr));
-#else
- *buf16 = readw((void __iomem *)ptr);
-#endif
+ *buf16 = __le16_to_cpu(readw((void __iomem *)ptr));
goto out;
break;
case 4: /* 4 bytes */
-#ifdef __ARMEB__
- *(buf) = __le32_to_cpu(readl(ptr));
-#else
- *(buf) = readl((void __iomem *)ptr);
-#endif
+ *(buf) = __le32_to_cpu(readl((void __iomem *)ptr));
goto out;
break;
}
@@ -582,19 +571,11 @@ static void read_mem32(u32 *buf, const v
if (size_bytes - i == 2) {
/* Handle 2 bytes in the end */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- *(buf16) = __le16_to_cpu(readw(ptr));
-#else
- *(buf16) = readw((void __iomem *)ptr);
-#endif
+ *(buf16) = __le16_to_cpu(readw((void __iomem *)ptr));
i += 2;
} else {
/* Read 4 bytes */
-#ifdef __ARMEB__
- *(buf) = __le32_to_cpu(readl(ptr));
-#else
- *(buf) = readl((void __iomem *)ptr);
-#endif
+ *(buf) = __le32_to_cpu(readl((void __iomem *)ptr));
i += 4;
}
buf++;
@@ -613,11 +594,7 @@ static u32 write_mem32(void __iomem *mem
u32 size_bytes)
{
u32 i = 0;
-#ifdef __ARMEB__
- u32 *ptr = (u32 *) mem_addr_start;
-#else
u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
u16 *buf16;
if (unlikely(!ptr || !buf))
@@ -627,40 +604,28 @@ static u32 write_mem32(void __iomem *mem
switch (size_bytes) {
case 2: /* 2 bytes */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- writew(__le16_to_cpu(*buf16), ptr);
-#else
- writew(*buf16, (void __iomem *)ptr);
-#endif
+ writew(__cpu_to_le16(*buf16), (void __iomem *)ptr);
return 2;
break;
+ case 1:
+ /* also need to write 4 bytes in this case
+* so falling through..
+*/
case 4: /* 4 bytes */
-#ifdef __ARMEB__
- writel(__cpu_to_le32(*buf), ptr);
-#else
- writel(*buf, (void __iomem *)ptr);
-#endif
+ writel(__cpu_to_le32(*buf), (void __iomem *)ptr);
return 4;
break;
}
while (i < size_bytes) {
- if (size_bytes - i == 2) {
+ if (unlikely(size_bytes - i == 2)) {
/* 2 bytes */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- wri
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Samstag 10 November 2007 23:04:41, you (Jiri Slaby) wrote:
> On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> > On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
> >> ...
> >> - if (size_bytes - i == 2) {
> >> + if (unlikely(size_bytes - i == 2)) {
> >> ...
> >
> > Please don't add likely/unlikely in drivers unless it brings a
> > measurable improvement.
> Why? Anyway I think this is the case. The body of the then branch is executed
> at
> most once, while the else branch each time but last. If you write/read 1002
> bytes, it means 250:1. ...and it's invoked from interrupt too...
I just did some measurements of how often (under real life scenarios like
downloading big files, websurfing, chat and ssh sessions) those pathes are
used.
While in the read_mem32 the unlikekly really seems to be of no use at all (the
switch-case ahead seems to be hit nearly always), the unlikely in the
write_mem32 seems to be fine.
I compared after each 30 seconds and got median ratio of 1381:1 (for the
likely path) after about 20 minutes, i see a range between 1046:1 and
3511:1. So i wouldn't call it a bad guess from my beginners point of view.
Thanks,
Frank
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
>> ...
>> --- a/drivers/char/nozomi.c
>> +++ b/drivers/char/nozomi.c
>> ...
>> -if (size_bytes - i == 2) {
>> +if (unlikely(size_bytes - i == 2)) {
>> ...
>
> Please don't add likely/unlikely in drivers unless it brings a
> measurable improvement.
Why? Anyway I think this is the case. The body of the then branch is executed at
most once, while the else branch each time but last. If you write/read 1002
bytes, it means 250:1. ...and it's invoked from interrupt too...
regards,
--
Jiri Slaby ([EMAIL PROTECTED])
Faculty of Informatics, Masaryk University
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
>...
> --- a/drivers/char/nozomi.c
> +++ b/drivers/char/nozomi.c
>...
> - if (size_bytes - i == 2) {
> + if (unlikely(size_bytes - i == 2)) {
>...
Please don't add likely/unlikely in drivers unless it brings a
measurable improvement.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 13/13] Char: nozomi, cleanup read and write
On Samstag 10 November 2007 00:51:33, you (Jiri Slaby) wrote: > nozomi, cleanup read and write > > - remove macros testing for endianness, le*_to_cpu and complements will do > it > - pointers cleanup (no need to have different pointers for be and le) > - put unlikely into reading/writing while loop, because it will proceed > through this case only on last two bytes > - also fix typos in write, le16_to_cpu instead of cpu_to_le16 for 2-byte > writes Even though this looks much better it does not work :-( With this patch applied the card goes "crazy". *sigh* I wish i had more time today to look into this, but it probably has to wait until next week on my side, sorry. But i'll definitly have a close look into this. Thanks so much for all this, Frank - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC 13/13] Char: nozomi, cleanup read and write
nozomi, cleanup read and write
- remove macros testing for endianness, le*_to_cpu and complements will do
it
- pointers cleanup (no need to have different pointers for be and le)
- put unlikely into reading/writing while loop, because it will proceed
through this case only on last two bytes
- also fix typos in write, le16_to_cpu instead of cpu_to_le16 for 2-byte
writes
Signed-off-by: Jiri Slaby <[EMAIL PROTECTED]>
---
commit fe27da2c4a35d2f91fd1ed542d91353f7a3c1dd2
tree a9f8d35b2047a24277d92758d3d579eb59427323
parent 92ca82bcbd5dde95096dac24d0f6a9beab68e003
author Jiri Slaby <[EMAIL PROTECTED]> Sat, 10 Nov 2007 00:19:04 +0100
committer Jiri Slaby <[EMAIL PROTECTED]> Sat, 10 Nov 2007 00:19:04 +0100
drivers/char/nozomi.c | 80 ++---
1 files changed, 16 insertions(+), 64 deletions(-)
diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 0735df0..7379cd4 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -105,6 +105,7 @@
#include
+#include
#define VERSION_STRING DRIVER_DESC " 2.1c (build date: " \
__DATE__ " " __TIME__ ")"
@@ -261,8 +262,7 @@ enum port_type {
PORT_ERROR = -1,
};
-#ifdef __ARMEB__
-/* Big endian */
+#ifdef __BIG_ENDIAN
struct toggles {
unsigned enabled:5; /*
@@ -470,16 +470,10 @@ static inline struct port *get_port_by_tty(const struct
tty_struct *tty)
* -Rewrite cleaner
*/
-static void read_mem32(u32 *buf, const void __iomem *mem_addr_start,
- u32 size_bytes)
+static void read_mem32(u32 *buf, void __iomem *ptr, u32 size_bytes)
{
- u32 i = 0;
-#ifdef __ARMEB__
- const u32 *ptr = (u32 *) mem_addr_start;
-#else
- const u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
u16 *buf16;
+ u32 i = 0;
if (unlikely(!ptr || !buf))
goto out;
@@ -488,40 +482,22 @@ static void read_mem32(u32 *buf, const void __iomem
*mem_addr_start,
switch (size_bytes) {
case 2: /* 2 bytes */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- *buf16 = __le16_to_cpu(readw(ptr));
-#else
- *buf16 = readw((void __iomem *)ptr);
-#endif
+ *buf16 = le16_to_cpu(readw(ptr));
goto out;
- break;
case 4: /* 4 bytes */
-#ifdef __ARMEB__
- *(buf) = __le32_to_cpu(readl(ptr));
-#else
- *(buf) = readl((void __iomem *)ptr);
-#endif
+ *buf = le32_to_cpu(readl(ptr));
goto out;
- break;
}
while (i < size_bytes) {
- if (size_bytes - i == 2) {
+ if (unlikely(size_bytes - i == 2)) {
/* Handle 2 bytes in the end */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- *(buf16) = __le16_to_cpu(readw(ptr));
-#else
- *(buf16) = readw((void __iomem *)ptr);
-#endif
+ *buf16 = le16_to_cpu(readw(ptr));
i += 2;
} else {
/* Read 4 bytes */
-#ifdef __ARMEB__
- *(buf) = __le32_to_cpu(readl(ptr));
-#else
- *(buf) = readl((void __iomem *)ptr);
-#endif
+ *buf = le32_to_cpu(readl(ptr));
i += 4;
}
buf++;
@@ -536,15 +512,9 @@ out:
* -Optimize
* -Rewrite cleaner
*/
-static u32 write_mem32(void __iomem *mem_addr_start, u32 *buf,
- u32 size_bytes)
+static u32 write_mem32(void __iomem *ptr, u32 *buf, u32 size_bytes)
{
u32 i = 0;
-#ifdef __ARMEB__
- u32 *ptr = (u32 *) mem_addr_start;
-#else
- u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
u16 *buf16;
if (unlikely(!ptr || !buf))
@@ -553,41 +523,23 @@ static u32 write_mem32(void __iomem *mem_addr_start, u32
*buf,
/* shortcut for extremely often used cases */
switch (size_bytes) {
case 2: /* 2 bytes */
- buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- writew(__le16_to_cpu(*buf16), ptr);
-#else
- writew(*buf16, (void __iomem *)ptr);
-#endif
+ buf16 = (u16 *)buf;
+ writew(cpu_to_le16(*buf16), ptr);
return 2;
- break;
case 4: /* 4 bytes */
-#ifdef __ARMEB__
- writel(__cpu_to_le32(*buf), ptr);
-#else
- writel(*buf, (void __iomem *)ptr);
-#endif
+ writel(cpu_to_le32(*buf), ptr);
return 4;
- break;
}
while (i < size_bytes) {
- if (size_bytes - i == 2) {
+ if (unlikely(size_bytes - i == 2)) {
/* 2 bytes */
buf16 = (u16 *) buf;
-#ifdef __ARMEB__
- writew(__le16_to_cpu(*buf16), ptr);
-#else
-
