[PATCH] Kill __bzero()

[PATCH] Kill __bzero()

[Franck Bui-Huu]

This patch removes this function because:

  1/ Its unconventional prototype is error prone: its prototype is
  the same as memset one but was documented by mips_ksym.c like the
  following:

	   extern void *__bzero(void *__s, size_t __count);

  2/ For the caller, it makes no difference to call memset instead
  since it has to setup the second parameter of __bzero to 0.

  3/ It's not part of the Linux user access API, so no module can use
  it.

  4/ It needs to be exported with EXPORT_SYMBOL and therefore consumes
  some extra bytes.

  5/ It has only one user.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---

 I'm wondering if I'm missing something, because this function seems
 so ugly and useless in the first place, that I can't refrain to
 submit a patch to get rid of it.

		Franck

 arch/mips/kernel/mips_ksyms.c |    2 --
 arch/mips/lib/csum_partial.S  |    2 +-
 arch/mips/lib/memcpy.S        |    2 +-
 arch/mips/lib/memset.S        |    4 +---
 include/asm-mips/uaccess.h    |    2 +-
 5 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
index 225755d..6da9fa8 100644
--- a/arch/mips/kernel/mips_ksyms.c
+++ b/arch/mips/kernel/mips_ksyms.c
@@ -14,7 +14,6 @@
 #include <asm/pgtable.h>
 #include <asm/uaccess.h>
 
-extern void *__bzero(void *__s, size_t __count);
 extern long __strncpy_from_user_nocheck_asm(char *__to,
                                             const char *__from, long __len);
 extern long __strncpy_from_user_asm(char *__to, const char *__from,
@@ -38,7 +37,6 @@ EXPORT_SYMBOL(kernel_thread);
  */
 EXPORT_SYMBOL(__copy_user);
 EXPORT_SYMBOL(__copy_user_inatomic);
-EXPORT_SYMBOL(__bzero);
 EXPORT_SYMBOL(__strncpy_from_user_nocheck_asm);
 EXPORT_SYMBOL(__strncpy_from_user_asm);
 EXPORT_SYMBOL(__strlen_user_nocheck_asm);
diff --git a/arch/mips/lib/csum_partial.S b/arch/mips/lib/csum_partial.S
index c0a77fe..8d3fa1e 100644
--- a/arch/mips/lib/csum_partial.S
+++ b/arch/mips/lib/csum_partial.S
@@ -694,7 +694,7 @@ l_exc:
 	ADD	dst, t0			# compute start address in a1
 	SUB	dst, src
 	/*
-	 * Clear len bytes starting at dst.  Can't call __bzero because it
+	 * Clear len bytes starting at dst.  Can't call memset because it
 	 * might modify len.  An inefficient loop for these rare times...
 	 */
 	beqz	len, done
diff --git a/arch/mips/lib/memcpy.S b/arch/mips/lib/memcpy.S
index a526c62..425f2c3 100644
--- a/arch/mips/lib/memcpy.S
+++ b/arch/mips/lib/memcpy.S
@@ -443,7 +443,7 @@ l_exc:
 	ADD	dst, t0			# compute start address in a1
 	SUB	dst, src
 	/*
-	 * Clear len bytes starting at dst.  Can't call __bzero because it
+	 * Clear len bytes starting at dst.  Can't call memset because it
 	 * might modify len.  An inefficient loop for these rare times...
 	 */
 	beqz	len, done
diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index 3f8b8b3..a13248b 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -46,7 +46,7 @@
 	.endm
 
 /*
- * memset(void *s, int c, size_t n)
+ * void *memset(void *s, int c, size_t n)
  *
  * a0: start of area to clear
  * a1: char to fill with
@@ -68,8 +68,6 @@ LEAF(memset)
 #endif
 	or		a1, t1
 1:
-
-FEXPORT(__bzero)
 	sltiu		t0, a2, LONGSIZE	/* very small region? */
 	bnez		t0, small_memset
 	 andi		t0, a0, LONGMASK	/* aligned? */
diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index c30c718..9b8234a 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -643,7 +643,7 @@ __clear_user(void __user *addr, __kernel_size_t size)
 		"move\t$4, %1\n\t"
 		"move\t$5, $0\n\t"
 		"move\t$6, %2\n\t"
-		__MODULE_JAL(__bzero)
+		__MODULE_JAL(memset)
 		"move\t%0, $6"
 		: "=r" (res)
 		: "r" (addr), "r" (size)
-- 
1.5.3.4

Re: [PATCH] Kill __bzero()

[Atsushi Nemoto]

On Sun, 04 Nov 2007 09:18:32 +0100, Franck Bui-Huu <fbuihuu@gmail.com> wrote:
>   2/ For the caller, it makes no difference to call memset instead
>   since it has to setup the second parameter of __bzero to 0.

__bzero() does not clobber v0 but memset() does.  I don't know if gcc
does some magical thing to protect v0, but it would be safer to add v0
explicitly to clobber-list of __clear_user().

---
Atsushi Nemoto

Re: [PATCH] Kill __bzero()

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Sun, 04 Nov 2007 09:18:32 +0100, Franck Bui-Huu <fbuihuu@gmail.com> wrote:
>>   2/ For the caller, it makes no difference to call memset instead
>>   since it has to setup the second parameter of __bzero to 0.
> 
> __bzero() does not clobber v0 but memset() does.  I don't know if gcc
> does some magical thing to protect v0, but it would be safer to add v0
> explicitly to clobber-list of __clear_user().
> 

good catch ! I'll do it.

thanks
		Franck

Re: [PATCH] Kill __bzero()

[Ralf Baechle]

On Sun, Nov 04, 2007 at 09:18:32AM +0100, Franck Bui-Huu wrote:

>   1/ Its unconventional prototype is error prone: its prototype is
>   the same as memset one but was documented by mips_ksym.c like the
>   following:
> 
> 	   extern void *__bzero(void *__s, size_t __count);
> 
>   2/ For the caller, it makes no difference to call memset instead
>   since it has to setup the second parameter of __bzero to 0.
> 
>   3/ It's not part of the Linux user access API, so no module can use
>   it.
> 
>   4/ It needs to be exported with EXPORT_SYMBOL and therefore consumes
>   some extra bytes.
> 
>   5/ It has only one user.
> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
> 
>  I'm wondering if I'm missing something, because this function seems
>  so ugly and useless in the first place, that I can't refrain to
>  submit a patch to get rid of it.

Memset is almost always only ever invoked with a zero argument.  So the
idea was to have something like this:

extern void *__memset(void *__s, int __c, size_t __count);
extern void *bzero(void *__s, size_t __count);

static inline void *memset(void *s, int c, size_t count)
{
	if (__builtin_constant_p(c) && c == 0) {
		bzero(s, count);
		return s;
	} else
		return __memset(s, __c, count);
}

But that was never quite implemented like this as you noticed.

As for the differences in the return value, they're because of of
clear_user and __clear_user which return the number of bytes that could
_not_ be cleared in $a2.  Memset being invoked through the normal C calling
conventions ignores this value while it's the actual result of interest for
__clear_user.

I hope that explains things a little.

  Ralf

Re: [PATCH] Kill __bzero()

[Franck Bui-Huu]

Ralf Baechle wrote:
> 
> Memset is almost always only ever invoked with a zero argument.  So the
> idea was to have something like this:
> 
> extern void *__memset(void *__s, int __c, size_t __count);
> extern void *bzero(void *__s, size_t __count);
> 
> static inline void *memset(void *s, int c, size_t count)
> {
> 	if (__builtin_constant_p(c) && c == 0) {
> 		bzero(s, count);
> 		return s;
> 	} else
> 		return __memset(s, __c, count);
> }
> 
> But that was never quite implemented like this as you noticed.

Well I'm not sure we really need this. bzero() is not part of the
Linux string API, so it can only be used by MIPS specific code. And
with the current implementation of bzero(), $a1 needs to be setup to 0
anyway. That's why I simply killed it...

BTW, can memset() be an inlined function ?

> 
> As for the differences in the return value, they're because of of
> clear_user and __clear_user which return the number of bytes that could
> _not_ be cleared in $a2.  Memset being invoked through the normal C calling
> conventions ignores this value while it's the actual result of interest for
> __clear_user.
> 

Yes I noticed this. Actually I'm wondering if we couldn't add a new
function, fill_user() like the following:

extern size_t fill_user(void __user *to, int c, size_t len);

This could be used by both memset() and clear_user():

#define memset(s,c,l)	({ (void)fill(s,c,l); s; })
#define clear_user(t,l)	fill_user(t,0,l)

Therefore the definition of clear_user() could be saner.

What do you think ?

		Franck 

Re: [PATCH] Kill __bzero()

[Ralf Baechle]

On Mon, Nov 05, 2007 at 10:51:43PM +0100, Franck Bui-Huu wrote:

> > Memset is almost always only ever invoked with a zero argument.  So the
> > idea was to have something like this:
> > 
> > extern void *__memset(void *__s, int __c, size_t __count);
> > extern void *bzero(void *__s, size_t __count);
> > 
> > static inline void *memset(void *s, int c, size_t count)
> > {
> > 	if (__builtin_constant_p(c) && c == 0) {
> > 		bzero(s, count);
> > 		return s;
> > 	} else
> > 		return __memset(s, __c, count);
> > }
> > 
> > But that was never quite implemented like this as you noticed.
> 
> Well I'm not sure we really need this. bzero() is not part of the
> Linux string API, so it can only be used by MIPS specific code. And
> with the current implementation of bzero(), $a1 needs to be setup to 0
> anyway. That's why I simply killed it...
> 
> BTW, can memset() be an inlined function ?

It can be anything, macro, inline or outline function.  In the kernel
there are fewer restrictions than for a standards compliant library in
userspace.

You may take the i386 implementation in include/asm-x86/string_32.h as
an extreme example.

Older gcc used to generate significantly worse code for inline functions
than for macros so Linux became a fairly excessive user of macros.  This
has very much improved since, so these days inlines are prefered over
macros where possible.

> Yes I noticed this. Actually I'm wondering if we couldn't add a new
> function, fill_user() like the following:
> 
> extern size_t fill_user(void __user *to, int c, size_t len);

That's much better function name than the old __bzero - except that
__bzero effectivly took a long argument for the 2nd argument so 32-bit
on 32-bit kernels and 64-bit on 64-bit kernels.

> This could be used by both memset() and clear_user():
> 
> #define memset(s,c,l)	({ (void)fill(s,c,l); s; })
> #define clear_user(t,l)	fill_user(t,0,l)
> 
> Therefore the definition of clear_user() could be saner.

Looks alot nicer that way though an inline is probably preferable as
expressed above.

  Ralf

Re: [PATCH] Kill __bzero()

[Franck Bui-Huu]

Ralf Baechle wrote:
> On Mon, Nov 05, 2007 at 10:51:43PM +0100, Franck Bui-Huu wrote:
> 
>>> Memset is almost always only ever invoked with a zero argument.  So the
>>> idea was to have something like this:
>>>
>>> extern void *__memset(void *__s, int __c, size_t __count);
>>> extern void *bzero(void *__s, size_t __count);
>>>
>>> static inline void *memset(void *s, int c, size_t count)
>>> {
>>> 	if (__builtin_constant_p(c) && c == 0) {
>>> 		bzero(s, count);
>>> 		return s;
>>> 	} else
>>> 		return __memset(s, __c, count);
>>> }
>>>
>>> But that was never quite implemented like this as you noticed.
>> Well I'm not sure we really need this. bzero() is not part of the
>> Linux string API, so it can only be used by MIPS specific code. And
>> with the current implementation of bzero(), $a1 needs to be setup to 0
>> anyway. That's why I simply killed it...
>>
>> BTW, can memset() be an inlined function ?
> 
> It can be anything, macro, inline or outline function.  In the kernel
> there are fewer restrictions than for a standards compliant library in
> userspace.
> 
> You may take the i386 implementation in include/asm-x86/string_32.h as
> an extreme example.
> 
> Older gcc used to generate significantly worse code for inline functions
> than for macros so Linux became a fairly excessive user of macros.  This
> has very much improved since, so these days inlines are prefered over
> macros where possible.

Yes but ISTR that gcc generates some calls to memset and since
builtin functions are disabled the final link failed if memset
is inlined. I'll try to reproduce...

> 
>> Yes I noticed this. Actually I'm wondering if we couldn't add a new
>> function, fill_user() like the following:
>>
>> extern size_t fill_user(void __user *to, int c, size_t len);
> 
> That's much better function name than the old __bzero - except that

Actually I named it '__fill_user', since it doesn't call access_ok().

> __bzero effectivly took a long argument for the 2nd argument so 32-bit
> on 32-bit kernels and 64-bit on 64-bit kernels.

Isn't size_t meaning ?

Perhaps in this case __kernel_size_t is better...

> 
>> This could be used by both memset() and clear_user():
>>
>> #define memset(s,c,l)	({ (void)fill(s,c,l); s; })
>> #define clear_user(t,l)	fill_user(t,0,l)
>>
>> Therefore the definition of clear_user() could be saner.
> 
> Looks alot nicer that way though an inline is probably preferable as
> expressed above.
> 

Yes I have a patchset which clean up a bit uaccess.h and does this but
it's under construction.  It actually tries to convert all macros into
inlines and the file is much more readable and as a bonus side we could
easily add __must_check annotations which are currently missing.

I'll try to finish it this week but in the meantime can we just kill
__bzero or do you want me to include it in the future patchset ?

		Franck

Re: [PATCH] Kill __bzero()

[Ralf Baechle]

On Tue, Nov 06, 2007 at 08:42:48AM +0100, Franck Bui-Huu wrote:

> > Older gcc used to generate significantly worse code for inline functions
> > than for macros so Linux became a fairly excessive user of macros.  This
> > has very much improved since, so these days inlines are prefered over
> > macros where possible.
> 
> Yes but ISTR that gcc generates some calls to memset and since
> builtin functions are disabled the final link failed if memset
> is inlined. I'll try to reproduce...

So both belt and suspenders then that is an inline/macro plus an outline
version?

> >> Yes I noticed this. Actually I'm wondering if we couldn't add a new
> >> function, fill_user() like the following:
> >>
> >> extern size_t fill_user(void __user *to, int c, size_t len);
> > 
> > That's much better function name than the old __bzero - except that
> 
> Actually I named it '__fill_user', since it doesn't call access_ok().
> 
> > __bzero effectivly took a long argument for the 2nd argument so 32-bit
> > on 32-bit kernels and 64-bit on 64-bit kernels.
> 
> Isn't size_t meaning ?
> 
> Perhaps in this case __kernel_size_t is better...

I wrote about the existing __bzero which takes the size_t length as third 
argument and a long sized fill pattern as the second.

> Yes I have a patchset which clean up a bit uaccess.h and does this but
> it's under construction.  It actually tries to convert all macros into
> inlines and the file is much more readable and as a bonus side we could
> easily add __must_check annotations which are currently missing.
> 
> I'll try to finish it this week but in the meantime can we just kill
> __bzero or do you want me to include it in the future patchset ?

There is enough time until 2.6.25 to complete your cleanups; no more
cleanups for 2.6.24.

  Ralf

Re: [PATCH] Kill __bzero()

[Franck Bui-Huu]

Ralf Baechle wrote:
> On Tue, Nov 06, 2007 at 08:42:48AM +0100, Franck Bui-Huu wrote:
> 
>>> Older gcc used to generate significantly worse code for inline functions
>>> than for macros so Linux became a fairly excessive user of macros.  This
>>> has very much improved since, so these days inlines are prefered over
>>> macros where possible.
>> Yes but ISTR that gcc generates some calls to memset and since
>> builtin functions are disabled the final link failed if memset
>> is inlined. I'll try to reproduce...
> 

OK, using Cobalt config and the patch below, I get the following link
failure:

$ make

[snip]

  CC      init/version.o
  LD      init/built-in.o
  LD      .tmp_vmlinux1
kernel/built-in.o: In function `param_sysfs_init':
params.c:(.init.text+0x19ac): undefined reference to `memset'
params.c:(.init.text+0x19ac): relocation truncated to fit: R_MIPS_26 against `memset'
fs/built-in.o: In function `locks_remove_flock':
(.text+0x14518): undefined reference to `memset'
fs/built-in.o: In function `locks_remove_flock':
(.text+0x14518): relocation truncated to fit: R_MIPS_26 against `memset'
fs/built-in.o: In function `__blkdev_get':
block_dev.c:(.text+0x31d40): undefined reference to `memset'
block_dev.c:(.text+0x31d40): relocation truncated to fit: R_MIPS_26 against `memset'
block_dev.c:(.text+0x31d50): undefined reference to `memset'
block_dev.c:(.text+0x31d50): relocation truncated to fit: R_MIPS_26 against `memset'
make: *** [.tmp_vmlinux1] Error 1


> So both belt and suspenders then that is an inline/macro plus an outline
> version?
> 

I guess so.

>>>> Yes I noticed this. Actually I'm wondering if we couldn't add a new
>>>> function, fill_user() like the following:
>>>>
>>>> extern size_t fill_user(void __user *to, int c, size_t len);
>>> That's much better function name than the old __bzero - except that
>> Actually I named it '__fill_user', since it doesn't call access_ok().
>>
>>> __bzero effectivly took a long argument for the 2nd argument so 32-bit
>>> on 32-bit kernels and 64-bit on 64-bit kernels.
>> Isn't size_t meaning ?
>>
>> Perhaps in this case __kernel_size_t is better...
> 
> I wrote about the existing __bzero which takes the size_t length as third 
> argument and a long sized fill pattern as the second.
> 

Sorry I was confused (again) because of the invisible second parameter
of __bzero...

>> Yes I have a patchset which clean up a bit uaccess.h and does this but
>> it's under construction.  It actually tries to convert all macros into
>> inlines and the file is much more readable and as a bonus side we could
>> easily add __must_check annotations which are currently missing.
>>
>> I'll try to finish it this week but in the meantime can we just kill
>> __bzero or do you want me to include it in the future patchset ?
> 
> There is enough time until 2.6.25 to complete your cleanups; no more
> cleanups for 2.6.24.
> 

Sure.

		Franck
-- 8< --

diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index 3f8b8b3..23f0490 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -54,7 +54,7 @@
  */
 	.set	noreorder
 	.align	5
-LEAF(memset)
+LEAF(__memset)
 	beqz		a1, 1f
 	 move		v0, a0			/* result */
 
diff --git a/include/asm-mips/string.h b/include/asm-mips/string.h
index 436e3ad..b2ef64c 100644
--- a/include/asm-mips/string.h
+++ b/include/asm-mips/string.h
@@ -132,7 +132,12 @@ strncmp(__const__ char *__cs, __const__ char *__ct, size_t __count)
 #endif /* CONFIG_32BIT */
 
 #define __HAVE_ARCH_MEMSET
-extern void *memset(void *__s, int __c, size_t __count);
+static inline void *memset(void *__s, int __c, size_t __count)
+{
+	extern void *__memset(void *s, int c, size_t len);
+
+	return __memset(__s, __c, __count);
+}
 
 #define __HAVE_ARCH_MEMCPY
 extern void *memcpy(void *__to, __const__ void *__from, size_t __n);