svn commit: r319507 - head/sys/fs/msdosfs

[Bruce Evans]

On Fri, 2 Jun 2017, Ed Maste wrote:

> Log:
>  msdosfs: use mem{cpy,move,set} instead of bcopy,bzero
>
>  This somewhat simplifies use of msdosfs code in userland (for makefs),
>  reduces diffs with NetBSD and is standard C as of C89.

This is a style bug in the kernel.  The kernel should use the standard BSD
functions bcopy() and bzero().

bzero() is better designed than memset() for zeroing.

bcopy() is essentially the same as memmove(), but is implemented more
optimally
   (the non-use of memmove() in the kernel used to be implememented by
   by not having it, but someone broke this by adding an memmove() wrapper
   which is an extern function that calls bcopy().  This is even slower than
   the inline wrappers used to break intentionally leaving out some of the
   other functions).

memcpy() became a non-style-bug in the kernel when dg optimized some
networking code by using it.  It was supposed to be used only for small
fixed-sized copies when the compiler can and does inlines it.  The extern
memcpy() unfortunately had to be implemented so that compiling with -O0
(which turns off inlining) is used, so that the code doesn't need ifdefs
for -O0.  This became nonsense when -ffreestanding killed inlining of
all Standard C functions.  The slow support function for -O0 is used in
all cases where memcpy() is used.  It turns out to be not very slow, at
least on x86.  On x86, it was never optimized like bcopy() was, but the
optimizations for bcopy() were very MD and are now not attempted, so
memcpy() is now just bcopy() with the support for overlapping copies
removed, so is slightly faster.  On newer x86, "rep movs[any]" is as
fast as anything for large copies, but has a large startup overhead so
is a bad method for small copies, but is used in all cases.  Compiler
inlines wouldn't use it for small copies, but the threshold for using it
is very MD so always using it is not too bad as a default.

memset() has a bad inline implementation.  It just calls bzero() when the
fill byte is zero.  Otherwise, it slowly stores 1 byte per iteration.
memset() is also extern in libkern with the same bad implementation, and
and it takes complicated macros to get the inline version.

memcmp() used to have a worse inline implementation that wrapped bcmp().
This was broken since bcmp() is boolean while memcmp() is 3-state.  This
has been fixed by using the extern libkern version which is a copy of
the generic libc version.  Except it is a bug for memcmp() to exist in
the kernel.

> Modified: head/sys/fs/msdosfs/msdosfs_conv.c
> ==============================================================================
> --- head/sys/fs/msdosfs/msdosfs_conv.c	Fri Jun  2 17:57:27 2017	(r319506)
> +++ head/sys/fs/msdosfs/msdosfs_conv.c	Fri Jun  2 18:39:53 2017	(r319507)
> @@ -536,7 +536,7 @@ unix2winfn(const u_char *un, size_t unlen, struct wine
> 	/*
> 	 * Initialize winentry to some useful default
> 	 */
> -	for (wcp = (uint8_t *)wep, i = sizeof(*wep); --i >= 0; *wcp++ = 0xff);
> +	memset(wep, 0xff, sizeof(*wep));
> 	wep->weCnt = cnt;
> 	wep->weAttributes = ATTR_WIN95;
> 	wep->weReserved1 = 0;

memset() uses the same slow loop as the old inline code.  Unfortunately,
there is no bset() function in the bcopy family, so the unusual case of
setting to nonzero had to be handled inline.

Bruce