Skip to contentSkip to conversation

lib: fix 842 build on 32-bit architectures

PATCH In review Arnd Bergmann 4 participants 7 messages May 13, 2015 – May 14, 2015

Subsystems (2)
Crypto API IBM Power 842 compression accelerator
lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

PATCH 1/1 lib: fix 842 build on 32-bit architectures

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

By marking that function inline, the fsize argument is always known at the time that do_index is called, and the compiler is able to replace the extremely expensive 64-bit division with a cheap constant shift operation.

Aside from fixing that link error, this approach should also improve both code size and performance on 32-bit architectures significantly.

Signed-off-by Arnd Bergmann <[email protected]>

---
Found while building arm32 allmodconfig with gcc-5.0

lib/842/842_decompress.c +1 -1
index 6b2b45aecde3..285bf6b6959c 100644
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
 	return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static inline int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
 {
 	u64 index, offset, total = round_down(p->out - p->ostart, 8);
 	int ret;
Acked-by on v1 1/1
#1 lore raw
Quoted 4 lines

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

Oops! Guess I should build/test on 32 bit more.

Quoted 9 lines

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

By marking that function inline, the fsize argument is always known at the time that do_index is called, and the compiler is able to replace the extremely expensive 64-bit division with a cheap constant shift operation.

alternately, we know that fsize will always be less than 64 bits, at most it's 4<<9 or 8<<8 (both == 1<<11). So we could just change its type to u16.

lib/842/842_decompress.c +1 -1
index 6b2b45aecde3..285bf6b6959c 100644
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
 	return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static int __do_index(struct sw842_param *p, u8 size, u8 bits, u16 fsize)
 {
 	u64 index, offset, total = round_down(p->out - p->ostart, 8);
 	int ret;

Or, we could inline it and change the type to u16. In any case,

Acked-by Dan Streetman <[email protected]>
Quoted 22 lines

Aside from fixing that link error, this approach should also improve both code size and performance on 32-bit architectures significantly.

Signed-off-by Arnd Bergmann <[email protected]>

---
Found while building arm32 allmodconfig with gcc-5.0

lib/842/842_decompress.c +1 -1
index 6b2b45aecde3..285bf6b6959c 100644
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
 	return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static inline int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
 {
 	u64 index, offset, total = round_down(p->out - p->ostart, 8);
 	int ret;
#2 lore raw
Quoted 11 lines · Dan Streetman
Quoted 4 lines

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

Oops! Guess I should build/test on 32 bit more.

Quoted 4 lines

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

wait, do you mean the 64 bit mod, total % fsize? That should already be fixed in Herbert's tree, I changed it to subtraction instead.

In any case, I looked at the code again and I think the fsize parameter can be removed, and just simply calculated in the function, it's just a shift. I'll send a patch.

Quoted 50 lines
Quoted 5 lines

By marking that function inline, the fsize argument is always known at the time that do_index is called, and the compiler is able to replace the extremely expensive 64-bit division with a cheap constant shift operation.

alternately, we know that fsize will always be less than 64 bits, at most it's 4<<9 or 8<<8 (both == 1<<11). So we could just change its type to u16.

lib/842/842_decompress.c +1 -1
index 6b2b45aecde3..285bf6b6959c 100644
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
        return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static int __do_index(struct sw842_param *p, u8 size, u8 bits, u16 fsize)
 {
        u64 index, offset, total = round_down(p->out - p->ostart, 8);
        int ret;

Or, we could inline it and change the type to u16. In any case,

Acked-by Dan Streetman <[email protected]>
Quoted 22 lines

Aside from fixing that link error, this approach should also improve both code size and performance on 32-bit architectures significantly.

Signed-off-by Arnd Bergmann <[email protected]>

---
Found while building arm32 allmodconfig with gcc-5.0

lib/842/842_decompress.c +1 -1
index 6b2b45aecde3..285bf6b6959c 100644
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
      return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static inline int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
 {
      u64 index, offset, total = round_down(p->out - p->ostart, 8);
      int ret;
author
#3 lore raw
Quoted 9 lines · On Thursday 14 May 2015 04:54:07 Dan Streetman wrote:
Quoted 8 lines · Dan Streetman
Quoted 4 lines

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

Oops! Guess I should build/test on 32 bit more.

Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

wait, do you mean the 64 bit mod, total % fsize? That should already be fixed in Herbert's tree, I changed it to subtraction instead.

Yes, that's the one. I was looking at yesterday's linux-next which still had the bug, but your fix has made it into today's release, so my patch is no longer needed.

Quoted 3 lines

In any case, I looked at the code again and I think the fsize parameter can be removed, and just simply calculated in the function, it's just a shift. I'll send a patch.

Not necessary for this problem any more, but it could still make sense if you think that improves the code. You can also try to see if marking the function inline has any effect on code size or performance if either of them matters.

Arnd

#4 lore raw
Quoted 21 lines · Arnd Bergmann
Quoted 9 lines · On Thursday 14 May 2015 04:54:07 Dan Streetman wrote:
Quoted 8 lines · Dan Streetman
Quoted 4 lines

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

Oops! Guess I should build/test on 32 bit more.

Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

wait, do you mean the 64 bit mod, total % fsize? That should already be fixed in Herbert's tree, I changed it to subtraction instead.

Yes, that's the one. I was looking at yesterday's linux-next which still had the bug, but your fix has made it into today's release, so my patch is no longer needed.

Quoted 8 lines
Quoted 3 lines

In any case, I looked at the code again and I think the fsize parameter can be removed, and just simply calculated in the function, it's just a shift. I'll send a patch.

Not necessary for this problem any more, but it could still make sense if you think that improves the code. You can also try to see if marking the function inline has any effect on code size or performance if either of them matters.

Yep, I'll run some performance tests both ways and send a patch if it does improve things. Thanks!

Quoted 2 lines

Arnd

maintainer
#5 lore raw
In reply to Arnd Bergmann
Quoted 26 lines · Arnd Bergmann

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

By marking that function inline, the fsize argument is always known at the time that do_index is called, and the compiler is able to replace the extremely expensive 64-bit division with a cheap constant shift operation.

Aside from fixing that link error, this approach should also improve both code size and performance on 32-bit architectures significantly.

Signed-off-by Arnd Bergmann <[email protected]>

---
Found while building arm32 allmodconfig with gcc-5.0

lib/842/842_decompress.c +0 -0
index 6b2b45aecde3..285bf6b6959c 100644

lib/842/842_decompress.c static int do_data(struct sw842_param *p, u8 n) · line 172

Quoted 5 lines
lib/842/842_decompress.c +1 -1
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
 	return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static inline int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)

Ugh, relying on inlining to work is fragile. I'm not against making this inline but please make it work even when it is out- of-line.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#6 lore raw
In reply to Arnd Bergmann
Quoted 26 lines · Arnd Bergmann

Building the 842 code on 32-bit ARM currently results in this link error:

ERROR: "__aeabi_uldivmod" [lib/842/842_decompress.ko] undefined!

The reason is that the __do_index function performs a 64-bit division by a power-of-two number, but it has no insight into the function arguments.

By marking that function inline, the fsize argument is always known at the time that do_index is called, and the compiler is able to replace the extremely expensive 64-bit division with a cheap constant shift operation.

Aside from fixing that link error, this approach should also improve both code size and performance on 32-bit architectures significantly.

Signed-off-by Arnd Bergmann <[email protected]>

---
Found while building arm32 allmodconfig with gcc-5.0

lib/842/842_decompress.c +0 -0
index 6b2b45aecde3..285bf6b6959c 100644

lib/842/842_decompress.c static int do_data(struct sw842_param *p, u8 n) · line 172

Quoted 5 lines
lib/842/842_decompress.c +1 -1
@@ -169,7 +169,7 @@ static int do_data(struct sw842_param *p, u8 n)
 	return 0;
 }
 
-static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)
+static inline int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize)

This had better get a comment to say why this is done, to stop the "don't do static inline in a .c" brigade reverting this change.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.