Skip to contentSkip to conversation

Modernize i386 string.h

PATCH In review Andi Kleen 6 participants 7 messages Jan 19, 2004 – Jan 20, 2004

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Modernize i386 string.h

This patch modernizes i386 string.h. It removes all the fragile i386 inline str* functions and just switches to gcc's builtin variants. Modern gcc should generate equivalent or better code. Sometimes it calls out-of-line code, in that case the standard C functions in lib/string.c is used.

Essentially it's porting the x86-64 string.h to i386.

memcpy and memset use the x86 string functions. They are always out-of-line now. These seem to do quite well on many modern x86 (C stepping K8, P3, P4). On some CPUs (Athlon XP) it may be better to use special unrolled versions for clear_page/copy_page, that is subject to further work. I removed the call to MMX memcpy from the main memcpy because MMX only helps for very big copies and normal memcpy is rarely used for that. Longer term it would
be useful to implement optimized clear_page/copy_page functions and switch to the correct one for the current CPU using the alternative() framework.

I also removed struct_cpy and just let the compiler expand it.

[see the big comment in arch/i386/lib/memcpy.c for more details and some considerations]

-Andi

arch/i386/lib/memmove.c new file +31 -0
diff -u /dev/null linux-string/arch/i386/lib/memmove.c
@@ -0,0 +1,31 @@
+#include <linux/module.h>
+#include <linux/string.h>
+
+/* Normally compiler builtins are used, but sometimes the compiler calls out
+   of line code. Based on asm-i386/string.h.
+
+   memmove is not that time critical, so this simple version should work.
+ */
+
+#undef memmove
+void *memmove(void * dest,const void *src,size_t count)
+{
+	if (dest < src) { 
+		__inline_memcpy(dest,src,count);
+	} else {
+		/* Could be more clever and move longs */
+		unsigned long d0, d1, d2;
+		__asm__ __volatile__(
+			"std\n\t"
+			"rep\n\t"
+			"movsb\n\t"
+			"cld"
+			: "=&c" (d0), "=&S" (d1), "=&D" (d2)
+			:"0" (count),
+			"1" (count-1+(const char *)src),
+			"2" (count-1+(char *)dest)
+			:"memory");
+	}
+	return dest;
+} 
+EXPORT_SYMBOL_NOVERS(memmove);
arch/i386/lib/memcpy.c renamed from arch/i386/lib/memcpy.c-STRING +47 -12

66 lines not on this page · raw message

arch/i386/lib/memset.c new file +27 -0
diff -u /dev/null linux-string/arch/i386/lib/memset.c
@@ -0,0 +1,27 @@
+#include <linux/module.h>
+#include <linux/string.h>
+
+/* Please see the introductionary comment in memcpy.c. It applies to 
+   memset too. */
+
+#undef memset
+void * memset(void * s, int c, size_t count)
+{
+	unsigned fill = (unsigned)c * 0x01010101; 
+	int d0, d1;
+	asm volatile(
+		"rep ; stosl\n\t"
+		"testl $2,%3\n\t"
+		"je 1f\n\t"
+		"stosw\n"
+		"1:\ttestl $1,%3\n\t"
+		"je 2f\n\t"
+		"stosb\n"
+		"2:"
+		: "=&c" (d0), "=&D" (d1)
+		: "a" (fill), "q" (count), "0" (count/4), "1" ((long) s)
+		: "memory");
+	return (s);     
+}
+
+EXPORT_SYMBOL_NOVERS(memset);
arch/i386/lib/strstr.c renamed from arch/i386/lib/strstr.c-STRING +0 -31
diff -u linux-string/arch/i386/lib/strstr.c-STRING /dev/null
@@ -1,31 +0,0 @@
-#include <linux/string.h>
-
-char * strstr(const char * cs,const char * ct)
-{
-int	d0, d1;
-register char * __res;
-__asm__ __volatile__(
-	"movl %6,%%edi\n\t"
-	"repne\n\t"
-	"scasb\n\t"
-	"notl %%ecx\n\t"
-	"decl %%ecx\n\t"	/* NOTE! This also sets Z if searchstring='' */
-	"movl %%ecx,%%edx\n"
-	"1:\tmovl %6,%%edi\n\t"
-	"movl %%esi,%%eax\n\t"
-	"movl %%edx,%%ecx\n\t"
-	"repe\n\t"
-	"cmpsb\n\t"
-	"je 2f\n\t"		/* also works for empty string, see above */
-	"xchgl %%eax,%%esi\n\t"
-	"incl %%esi\n\t"
-	"cmpb $0,-1(%%eax)\n\t"
-	"jne 1b\n\t"
-	"xorl %%eax,%%eax\n\t"
-	"2:"
-	:"=a" (__res), "=&c" (d0), "=&S" (d1)
-	:"0" (0), "1" (0xffffffff), "2" (cs), "g" (ct)
-	:"dx", "di");
-return __res;
-}
-
arch/i386/lib/Makefile renamed from arch/i386/lib/Makefile-STRING +1 -1
diff -u linux-string/arch/i386/lib/Makefile-STRING linux-string/arch/i386/lib/Makefile
@@ -5,7 +5,7 @@
 
 lib-y = checksum.o delay.o \
 	usercopy.o getuser.o \
-	memcpy.o strstr.o
+	memcpy.o memmove.o memset.o
 
 lib-$(CONFIG_X86_USE_3DNOW) += mmx.o
 lib-$(CONFIG_HAVE_DEC_LOCK) += dec_and_lock.o
arch/i386/kernel/i386_ksyms.c renamed from arch/i386/kernel/i386_ksyms.c-STRING +32 -7
diff -u linux-string/arch/i386/kernel/i386_ksyms.c-STRING linux-string/arch/i386/kernel/i386_ksyms.c
@@ -133,6 +133,38 @@
 EXPORT_SYMBOL(pcibios_get_irq_routing_table);
 #endif
 
+/* Export string functions. We normally rely on gcc builtin for most of these,
+   but gcc sometimes decides not to inline them. */    
+#undef memchr
+#undef strlen
+#undef strcpy
+#undef strncmp
+#undef strncpy
+#undef strchr	
+#undef strcmp 
+#undef strcpy 
+#undef strcat
+
+extern size_t strlen(const char *);
+extern char * strcpy(char * dest,const char *src);
+extern int strcmp(const char * cs,const char * ct);
+extern void *memchr(const void *s, int c, size_t n);
+extern char * strcat(char *, const char *);
+
+EXPORT_SYMBOL_NOVERS(strlen);
+EXPORT_SYMBOL_NOVERS(memmove);
+EXPORT_SYMBOL_NOVERS(strcpy);
+EXPORT_SYMBOL_NOVERS(strncmp);
+EXPORT_SYMBOL_NOVERS(strncpy);
+EXPORT_SYMBOL_NOVERS(strchr);
+EXPORT_SYMBOL_NOVERS(strcmp); 
+EXPORT_SYMBOL_NOVERS(strcat);
+EXPORT_SYMBOL_NOVERS(strncat);
+EXPORT_SYMBOL_NOVERS(memchr);
+EXPORT_SYMBOL_NOVERS(strrchr);
+EXPORT_SYMBOL_NOVERS(strnlen);
+EXPORT_SYMBOL_NOVERS(memscan);
+
 #ifdef CONFIG_X86_USE_3DNOW
 EXPORT_SYMBOL(_mmx_memcpy);
 EXPORT_SYMBOL(mmx_clear_page);
@@ -179,13 +211,6 @@
 EXPORT_SYMBOL_GPL(set_nmi_callback);
 EXPORT_SYMBOL_GPL(unset_nmi_callback);
  
-#undef memcpy
-#undef memset
-extern void * memset(void *,int,__kernel_size_t);
-extern void * memcpy(void *,const void *,__kernel_size_t);
-EXPORT_SYMBOL_NOVERS(memcpy);
-EXPORT_SYMBOL_NOVERS(memset);
-
 #ifdef CONFIG_HAVE_DEC_LOCK
 EXPORT_SYMBOL(atomic_dec_and_lock);
 #endif
arch/i386/kernel/process.c renamed from arch/i386/kernel/process.c-STRING +1 -1
diff -u linux-string/arch/i386/kernel/process.c-STRING linux-string/arch/i386/kernel/process.c
@@ -344,7 +344,7 @@
 	int err;
 
 	childregs = ((struct pt_regs *) (THREAD_SIZE + (unsigned long) p->thread_info)) - 1;
-	struct_cpy(childregs, regs);
+	*childregs = *regs;
 	childregs->eax = 0;
 	childregs->esp = esp;
 	p->set_child_tid = p->clear_child_tid = NULL;
include/asm-i386/string.h renamed from include/asm-i386/string.h-STRING +30 -445

498 lines not on this page · raw message

#1 lore raw
Quoted 5 lines · Andi Kleen

This patch modernizes i386 string.h. It removes all the fragile i386 inline str* functions and just switches to gcc's builtin variants. Modern gcc should generate equivalent or better code. Sometimes it calls out-of-line code, in that case the standard C functions in lib/string.c is used.

[...]

Quoted 14 lines

diff -u linux-string/arch/i386/kernel/i386_ksyms.c-STRING linux-string/arch/i386/kernel/i386_ksyms.c

arch/i386/kernel/i386_ksyms.c renamed from arch/i386/kernel/i386_ksyms.c-STRING +7 -0
@@ -133,6 +133,38 @@
 EXPORT_SYMBOL(pcibios_get_irq_routing_table);
 #endif
 
+/* Export string functions. We normally rely on gcc builtin for most of these,
+   but gcc sometimes decides not to inline them. */    
+#undef memchr
+#undef strlen
+#undef strcpy
+#undef strncmp
+#undef strncpy

arch/i386/kernel/i386_ksyms.c line 152

Quoted 10 lines
arch/i386/kernel/i386_ksyms.c renamed from arch/i386/kernel/i386_ksyms.c-STRING +10 -0
+#undef strchr	
+#undef strcmp 
+#undef strcpy 
+#undef strcat
+
+extern size_t strlen(const char *);
+extern char * strcpy(char * dest,const char *src);
+extern int strcmp(const char * cs,const char * ct);
+extern void *memchr(const void *s, int c, size_t n);
+extern char * strcat(char *, const char *);

ISTR this patch being shot down in the past...

It seems suboptimal for people with ancient compilers, or for people on embedded 486's, doesn't it?

Jeff

#2 lore raw
Quoted 34 lines · Jeff Garzik
Quoted 5 lines · Andi Kleen

This patch modernizes i386 string.h. It removes all the fragile i386 inline str* functions and just switches to gcc's builtin variants. Modern gcc should generate equivalent or better code. Sometimes it calls out-of-line code, in that case the standard C functions in lib/string.c is used.

[...]

Quoted 14 lines

diff -u linux-string/arch/i386/kernel/i386_ksyms.c-STRING linux-string/arch/i386/kernel/i386_ksyms.c

arch/i386/kernel/i386_ksyms.c renamed from arch/i386/kernel/i386_ksyms.c-STRING +7 -0
@@ -133,6 +133,38 @@
 EXPORT_SYMBOL(pcibios_get_irq_routing_table);
 #endif
 
+/* Export string functions. We normally rely on gcc builtin for most of these,
+   but gcc sometimes decides not to inline them. */
+#undef memchr
+#undef strlen
+#undef strcpy
+#undef strncmp
+#undef strncpy

arch/i386/kernel/i386_ksyms.c line 152

Quoted 10 lines
arch/i386/kernel/i386_ksyms.c renamed from arch/i386/kernel/i386_ksyms.c-STRING +10 -0
+#undef strchr
+#undef strcmp
+#undef strcpy
+#undef strcat
+
+extern size_t strlen(const char *);
+extern char * strcpy(char * dest,const char *src);
+extern int strcmp(const char * cs,const char * ct);
+extern void *memchr(const void *s, int c, size_t n);
+extern char * strcat(char *, const char *);

ISTR this patch being shot down in the past...

Quoted 5 lines

It seems suboptimal for people with ancient compilers, or for people on embedded 486's, doesn't it?

Jeff

I don't think so. It helps reduce the size of the code by having only one copy of each routine in memory rather than zillions.

If your code can't stand the overhead of the call/return, then you put some inline code at that particular place, not everywhere!

Cheers,
Dick Johnson
Penguin : Linux version 2.4.24 on an i686 machine (797.90 BogoMips).
Note 96.31% of all statistics are fiction.

#3 lore raw
In reply to Jeff Garzik
Quoted 1 line

ISTR this patch being shot down in the past...

I don't remember that.

Quoted 3 lines

It seems suboptimal for people with ancient compilers, or for people on embedded 486's, doesn't it?

It is unlikely to make much difference for them. Kernel str* handling is not very time critical (except memset/memcpy, but they are handled efficiently on all compilers) The patch is mainly to make the code more maintainable.

-Andi

#4 lore raw
In reply to Andi Kleen

include/asm-i386/string.h line 37

Quoted 10 lines · Andi Kleen
include/asm-i386/string.h renamed from include/asm-i386/string.h-STRING +10 -0
+#define __HAVE_ARCH_MEMCPY 1
+extern void *__memcpy(void *to, const void *from, size_t len); 
+#define memcpy(dst,src,len) \
+	({ size_t __len = (len);				\
+	   void *__ret;						\
+	   if (__builtin_constant_p(len) && __len >= 128)	\
+		 __ret = __memcpy((dst),(src),__len);		\
+	   else							\
+		 __ret = __builtin_memcpy((dst),(src),__len);	\
+	   __ret; }) 

Why not just __builtin_memcpy? Or indeed, why bother defining anything at all, since the compiler will infer __builtin_memcpy from the external symbol memcpy.

r~

#5 lore raw
Quoted 15 lines · Richard Henderson

include/asm-i386/string.h line 37

Quoted 10 lines · Andi Kleen
include/asm-i386/string.h renamed from include/asm-i386/string.h-STRING +10 -0
+#define __HAVE_ARCH_MEMCPY 1
+extern void *__memcpy(void *to, const void *from, size_t len); 
+#define memcpy(dst,src,len) \
+	({ size_t __len = (len);				\
+	   void *__ret;						\
+	   if (__builtin_constant_p(len) && __len >= 128)	\
+		 __ret = __memcpy((dst),(src),__len);		\
+	   else							\
+		 __ret = __builtin_memcpy((dst),(src),__len);	\
+	   __ret; }) 

Why not just __builtin_memcpy? Or indeed, why bother defining anything at all, since the compiler will infer __builtin_memcpy from the external symbol memcpy.

I was considering that when I moved the code over from x86-64.

On x86-64 (in gcc 3.1 timeframe) I did it originally this way because memcpy didn't have this "use external for big copies" logic. I wasn't sure if gcc 3.3 handles it now correctly. If you can confirm that it does I will happily remove it.

-Andi

#6 lore raw
In reply to Andi Kleen

In message <20040118200919.GA26573@averell> you write:

Quoted 1 line

+EXPORT_SYMBOL_NOVERS_NOVERS(memmove);

FYI: this has no meaningful semantic in 2.6 (we don't mangle symbols, but include the version in a separate section anyway).

Please use EXPORT_SYMBOL() everywhere in new code.

Thanks!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.