Consolidation of asm/unaligned.h

Consolidation of asm/unaligned.h

[David S. Miller]

Several architectures do their asm/unaligned.h unaligned load/store
support by simply casting the value to a packed strcuture, and
this forces gcc to assume the object is not aligned properly.

This technique originated in Richard Henderson's asm-alpha/unaligned.h,
IA64 uses the same technique as well.

This works well on RISC systems for two reasons:

1) On systems like Alpha, MIPS, et al. which have special
   "load unaligned" instructions, GCC knows to emit them
   for code like this.

2) Even on systems without explicit unaligned load/store instruction
   support, the code emitted (basically, byte loads with shifts and
   ors) is about the same as you get when emitting a memmove() call
   and you don't need the local stack slot to boot.

I was going to thus move asm-sparc64/unaligned.h over to such a
scheme, but then I noticed that nobody actually includes the current
memmove() based asm-generic/unaligned.h code.  So why not put the
portable packed structure implementation into asm-generic/unaligned.h
and then make asm-{alpha,ia64,sparc64}/unaligned.h simply include that?

I only had to make minor modifications to the alpha header when placing
it into the generic area.  In particular I had to convert some explicit
"unsigned long", "unsigned int" et al. into the arch-agnostic "u64" "u32"
etc.  so that even 32-bit platforms could use this.

Come to think of it I'll make sparc32 use this as well.

I looked at some other platform unaligned.h headers:

1) ARM is trying to be incredibly clever, and open codes the shifts
   and ors.  I think it would be better if it used something similar
   to the packed structure technique.

2) CRIS, like x86, can do unaligned stuff directly.

3) FRV needs help doing unaligned stuff, it probably also could use
   the packed structure stuff.

4) h8300 needs help, could use this new asm-generic/unaligned.h header

5) m32r likewise

6) M68K can do unaligned access directly.

7) MIPS appears to be a copy of the original alpha/ia64 unaligned.h header,
   so I converted it to use the new asm-generic/unaligned.h too

8) PARISC is just a copy of asm-sparc/unaligned.h, so I converted it over
   to use asm-generic/unaligned.h too

9) PPC/PPC64 can do unaligned access directly in big-endian mode which is
   what the Linux kernel runs in

10) S390 can do it directly as well

11) SH/SH64 just has the memmove() code ala asm-sparc/unaligned.h, I converted
    it to use asm-generic/unaligned.h

12) V850 has some clever code just like ARM, so I didn't touch it.

So this is the patch I came up with.

Any suggested changes or objections?

Thanks.

===== include/asm-alpha/unaligned.h 1.2 vs edited =====
--- 1.2/include/asm-alpha/unaligned.h	2003-06-08 15:31:17 -07:00
+++ edited/include/asm-alpha/unaligned.h	2005-03-17 10:25:43 -08:00
@@ -1,114 +1,6 @@
 #ifndef __ALPHA_UNALIGNED_H
 #define __ALPHA_UNALIGNED_H
 
-/* 
- * The main single-value unaligned transfer routines.
- */
-#define get_unaligned(ptr) \
-	((__typeof__(*(ptr)))__get_unaligned((ptr), sizeof(*(ptr))))
-#define put_unaligned(x,ptr) \
-	__put_unaligned((unsigned long)(x), (ptr), sizeof(*(ptr)))
-
-/*
- * This is a silly but good way to make sure that
- * the get/put functions are indeed always optimized,
- * and that we use the correct sizes.
- */
-extern void bad_unaligned_access_length(void) __attribute__((noreturn));
-
-/*
- * EGCS 1.1 knows about arbitrary unaligned loads.  Define some
- * packed structures to talk about such things with.
- */
-
-struct __una_u64 { __u64 x __attribute__((packed)); };
-struct __una_u32 { __u32 x __attribute__((packed)); };
-struct __una_u16 { __u16 x __attribute__((packed)); };
-
-/*
- * Elemental unaligned loads 
- */
-
-extern inline unsigned long __uldq(const unsigned long * r11)
-{
-	const struct __una_u64 *ptr = (const struct __una_u64 *) r11;
-	return ptr->x;
-}
-
-extern inline unsigned long __uldl(const unsigned int * r11)
-{
-	const struct __una_u32 *ptr = (const struct __una_u32 *) r11;
-	return ptr->x;
-}
-
-extern inline unsigned long __uldw(const unsigned short * r11)
-{
-	const struct __una_u16 *ptr = (const struct __una_u16 *) r11;
-	return ptr->x;
-}
-
-/*
- * Elemental unaligned stores 
- */
-
-extern inline void __ustq(unsigned long r5, unsigned long * r11)
-{
-	struct __una_u64 *ptr = (struct __una_u64 *) r11;
-	ptr->x = r5;
-}
-
-extern inline void __ustl(unsigned long r5, unsigned int * r11)
-{
-	struct __una_u32 *ptr = (struct __una_u32 *) r11;
-	ptr->x = r5;
-}
-
-extern inline void __ustw(unsigned long r5, unsigned short * r11)
-{
-	struct __una_u16 *ptr = (struct __una_u16 *) r11;
-	ptr->x = r5;
-}
-
-extern inline unsigned long __get_unaligned(const void *ptr, size_t size)
-{
-	unsigned long val;
-	switch (size) {
-	      case 1:
-		val = *(const unsigned char *)ptr;
-		break;
-	      case 2:
-		val = __uldw((const unsigned short *)ptr);
-		break;
-	      case 4:
-		val = __uldl((const unsigned int *)ptr);
-		break;
-	      case 8:
-		val = __uldq((const unsigned long *)ptr);
-		break;
-	      default:
-		bad_unaligned_access_length();
-	}
-	return val;
-}
-
-extern inline void __put_unaligned(unsigned long val, void *ptr, size_t size)
-{
-	switch (size) {
-	      case 1:
-		*(unsigned char *)ptr = (val);
-	        break;
-	      case 2:
-		__ustw(val, (unsigned short *)ptr);
-		break;
-	      case 4:
-		__ustl(val, (unsigned int *)ptr);
-		break;
-	      case 8:
-		__ustq(val, (unsigned long *)ptr);
-		break;
-	      default:
-	    	bad_unaligned_access_length();
-	}
-}
+#include <asm-generic/unaligned.h>
 
 #endif
===== include/asm-generic/unaligned.h 1.1 vs edited =====
--- 1.1/include/asm-generic/unaligned.h	2002-02-05 09:39:44 -08:00
+++ edited/include/asm-generic/unaligned.h	2005-03-17 10:25:14 -08:00
@@ -4,17 +4,118 @@
 /*
  * For the benefit of those who are trying to port Linux to another
  * architecture, here are some C-language equivalents. 
+ *
+ * This is based almost entirely upon Richard Henderson's
+ * asm-alpha/unaligned.h implementation.  Some comments were
+ * taken from David Mosberger's asm-ia64/unaligned.h header.
  */
 
-#include <asm/string.h>
-
+#include <linux/types.h>
 
+/* 
+ * The main single-value unaligned transfer routines.
+ */
 #define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memcpy(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
+	((__typeof__(*(ptr)))__get_unaligned((ptr), sizeof(*(ptr))))
+#define put_unaligned(x,ptr) \
+	__put_unaligned((unsigned long)(x), (ptr), sizeof(*(ptr)))
+
+/*
+ * This function doesn't actually exist.  The idea is that when
+ * someone uses the macros below with an unsupported size (datatype),
+ * the linker will alert us to the problem via an unresolved reference
+ * error.
+ */
+extern void bad_unaligned_access_length(void) __attribute__((noreturn));
+
+struct __una_u64 { __u64 x __attribute__((packed)); };
+struct __una_u32 { __u32 x __attribute__((packed)); };
+struct __una_u16 { __u16 x __attribute__((packed)); };
+
+/*
+ * Elemental unaligned loads 
+ */
+
+static inline unsigned long __uldq(const __u64 *addr)
+{
+	const struct __una_u64 *ptr = (const struct __una_u64 *) addr;
+	return ptr->x;
+}
+
+static inline unsigned long __uldl(const __u32 *addr)
+{
+	const struct __una_u32 *ptr = (const struct __una_u32 *) addr;
+	return ptr->x;
+}
+
+static inline unsigned long __uldw(const __u16 *addr)
+{
+	const struct __una_u16 *ptr = (const struct __una_u16 *) addr;
+	return ptr->x;
+}
+
+/*
+ * Elemental unaligned stores 
+ */
+
+static inline void __ustq(__u64 val, __u64 *addr)
+{
+	struct __una_u64 *ptr = (struct __una_u64 *) addr;
+	ptr->x = val;
+}
+
+static inline void __ustl(__u32 val, __u32 *addr)
+{
+	struct __una_u32 *ptr = (struct __una_u32 *) addr;
+	ptr->x = val;
+}
+
+static inline void __ustw(__u16 val, __u16 *addr)
+{
+	struct __una_u16 *ptr = (struct __una_u16 *) addr;
+	ptr->x = val;
+}
+
+static inline unsigned long __get_unaligned(const void *ptr, size_t size)
+{
+	unsigned long val;
+	switch (size) {
+	case 1:
+		val = *(const __u8 *)ptr;
+		break;
+	case 2:
+		val = __uldw((const __u16 *)ptr);
+		break;
+	case 4:
+		val = __uldl((const __u32 *)ptr);
+		break;
+	case 8:
+		val = __uldq((const __u64 *)ptr);
+		break;
+	default:
+		bad_unaligned_access_length();
+	};
+	return val;
+}
 
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memcpy((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+static inline void __put_unaligned(unsigned long val, void *ptr, size_t size)
+{
+	switch (size) {
+	case 1:
+		*(__u8 *)ptr = val;
+	        break;
+	case 2:
+		__ustw(val, (__u16 *)ptr);
+		break;
+	case 4:
+		__ustl(val, (__u32 *)ptr);
+		break;
+	case 8:
+		__ustq(val, (__u64 *)ptr);
+		break;
+	default:
+	    	bad_unaligned_access_length();
+	};
+}
 
 #endif /* _ASM_GENERIC_UNALIGNED_H */
===== include/asm-ia64/unaligned.h 1.4 vs edited =====
--- 1.4/include/asm-ia64/unaligned.h	2004-01-23 10:52:25 -08:00
+++ edited/include/asm-ia64/unaligned.h	2005-03-17 10:25:54 -08:00
@@ -1,121 +1,6 @@
 #ifndef _ASM_IA64_UNALIGNED_H
 #define _ASM_IA64_UNALIGNED_H
 
-#include <linux/types.h>
-
-/*
- * The main single-value unaligned transfer routines.
- *
- * Based on <asm-alpha/unaligned.h>.
- *
- * Copyright (C) 1998, 1999, 2003 Hewlett-Packard Co
- *	David Mosberger-Tang <davidm@hpl.hp.com>
- */
-#define get_unaligned(ptr) \
-	((__typeof__(*(ptr)))ia64_get_unaligned((ptr), sizeof(*(ptr))))
-
-#define put_unaligned(x,ptr) \
-	ia64_put_unaligned((unsigned long)(x), (ptr), sizeof(*(ptr)))
-
-struct __una_u64 { __u64 x __attribute__((packed)); };
-struct __una_u32 { __u32 x __attribute__((packed)); };
-struct __una_u16 { __u16 x __attribute__((packed)); };
-
-static inline unsigned long
-__uld8 (const unsigned long * addr)
-{
-	const struct __una_u64 *ptr = (const struct __una_u64 *) addr;
-	return ptr->x;
-}
-
-static inline unsigned long
-__uld4 (const unsigned int * addr)
-{
-	const struct __una_u32 *ptr = (const struct __una_u32 *) addr;
-	return ptr->x;
-}
-
-static inline unsigned long
-__uld2 (const unsigned short * addr)
-{
-	const struct __una_u16 *ptr = (const struct __una_u16 *) addr;
-	return ptr->x;
-}
-
-static inline void
-__ust8 (unsigned long val, unsigned long * addr)
-{
-	struct __una_u64 *ptr = (struct __una_u64 *) addr;
-	ptr->x = val;
-}
-
-static inline void
-__ust4 (unsigned long val, unsigned int * addr)
-{
-	struct __una_u32 *ptr = (struct __una_u32 *) addr;
-	ptr->x = val;
-}
-
-static inline void
-__ust2 (unsigned long val, unsigned short * addr)
-{
-	struct __una_u16 *ptr = (struct __una_u16 *) addr;
-	ptr->x = val;
-}
-
-
-/*
- * This function doesn't actually exist.  The idea is that when someone uses the macros
- * below with an unsupported size (datatype), the linker will alert us to the problem via
- * an unresolved reference error.
- */
-extern unsigned long ia64_bad_unaligned_access_length (void);
-
-#define ia64_get_unaligned(_ptr,size)						\
-({										\
-	const void *__ia64_ptr = (_ptr);					\
-	unsigned long __ia64_val;						\
-										\
-	switch (size) {								\
-	      case 1:								\
-		__ia64_val = *(const unsigned char *) __ia64_ptr;		\
-		break;								\
-	      case 2:								\
-		__ia64_val = __uld2((const unsigned short *)__ia64_ptr);	\
-		break;								\
-	      case 4:								\
-		__ia64_val = __uld4((const unsigned int *)__ia64_ptr);		\
-		break;								\
-	      case 8:								\
-		__ia64_val = __uld8((const unsigned long *)__ia64_ptr);		\
-		break;								\
-	      default:								\
-		__ia64_val = ia64_bad_unaligned_access_length();		\
-	}									\
-	__ia64_val;								\
-})
-
-#define ia64_put_unaligned(_val,_ptr,size)				\
-do {									\
-	const void *__ia64_ptr = (_ptr);				\
-	unsigned long __ia64_val = (_val);				\
-									\
-	switch (size) {							\
-	      case 1:							\
-		*(unsigned char *)__ia64_ptr = (__ia64_val);		\
-	        break;							\
-	      case 2:							\
-		__ust2(__ia64_val, (unsigned short *)__ia64_ptr);	\
-		break;							\
-	      case 4:							\
-		__ust4(__ia64_val, (unsigned int *)__ia64_ptr);		\
-		break;							\
-	      case 8:							\
-		__ust8(__ia64_val, (unsigned long *)__ia64_ptr);	\
-		break;							\
-	      default:							\
-	    	ia64_bad_unaligned_access_length();			\
-	}								\
-} while (0)
+#include <asm-generic/unaligned.h>
 
 #endif /* _ASM_IA64_UNALIGNED_H */
===== include/asm-mips/unaligned.h 1.5 vs edited =====
--- 1.5/include/asm-mips/unaligned.h	2004-02-19 12:53:03 -08:00
+++ edited/include/asm-mips/unaligned.h	2005-03-17 10:43:02 -08:00
@@ -9,136 +9,6 @@
 #ifndef _ASM_UNALIGNED_H
 #define _ASM_UNALIGNED_H
 
-#include <linux/types.h>
-
-/*
- * get_unaligned - get value from possibly mis-aligned location
- * @ptr: pointer to value
- *
- * This macro should be used for accessing values larger in size than
- * single bytes at locations that are expected to be improperly aligned,
- * e.g. retrieving a u16 value from a location not u16-aligned.
- *
- * Note that unaligned accesses can be very expensive on some architectures.
- */
-#define get_unaligned(ptr) \
-	((__typeof__(*(ptr)))__get_unaligned((ptr), sizeof(*(ptr))))
-
-/*
- * put_unaligned - put value to a possibly mis-aligned location
- * @val: value to place
- * @ptr: pointer to location
- *
- * This macro should be used for placing values larger in size than
- * single bytes at locations that are expected to be improperly aligned,
- * e.g. writing a u16 value to a location not u16-aligned.
- *
- * Note that unaligned accesses can be very expensive on some architectures.
- */
-#define put_unaligned(x,ptr) \
-	__put_unaligned((__u64)(x), (ptr), sizeof(*(ptr)))
-
-/*
- * This is a silly but good way to make sure that
- * the get/put functions are indeed always optimized,
- * and that we use the correct sizes.
- */
-extern void bad_unaligned_access_length(void);
-
-/*
- * EGCS 1.1 knows about arbitrary unaligned loads.  Define some
- * packed structures to talk about such things with.
- */
-
-struct __una_u64 { __u64 x __attribute__((packed)); };
-struct __una_u32 { __u32 x __attribute__((packed)); };
-struct __una_u16 { __u16 x __attribute__((packed)); };
-
-/*
- * Elemental unaligned loads 
- */
-
-static inline __u64 __uldq(const __u64 * r11)
-{
-	const struct __una_u64 *ptr = (const struct __una_u64 *) r11;
-	return ptr->x;
-}
-
-static inline __u32 __uldl(const __u32 * r11)
-{
-	const struct __una_u32 *ptr = (const struct __una_u32 *) r11;
-	return ptr->x;
-}
-
-static inline __u16 __uldw(const __u16 * r11)
-{
-	const struct __una_u16 *ptr = (const struct __una_u16 *) r11;
-	return ptr->x;
-}
-
-/*
- * Elemental unaligned stores 
- */
-
-static inline void __ustq(__u64 r5, __u64 * r11)
-{
-	struct __una_u64 *ptr = (struct __una_u64 *) r11;
-	ptr->x = r5;
-}
-
-static inline void __ustl(__u32 r5, __u32 * r11)
-{
-	struct __una_u32 *ptr = (struct __una_u32 *) r11;
-	ptr->x = r5;
-}
-
-static inline void __ustw(__u16 r5, __u16 * r11)
-{
-	struct __una_u16 *ptr = (struct __una_u16 *) r11;
-	ptr->x = r5;
-}
-
-static inline __u64 __get_unaligned(const void *ptr, size_t size)
-{
-	__u64 val;
-
-	switch (size) {
-	case 1:
-		val = *(const __u8 *)ptr;
-		break;
-	case 2:
-		val = __uldw((const __u16 *)ptr);
-		break;
-	case 4:
-		val = __uldl((const __u32 *)ptr);
-		break;
-	case 8:
-		val = __uldq((const __u64 *)ptr);
-		break;
-	default:
-		bad_unaligned_access_length();
-	}
-	return val;
-}
-
-static inline void __put_unaligned(__u64 val, void *ptr, size_t size)
-{
-	switch (size) {
-	      case 1:
-		*(__u8 *)ptr = (val);
-	        break;
-	      case 2:
-		__ustw(val, (__u16 *)ptr);
-		break;
-	      case 4:
-		__ustl(val, (__u32 *)ptr);
-		break;
-	      case 8:
-		__ustq(val, (__u64 *)ptr);
-		break;
-	      default:
-	    	bad_unaligned_access_length();
-	}
-}
+#include <asm-generic/unaligned.h>
 
 #endif /* _ASM_UNALIGNED_H */
===== include/asm-parisc/unaligned.h 1.2 vs edited =====
--- 1.2/include/asm-parisc/unaligned.h	2002-07-20 02:52:25 -07:00
+++ edited/include/asm-parisc/unaligned.h	2005-03-17 10:44:06 -08:00
@@ -1,22 +1,7 @@
 #ifndef _ASM_PARISC_UNALIGNED_H_
 #define _ASM_PARISC_UNALIGNED_H_
 
-/* parisc can't handle unaligned accesses. */
-/* copied from asm-sparc/unaligned.h */
-
-#include <linux/string.h>
-
-
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
-
+#include <asm-parisc/unaligned.h>
 
 #ifdef __KERNEL__
 struct pt_regs;
===== include/asm-sh/unaligned.h 1.1 vs edited =====
--- 1.1/include/asm-sh/unaligned.h	2002-02-05 09:39:53 -08:00
+++ edited/include/asm-sh/unaligned.h	2005-03-17 10:45:52 -08:00
@@ -2,18 +2,6 @@
 #define __ASM_SH_UNALIGNED_H
 
 /* SH can't handle unaligned accesses. */
-
-#include <linux/string.h>
-
-
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+#include <asm-generic/unaligned.h>
 
 #endif /* __ASM_SH_UNALIGNED_H */
===== include/asm-sh64/unaligned.h 1.1 vs edited =====
--- 1.1/include/asm-sh64/unaligned.h	2004-06-29 07:44:46 -07:00
+++ edited/include/asm-sh64/unaligned.h	2005-03-17 10:46:04 -08:00
@@ -12,17 +12,6 @@
  *
  */
 
-#include <linux/string.h>
-
-
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+#include <asm-generic/unaligned.h>
 
 #endif /* __ASM_SH64_UNALIGNED_H */
===== include/asm-sparc/unaligned.h 1.1 vs edited =====
--- 1.1/include/asm-sparc/unaligned.h	2002-02-05 09:39:48 -08:00
+++ edited/include/asm-sparc/unaligned.h	2005-03-17 10:38:21 -08:00
@@ -1,19 +1,6 @@
 #ifndef _ASM_SPARC_UNALIGNED_H_
 #define _ASM_SPARC_UNALIGNED_H_
 
-/* Sparc can't handle unaligned accesses. */
-
-#include <linux/string.h>
-
-
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+#include <asm-generic/unaligned.h>
 
 #endif /* _ASM_SPARC_UNALIGNED_H */
===== include/asm-sparc64/unaligned.h 1.1 vs edited =====
--- 1.1/include/asm-sparc64/unaligned.h	2002-02-05 09:39:50 -08:00
+++ edited/include/asm-sparc64/unaligned.h	2005-03-17 10:26:04 -08:00
@@ -1,19 +1,6 @@
 #ifndef _ASM_SPARC64_UNALIGNED_H_
 #define _ASM_SPARC64_UNALIGNED_H_
 
-/* Sparc can't handle unaligned accesses. */
-
-#include <linux/string.h>
-
-
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+#include <asm-generic/unaligned.h>
 
 #endif /* _ASM_SPARC64_UNALIGNED_H */

Re: Consolidation of asm/unaligned.h

[Russell King]

On Thu, Mar 17, 2005 at 10:47:44AM -0800, David S. Miller wrote:
> 1) ARM is trying to be incredibly clever, and open codes the shifts
>    and ors.  I think it would be better if it used something similar
>    to the packed structure technique.

I agree with your sentiment, though I'd like to compare the produced
code to ensure that there isn't something unexpected somewhere.

Thanks Dave, this looks interesting.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Consolidation of asm/unaligned.h

[David S. Miller]

On Thu, 17 Mar 2005 21:33:33 +0000
Russell King <rmk@arm.linux.org.uk> wrote:

> I agree with your sentiment, though I'd like to compare the produced
> code to ensure that there isn't something unexpected somewhere.
> 
> Thanks Dave, this looks interesting.

No problem.  I didn't convert ARM, so you can just do so when you
have determined that such a change is appropriate.

Re: Consolidation of asm/unaligned.h

[Geert Uytterhoeven]

On Thu, 17 Mar 2005, David S. Miller wrote:
> 6) M68K can do unaligned access directly.

Just for the record, this is true for 68020 and later only. I.e. some MMU-less
variants need unaligned support.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Consolidation of asm/unaligned.h

[David S. Miller]

On Thu, 17 Mar 2005 22:57:25 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Thu, 17 Mar 2005, David S. Miller wrote:
> > 6) M68K can do unaligned access directly.
> 
> Just for the record, this is true for 68020 and later only. I.e. some MMU-less
> variants need unaligned support.

Thanks for the info.  I've just added the following diff
on top so that the COLDFIRE build uses the new generic
unaligned header.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/17 14:02:44-08:00 davem@sunset.davemloft.net 
#   [M68KNOMMU]: Use asm-generic/unaligned.h for COLDFIRE.
#   
#   Based upon comments from Geert Uytterhoeven.
#   
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# include/asm-m68knommu/unaligned.h
#   2005/03/17 14:02:09-08:00 davem@sunset.davemloft.net +1 -9
#   [M68KNOMMU]: Use asm-generic/unaligned.h for COLDFIRE.
# 
diff -Nru a/include/asm-m68knommu/unaligned.h b/include/asm-m68knommu/unaligned.h
--- a/include/asm-m68knommu/unaligned.h	2005-03-17 14:03:19 -08:00
+++ b/include/asm-m68knommu/unaligned.h	2005-03-17 14:03:19 -08:00
@@ -5,15 +5,7 @@
 
 #ifdef CONFIG_COLDFIRE
 
-/* Use memmove here, so gcc does not insert a __builtin_memcpy. */
-
-#define get_unaligned(ptr) \
-  ({ __typeof__(*(ptr)) __tmp; memmove(&__tmp, (ptr), sizeof(*(ptr))); __tmp; })
-
-#define put_unaligned(val, ptr)				\
-  ({ __typeof__(*(ptr)) __tmp = (val);			\
-     memmove((ptr), &__tmp, sizeof(*(ptr)));		\
-     (void)0; })
+#include <asm-generic/unaligned.h>
 
 #else
 /*

Re: Consolidation of asm/unaligned.h

[Ralf Baechle]

On Thu, Mar 17, 2005 at 10:47:44AM -0800, David S. Miller wrote:

> 7) MIPS appears to be a copy of the original alpha/ia64 unaligned.h header,
>    so I converted it to use the new asm-generic/unaligned.h too

That's indeed the heritage of the current MIPS implementation.  We used to
use inline assembler because some older compiler (afair egcs 1.0.3a or
egcs 1.1.2 vintage) used to generate very bad code from the packed method.

> Any suggested changes or objections?

Looks good.

  Ralf

Re: Consolidation of asm/unaligned.h

[Richard Henderson]

On Thu, Mar 17, 2005 at 02:03:16PM -0800, David S. Miller wrote:
> Thanks for the info.  I've just added the following diff
> on top so that the COLDFIRE build uses the new generic
> unaligned header.

You do realize that the compiler knows when the target supports unaligned
loads, and optimizes the __attribute__((packed)) case appropriately?  I'd
be surprised if *any* target needed a specialized version (indeed, if so,
I'd consider this a failing in the compiler).


r~

Re: Consolidation of asm/unaligned.h

[David Woodhouse]

On Thu, 2005-03-17 at 10:47 -0800, David S. Miller wrote:
> Any suggested changes or objections?

There remains the question of _when_ it's sane to cross over from a
direct dereference (and possible trap) to using get_unaligned().

This in fact differs violently between architectures. On i386 we might
as well _always_ just dereference, while on a large number of MMU-less
machines the needle is at the other end of the scale -- we now support
machines on which we _cannot_ fix up alignment traps (bonus points, btw,
to whomever came up with _imprecise_ alignment exceptions for FR-V).

At the moment, the decision of whether to use get_unaligned() or a
direct dereference seems to be largely random -- there are no real
guidelines on when to make either choice. I'd like to see the
get_unaligned() macro take _two_ arguments; the first being the pointer,
and the second being the estimated probability that it'll _actually_ be
unaligned. That probability doesn't have to be particularly accurate;
just enough to let the architecture code take the hint. Some
architectures will need to do the manual load in 100% of cases; some in
0% of cases, and others in between. We'd accept only constants rather
than being silly about it, so the appropriate path can be chosen
entirely at compile time.

-- 
dwmw2

Re: Consolidation of asm/unaligned.h

[David S. Miller]

On Thu, 17 Mar 2005 16:20:34 -0800
Richard Henderson <rth@twiddle.net> wrote:

> You do realize that the compiler knows when the target supports unaligned
> loads, and optimizes the __attribute__((packed)) case appropriately?  I'd
> be surprised if *any* target needed a specialized version (indeed, if so,
> I'd consider this a failing in the compiler).

That's a good point.

Less compilation overhead though for the direct pointer deref defines.

Re: Consolidation of asm/unaligned.h

[James Bottomley]

I just got around to checking this on parisc, and I'm afraid we have a
toolchain cockup:  Our gcc can't optimise the sizeof() if the user is an
inline function.  It can, however if the functions are made #defines
instead.

Would the attached be OK with everyone?  It works fine for us.

Thanks,

James

===== include/asm-generic/unaligned.h 1.2 vs edited =====
--- 1.2/include/asm-generic/unaligned.h	2005-03-17 15:54:10 -06:00
+++ edited/include/asm-generic/unaligned.h	2005-04-03 21:17:14 -05:00
@@ -12,14 +12,6 @@
 
 #include <linux/types.h>
 
-/* 
- * The main single-value unaligned transfer routines.
- */
-#define get_unaligned(ptr) \
-	((__typeof__(*(ptr)))__get_unaligned((ptr), sizeof(*(ptr))))
-#define put_unaligned(x,ptr) \
-	__put_unaligned((unsigned long)(x), (ptr), sizeof(*(ptr)))
-
 /*
  * This function doesn't actually exist.  The idea is that when
  * someone uses the macros below with an unsupported size (datatype),
@@ -76,46 +68,44 @@
 	ptr->x = val;
 }
 
-static inline unsigned long __get_unaligned(const void *ptr, size_t size)
-{
-	unsigned long val;
-	switch (size) {
-	case 1:
-		val = *(const __u8 *)ptr;
-		break;
-	case 2:
-		val = __uldw((const __u16 *)ptr);
-		break;
-	case 4:
-		val = __uldl((const __u32 *)ptr);
-		break;
-	case 8:
-		val = __uldq((const __u64 *)ptr);
-		break;
-	default:
-		bad_unaligned_access_length();
-	};
-	return val;
-}
-
-static inline void __put_unaligned(unsigned long val, void *ptr, size_t size)
-{
-	switch (size) {
-	case 1:
-		*(__u8 *)ptr = val;
-	        break;
-	case 2:
-		__ustw(val, (__u16 *)ptr);
-		break;
-	case 4:
-		__ustl(val, (__u32 *)ptr);
-		break;
-	case 8:
-		__ustq(val, (__u64 *)ptr);
-		break;
-	default:
-	    	bad_unaligned_access_length();
-	};
-}
+#define get_unaligned(ptr) ({				\
+	unsigned long val;				\
+	switch (sizeof(*(ptr))) {			\
+	case 1:						\
+		val = *(const __u8 *)(ptr);		\
+		break;					\
+	case 2:						\
+		val = __uldw((const __u16 *)(ptr));	\
+		break;					\
+	case 4:						\
+		val = __uldl((const __u32 *)(ptr));	\
+		break;					\
+	case 8:						\
+		val = __uldq((const __u64 *)(ptr));	\
+		break;					\
+	default:					\
+		bad_unaligned_access_length();		\
+	};						\
+	(__typeof__(*(ptr)))val;			\
+})
+
+#define put_unaligned(val, ptr) ({			\
+	switch (sizeof(*(ptr))) {			\
+	case 1:						\
+		*(__u8 *)(ptr) = (val);			\
+	        break;					\
+	case 2:						\
+		__ustw(val, (__u16 *)(ptr));		\
+		break;					\
+	case 4:						\
+		__ustl(val, (__u32 *)(ptr));		\
+		break;					\
+	case 8:						\
+		__ustq(val, (__u64 *)(ptr));		\
+		break;					\
+	default:					\
+	    	bad_unaligned_access_length();		\
+	};						\
+})
 
 #endif /* _ASM_GENERIC_UNALIGNED_H */
===== include/asm-parisc/unaligned.h 1.3 vs edited =====
--- 1.3/include/asm-parisc/unaligned.h	2005-03-17 15:54:10 -06:00
+++ edited/include/asm-parisc/unaligned.h	2005-04-03 16:14:55 -05:00
@@ -1,7 +1,7 @@
 #ifndef _ASM_PARISC_UNALIGNED_H_
 #define _ASM_PARISC_UNALIGNED_H_
 
-#include <asm-parisc/unaligned.h>
+#include <asm-generic/unaligned.h>
 
 #ifdef __KERNEL__
 struct pt_regs;

Re: Consolidation of asm/unaligned.h

[David S. Miller]

On Sun, 03 Apr 2005 22:42:41 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> I just got around to checking this on parisc, and I'm afraid we have a
> toolchain cockup:  Our gcc can't optimise the sizeof() if the user is an
> inline function.  It can, however if the functions are made #defines
> instead.
> 
> Would the attached be OK with everyone?  It works fine for us.

Please explain what you mean by "optimize"?

If you are saying what I think you're saying (that gcc always
emits the entire switch statement, not just the constant case
we need) I bet it has to do with inlining or switch statement
heuristics.

In any event, I'd like to be better informed, because this means
you're getting terrible code in a lot of places as this is
a common technique in the kernel header files.

I also don't want to make these macros, since if we do so we lose
the type checking and we also get into the world of CPP macro arg
multiple-evaluation crazyness.

Re: Consolidation of asm/unaligned.h

[James Bottomley]

On Sun, 2005-04-03 at 21:02 -0700, David S. Miller wrote:
> Please explain what you mean by "optimize"?
> 
> If you are saying what I think you're saying (that gcc always
> emits the entire switch statement, not just the constant case
> we need) I bet it has to do with inlining or switch statement
> heuristics.

Precisely.  Our current gcc doesn't optimise away the additional cases
even though sizeof(*ptr) should be a known constant.  One of our
compiler people said they'd take a look at it, but even if they manage
to fix it, we need a current compiler to be able to build the kernel.

> In any event, I'd like to be better informed, because this means
> you're getting terrible code in a lot of places as this is
> a common technique in the kernel header files.

That wouldn't surprise me.  However, all of our other uses of this
technique are in #defines, so this is the first time we've run into the
problem.

> I also don't want to make these macros, since if we do so we lose
> the type checking and we also get into the world of CPP macro arg
> multiple-evaluation crazyness.

Actually, Matthew Wilcox suggested an alternative:  If the inline
function arguments for the size are consts, then the compiler seems to
behave correctly, so how about the attached?

James

===== include/asm-generic/unaligned.h 1.2 vs edited =====
--- 1.2/include/asm-generic/unaligned.h	2005-03-17 15:54:10 -06:00
+++ edited/include/asm-generic/unaligned.h	2005-04-03 23:09:27 -05:00
@@ -76,7 +76,7 @@
 	ptr->x = val;
 }
 
-static inline unsigned long __get_unaligned(const void *ptr, size_t size)
+static inline unsigned long __get_unaligned(const void *ptr, const size_t size)
 {
 	unsigned long val;
 	switch (size) {
@@ -98,7 +98,7 @@
 	return val;
 }
 
-static inline void __put_unaligned(unsigned long val, void *ptr, size_t size)
+static inline void __put_unaligned(unsigned long val, void *ptr, const size_t size)
 {
 	switch (size) {
 	case 1:
===== include/asm-parisc/unaligned.h 1.3 vs edited =====
--- 1.3/include/asm-parisc/unaligned.h	2005-03-17 15:54:10 -06:00
+++ edited/include/asm-parisc/unaligned.h	2005-04-03 16:14:55 -05:00
@@ -1,7 +1,7 @@
 #ifndef _ASM_PARISC_UNALIGNED_H_
 #define _ASM_PARISC_UNALIGNED_H_
 
-#include <asm-parisc/unaligned.h>
+#include <asm-generic/unaligned.h>
 
 #ifdef __KERNEL__
 struct pt_regs;

Re: Consolidation of asm/unaligned.h

[David S. Miller]

On Sun, 03 Apr 2005 23:15:36 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> Actually, Matthew Wilcox suggested an alternative:  If the inline
> function arguments for the size are consts, then the compiler seems to
> behave correctly, so how about the attached?

That looks perfectly reasonable.

BTW, the second hunk isn't needed in Linus's current tree
which has that include fix already :-)

Re: Consolidation of asm/unaligned.h

[Ralf Baechle]

On Sun, Apr 03, 2005 at 10:42:41PM -0500, James Bottomley wrote:

> I just got around to checking this on parisc, and I'm afraid we have a
> toolchain cockup:  Our gcc can't optimise the sizeof() if the user is an
> inline function.  It can, however if the functions are made #defines
> instead.
> 
> Would the attached be OK with everyone?  It works fine for us.

This started happening occasionally with gcc 3.4 so it's not just PARISC
specific - just rather hard to hit:

  http://www.linux-mips.org/archives/linux-mips/2004-10/msg00039.html

  Ralf

Re: Consolidation of asm/unaligned.h

[Matthew Wilcox]

On Sun, Apr 03, 2005 at 11:15:36PM -0500, James Bottomley wrote:
> Actually, Matthew Wilcox suggested an alternative:  If the inline
> function arguments for the size are consts, then the compiler seems to
> behave correctly, so how about the attached?

I only tested it; the idea was Randolph Chung's.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: Consolidation of asm/unaligned.h

[James Bottomley]

On Mon, 2005-04-04 at 11:47 +0100, Ralf Baechle wrote:
> This started happening occasionally with gcc 3.4 so it's not just PARISC
> specific - just rather hard to hit:

Could you verify that the addition of the consts to the inline function
size arguments (previously posted patch) fixes this?  If so, I'll send
the patch on.

James

Re: Consolidation of asm/unaligned.h

[Ralf Baechle]

On Tue, Apr 05, 2005 at 09:10:54AM -0500, James Bottomley wrote:

> On Mon, 2005-04-04 at 11:47 +0100, Ralf Baechle wrote:
> > This started happening occasionally with gcc 3.4 so it's not just PARISC
> > specific - just rather hard to hit:
> 
> Could you verify that the addition of the consts to the inline function
> size arguments (previously posted patch) fixes this?  If so, I'll send
> the patch on.

I can't reproduce the mistake reported to me unfortunately.  The patch
looks good and doesn't create any new problems on MIPS, so please go ahead.

  Ralf

Re: Consolidation of asm/unaligned.h

[Matthew Wilcox]

On Tue, Apr 05, 2005 at 09:10:54AM -0500, James Bottomley wrote:
> On Mon, 2005-04-04 at 11:47 +0100, Ralf Baechle wrote:
> > This started happening occasionally with gcc 3.4 so it's not just PARISC
> > specific - just rather hard to hit:
> 
> Could you verify that the addition of the consts to the inline function
> size arguments (previously posted patch) fixes this?  If so, I'll send
> the patch on.

FWIW, adding consts doesn't help gcc-3.0.  I don't have a problem with
forcing people to upgrade from 3.0 to 3.3, I just wanted to be sure
everyone's aware of the problem.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: Consolidation of asm/unaligned.h

[Ralf Baechle]

On Tue, Apr 05, 2005 at 04:02:18PM +0100, Matthew Wilcox wrote:

> > Could you verify that the addition of the consts to the inline function
> > size arguments (previously posted patch) fixes this?  If so, I'll send
> > the patch on.
> 
> FWIW, adding consts doesn't help gcc-3.0.  I don't have a problem with
> forcing people to upgrade from 3.0 to 3.3, I just wanted to be sure
> everyone's aware of the problem.

3.0 has been a significant source of trouble on MIPS.  Fortunately 3.0
and 3.1 seems to have died out already so I don't need to force anybody :)

  Ralf