Kill __bzero()

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

Quoted 2 lines · Franck Bui-Huu

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().

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 <[email protected]>

---

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.

Quoted 15 lines

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);
}

Quoted 1 line

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 ?

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.

Quoted 15 lines · Franck Bui-Huu

Quoted 15 lines

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);
}

Quoted 7 lines

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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...

Quoted 6 lines

Quoted 4 lines

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().

Quoted 2 lines

__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...

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 ?

Quoted 7 lines · Franck Bui-Huu

Quoted 4 lines

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?

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

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().

Quoted 5 lines

Quoted 2 lines

__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.

Quoted 7 lines

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.