byteorder: eliminate pointer bytorder api

PATCH In review Harvey Harrison 3 participants 7 messages May 20, 2008 – May 27, 2008

Share & tools

Harvey Harrison

author

2008-05-20 19:25 UTC #0 lore raw

PATCH 2/2 byteorder: eliminate pointer bytorder api

Not a great api, should be using cpu_to_etc and deref the pointer yourself. cpu_to_le16p
cpu_to_le32p
cpu_to_le64p
cpu_to_be16p
cpu_to_be32p
cpu_to_be64p

Replaced by the aligned get_/put_ helpers
le16_to_cpup
le32_to_cpup
le64_to_cpup
be16_to_cpup
be32_to_cpup
be64_to_cpup

Also add const to the get/put helpers and use them in the access_ok case for unaligned access.

Signed-off-by Harvey Harrison <[email protected]>

---
 include/linux/byteorder/big_endian.h    |   50 +-----------------------------
 include/linux/byteorder/generic.h       |   24 ++++-----------
 include/linux/byteorder/little_endian.h |   50 +-----------------------------
 include/linux/unaligned/access_ok.h     |   12 ++++----
 4 files changed, 16 insertions(+), 120 deletions(-)

include/linux/byteorder/big_endian.h +2 ~~-48~~

index 961ed4b..c9a5bd7 100644
@@ -15,6 +15,7 @@
 #define __constant_ntohl(x) ((__force __u32)(__be32)(x))
 #define __constant_htons(x) ((__force __be16)(__u16)(x))
 #define __constant_ntohs(x) ((__force __u16)(__be16)(x))
+
 #define __constant_cpu_to_le64(x) ((__force __le64)___constant_swab64((x)))
 #define __constant_le64_to_cpu(x) ___constant_swab64((__force __u64)(__le64)(x))
 #define __constant_cpu_to_le32(x) ((__force __le32)___constant_swab32((x)))

@@ -27,6 +28,7 @@
 #define __constant_be32_to_cpu(x) ((__force __u32)(__be32)(x))
 #define __constant_cpu_to_be16(x) ((__force __be16)(__u16)(x))
 #define __constant_be16_to_cpu(x) ((__force __u16)(__be16)(x))
+
 #define __cpu_to_le64(x) ((__force __le64)__swab64((x)))
 #define __le64_to_cpu(x) __swab64((__force __u64)(__le64)(x))
 #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))

@@ -40,54 +42,6 @@
 #define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
 #define __be16_to_cpu(x) ((__force __u16)(__be16)(x))
 
-static inline __le64 __cpu_to_le64p(const __u64 *p)
-{
-	return (__force __le64)__swab64p(p);
-}
-static inline __u64 __le64_to_cpup(const __le64 *p)
-{
-	return __swab64p((__u64 *)p);
-}
-static inline __le32 __cpu_to_le32p(const __u32 *p)
-{
-	return (__force __le32)__swab32p(p);
-}
-static inline __u32 __le32_to_cpup(const __le32 *p)
-{
-	return __swab32p((__u32 *)p);
-}
-static inline __le16 __cpu_to_le16p(const __u16 *p)
-{
-	return (__force __le16)__swab16p(p);
-}
-static inline __u16 __le16_to_cpup(const __le16 *p)
-{
-	return __swab16p((__u16 *)p);
-}
-static inline __be64 __cpu_to_be64p(const __u64 *p)
-{
-	return (__force __be64)*p;
-}
-static inline __u64 __be64_to_cpup(const __be64 *p)
-{
-	return (__force __u64)*p;
-}
-static inline __be32 __cpu_to_be32p(const __u32 *p)
-{
-	return (__force __be32)*p;
-}
-static inline __u32 __be32_to_cpup(const __be32 *p)
-{
-	return (__force __u32)*p;
-}
-static inline __be16 __cpu_to_be16p(const __u16 *p)
-{
-	return (__force __be16)*p;
-}
-static inline __u16 __be16_to_cpup(const __be16 *p)
-{
-	return (__force __u16)*p;
-}
 #define __cpu_to_le64s(x) __swab64s((x))
 #define __le64_to_cpus(x) __swab64s((x))
 #define __cpu_to_le32s(x) __swab32s((x))

include/linux/byteorder/generic.h +6 ~~-18~~

index 1f0c07e..fee629f 100644
@@ -94,18 +94,6 @@
 #define be32_to_cpu __be32_to_cpu
 #define cpu_to_be16 __cpu_to_be16
 #define be16_to_cpu __be16_to_cpu
-#define cpu_to_le64p __cpu_to_le64p
-#define le64_to_cpup __le64_to_cpup
-#define cpu_to_le32p __cpu_to_le32p
-#define le32_to_cpup __le32_to_cpup
-#define cpu_to_le16p __cpu_to_le16p
-#define le16_to_cpup __le16_to_cpup
-#define cpu_to_be64p __cpu_to_be64p
-#define be64_to_cpup __be64_to_cpup
-#define cpu_to_be32p __cpu_to_be32p
-#define be32_to_cpup __be32_to_cpup
-#define cpu_to_be16p __cpu_to_be16p
-#define be16_to_cpup __be16_to_cpup
 #define cpu_to_le64s __cpu_to_le64s
 #define le64_to_cpus __le64_to_cpus
 #define cpu_to_le32s __cpu_to_le32s

@@ -119,32 +107,32 @@
 #define cpu_to_be16s __cpu_to_be16s
 #define be16_to_cpus __be16_to_cpus
 
-static inline u16 get_le16(void *ptr)
+static inline u16 get_le16(const void *ptr)
 {
 	return le16_to_cpu(*(__le16 *)ptr);
 }
 
-static inline u32 get_le32(void *ptr)
+static inline u32 get_le32(const void *ptr)
 {
 	return le32_to_cpu(*(__le32 *)ptr);
 }
 
-static inline u64 get_le64(void *ptr)
+static inline u64 get_le64(const void *ptr)
 {
 	return le64_to_cpu(*(__le64 *)ptr);
 }
 
-static inline u16 get_be16(void *ptr)
+static inline u16 get_be16(const void *ptr)
 {
 	return be16_to_cpu(*(__be16 *)ptr);
 }
 
-static inline u32 get_be32(void *ptr)
+static inline u32 get_be32(const void *ptr)
 {
 	return be32_to_cpu(*(__be32 *)ptr);
 }
 
-static inline u64 get_be64(void *ptr)
+static inline u64 get_be64(const void *ptr)
 {
 	return be64_to_cpu(*(__be64 *)ptr);
 }

include/linux/byteorder/little_endian.h +2 ~~-48~~

68 lines not on this page · raw message

include/linux/unaligned/access_ok.h +6 -6

index 99c1b4d..c16698a 100644
@@ -6,32 +6,32 @@
 
 static inline u16 get_unaligned_le16(const void *p)
 {
-	return le16_to_cpup((__le16 *)p);
+	return get_le16(p);
 }
 
 static inline u32 get_unaligned_le32(const void *p)
 {
-	return le32_to_cpup((__le32 *)p);
+	return get_le32(p);
 }
 
 static inline u64 get_unaligned_le64(const void *p)
 {
-	return le64_to_cpup((__le64 *)p);
+	return get_le64(p);
 }
 
 static inline u16 get_unaligned_be16(const void *p)
 {
-	return be16_to_cpup((__be16 *)p);
+	return get_be16(p);
 }
 
 static inline u32 get_unaligned_be32(const void *p)
 {
-	return be32_to_cpup((__be32 *)p);
+	return get_be32(p);
 }
 
 static inline u64 get_unaligned_be64(const void *p)
 {
-	return be64_to_cpup((__be64 *)p);
+	return get_be64(p);
 }
 
 static inline void put_unaligned_le16(u16 val, void *p)

Signature · 1 line

1.5.5.1.570.g26b5e

David Miller

2008-05-20 21:17 UTC #1 lore raw

From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 12:24:09 -0700

Quoted 16 lines

Not a great api, should be using cpu_to_etc and deref the pointer yourself. cpu_to_le16p
cpu_to_le32p
cpu_to_le64p
cpu_to_be16p
cpu_to_be32p
cpu_to_be64p
Replaced by the aligned get_/put_ helpers
le16_to_cpup
le32_to_cpup
le64_to_cpup
be16_to_cpup
be32_to_cpup
be64_to_cpup

Quoted 4 lines

Also add const to the get/put helpers and use them in the access_ok case for unaligned access.
Signed-off-by Harvey Harrison <[email protected]>

But what you're doing in the first patch is killing performance for some cases.

The reason we use the cpu_to_*p() interfaces is to get the other-endian load and store instructions some processors have.

What you're doing is undoing all of that work we've done to take advantage of such things.

Harvey Harrison

author

2008-05-20 22:15 UTC #2 lore raw

Quoted 24 lines · David Miller

From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 12:24:09 -0700
Quoted 16 lines
Not a great api, should be using cpu_to_etc and deref the pointer yourself. cpu_to_le16p
cpu_to_le32p
cpu_to_le64p
cpu_to_be16p
cpu_to_be32p
cpu_to_be64p
Replaced by the aligned get_/put_ helpers
le16_to_cpup
le32_to_cpup
le64_to_cpup
be16_to_cpup
be32_to_cpup
be64_to_cpup
Quoted 4 lines
Also add const to the get/put helpers and use them in the access_ok case for unaligned access.
Signed-off-by Harvey Harrison <[email protected]>

Quoted 8 lines

But what you're doing in the first patch is killing performance for some cases.
The reason we use the cpu_to_*p() interfaces is to get the other-endian load and store instructions some processors have.
What you're doing is undoing all of that work we've done to take advantage of such things.

Obviously I missed that part, my apologies. Would it be acceptable if, taking the possibly arch-specific parts, moved the [endian]_to_cpup name over to get_[endian]

ie:

le16_to_cpup -> get_le16

I'd leave cpu_to_le16p alone (it's not used that much anyway).

To make it similar to the get_unaligned_le16 helpers?

Thanks for having a look,

Harvey

David Miller

2008-05-20 22:20 UTC #3 lore raw

From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 15:15:25 -0700

Quoted 3 lines

Obviously I missed that part, my apologies. Would it be acceptable if, taking the possibly arch-specific parts, moved the [endian]_to_cpup name over to get_[endian]

Why are we fiddling with interface names that have been fine for about 10 years?

Harvey Harrison

author

2008-05-20 22:30 UTC #4 lore raw

Quoted 9 lines · David Miller

From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 15:15:25 -0700
Quoted 3 lines
Obviously I missed that part, my apologies. Would it be acceptable if, taking the possibly arch-specific parts, moved the [endian]_to_cpup name over to get_[endian]
Why are we fiddling with interface names that have been fine for about 10 years?

Saw a lot of (or similar in a private helper):

*(__be32 *)ptr = cpu_to_be32(val);

So I came up with

void put_be32(val, ptr);

This looked a lot like the put_unaligned_be32 helpers and only left a gap that was get_be32(ptr).

But this was exactly the same as the existing be32_to_cpup, so I wasn't sure if I should add it or not. In the end I just went ahead and did it and wanted to see what the patch would be like moving over existing users to the new api looked like.

On top of that I did the cpu_to_be32p removal, which probably was not the brightest thing ever.

So, that leaves (repeat for various endian values, be32 is an example):

1) should put_be32(val, ptr) be added (it seems useful and lots of code is already rolling their own if they aren't opencoding it)

2) should get_be32 be added purely to have a symmetric api, even through it is equivalent to be32_to_cpup? (nice that it looks just like get_unaligned_be32...makes alignment explicit)

3) if 2) should existiing be32_to_cpup users be moved over to the new api

4) if 3) should cpu_to_be32p be moved/changed at all (unlikely)

I hope that explains where I was coming from with this set of patches.

Harvey

Jan Engelhardt

2008-05-26 12:18 UTC #5 lore raw

Quoted 10 lines · Harvey Harrison

Quoted 9 lines · David Miller
From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 15:15:25 -0700
Quoted 3 lines
Obviously I missed that part, my apologies. Would it be acceptable if, taking the possibly arch-specific parts, moved the [endian]_to_cpup name over to get_[endian]
Why are we fiddling with interface names that have been fine for about 10 years?

I suggest some comments be added to the cpu_to_*p() to specify their reason for being there (namely, speedups on some CPUs)

Quoted 7 lines

Saw a lot of (or similar in a private helper):
*(__be32 *)ptr = cpu_to_be32(val);
So I came up with
void put_be32(val, ptr);

I think it would be better to follow the common notation of the target being on the left side (like most intel asm commands and things like C's memcpy, etc)

	void put_be32(ptr, val);

Harvey Harrison

author

2008-05-27 22:40 UTC #6 lore raw

Quoted 9 lines · Jan Engelhardt

Quoted 8 lines · Harvey Harrison
Quoted 7 lines · David Miller
From: Harvey Harrison <[email protected]>
Date: Tue, 20 May 2008 15:15:25 -0700
Quoted 3 lines
Obviously I missed that part, my apologies. Would it be acceptable if, taking the possibly arch-specific parts, moved the [endian]_to_cpup name over to get_[endian]

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
Why are we fiddling with interface names that have been fine for about 10 years?
I suggest some comments be added to the cpu_to_*p() to specify their reason for being there (namely, speedups on some CPUs)

Agreed.

Quoted 5 lines

Quoted 5 lines
Saw a lot of (or similar in a private helper):
*(__be32 *)ptr = cpu_to_be32(val);

Quoted 7 lines

Quoted 3 lines
So I came up with
void put_be32(val, ptr);
I think it would be better to follow the common notation of the target being on the left side (like most intel asm commands and things like C's memcpy, etc)

I based this on the existing put_unaligned_be32 to have the same arg order.

Cheers,

Harvey