Date: Sat, 5 May 2018 14:57:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: Mateusz Guzik <mjguzik@gmail.com>, Steven Hartland <steven.hartland@multiplay.co.uk>, Mateusz Guzik <mjg@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r333266 - head/sys/amd64/amd64 Message-ID: <20180505142152.W2917@besplex.bde.org> In-Reply-To: <CANCZdfpi4kqq5YwdgpQrvyrQg5KdEtmLLRbS9_2tMF_v-S9zYw@mail.gmail.com> References: <201805042241.w44MfC9E090893@repo.freebsd.org> <CAHEMsqbybV8%2BYO2rE8BqbTKspqf_xVGKcH_vKJE-b3NFmuQCDA@mail.gmail.com> <CAGudoHFDNLsMxKz9nY%2BvsOm66=7W0gd1-BBwi5ABEwOGiLvDbQ@mail.gmail.com> <CANCZdfpi4kqq5YwdgpQrvyrQg5KdEtmLLRbS9_2tMF_v-S9zYw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 4 May 2018, Warner Losh wrote: > On Fri, May 4, 2018 at 5:12 PM, Mateusz Guzik <mjguzik@gmail.com> wrote: > >> On Sat, May 5, 2018 at 12:58 AM, Steven Hartland < >> steven.hartland@multiplay.co.uk> wrote: >> >>> Can we get the why in commit messages please? >>> >>> This sort of message doesnt provide anything more that can be obtained >>> from reading the diff, which just leaves us wondering why? >>> >>> I=E2=80=99m sure there is a good reason, but without confirmation we=E2= =80=99re just left >>> guessing. The knock on to this is if some assumption that caused the wh= y >>> changes, anyone looking at this will not be able to make an informed >>> descision that that was the case. >>> >> bcopy is an equivalent of memmove, i.e. it accepts overlapping buffers. >> But if we know for a fact they don't overlap (like here), doing this ove= r >> memcpy (which does not accept such buffers) only puts avoidable >> constraints on the optimizer. Indeed, but clang already does adequate optimization for som manye cases (especially amd64), so these small changes are not much more than special micro-optimizations for gcc on 32-bit arches. I care about gcc and 32-bit arches, but you don't. > bcopy, in userland, is memmove. bcopy in the kernel has had a more > complicated history. Today it's more like memmove, but at times in the > history of BSD/Unix it's be more akin to memcpy with a companion ovbcopy > used for overlapping copies. FreeBSD has almost always been more in the I think (but don't know) that ovbcopy is a SYSVism and bcopy() always handled overlapping copies in BSD. It was not well documented that it did, but with only 1 memory-copying function that function has to handle overlapping copies or be even better documented to not handle them. > 'bcopy is memmove' rather than the 'bcopy is memcpy' though some of the > lower-tier architectures pulled in ovbcopy which we recently GC'd from > NetBSD and/or OpenBSD. In all of 4.4BSD /sys, ovbcopy is only referenced on 34 lines (almost half in tags files), mostly to implement it on some arches: - news3400, hp300, i386, luna68k: alias for bcopy - sparc64: separate from bcopy. bcopy seems to be like memcpy and doesn't handle overlapping copies. - vax/inline/machpats.c: separate and too vaxish for me to understand (seem= s to be just a prologue) - netiso/iso_pcb.c, net/slcompress.c, sparc/pmap.c. netinet/ip_output.c, netinet/ip_nroute.c: actually use it The sparc64 and vax code is an indication that bcopy didn't always handle overlapping copies in BSD. > Plus there's been an irrational encouragement of > using bcopy over mem* which has lead to the current state of affairs. You mean a rational encouragement. > For the vast majority of uses, it hasn't really mattered much in the past= =2E > It hasn't shown up on radar. It matters even less now. Deciding if the copies overlap takes about 1 branch, and with modern branch prediction that often costs about 1 cycle. The x86 library implementation wastes more like 50 cycles in other ways. > However, as its optimization has moved into > the compiler I'm guessing that's changed. It's that change of heart I thi= nk > that are taking people by surprise. I blame micro-benchmarks. Amdahls' law applies and gives a limit of about 1% for the possible improvements from optimizing bcopy(), except in micro-benchmarks. That is even though the kernel spends a relatively large amount of time in bcopy(). Userland might take 80% of the time, the kernel 20%, and bcopy() 10% of the 20% =3D 2%. After optimizing bcopy(= ) to be twice as fast (which is difficult), you have speeded up applications by 1% at most. Bruce From owner-svn-src-all@freebsd.org Sat May 5 05:31:14 2018 Return-Path: <owner-svn-src-all@freebsd.org> Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4A2DFFCAC03; Sat, 5 May 2018 05:31:14 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 672EF7F5F6; Sat, 5 May 2018 05:31:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 22CD8104FCFA; Sat, 5 May 2018 15:31:12 +1000 (AEST) Date: Sat, 5 May 2018 15:31:11 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik <mjg@freebsd.org> cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r333265 - head/sys/sys In-Reply-To: <201805042233.w44MXsC7089190@repo.freebsd.org> Message-ID: <20180505151622.Q3442@besplex.bde.org> References: <201805042233.w44MXsC7089190@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=y8u3x2SE7Zv_5vRtElYA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" <svn-src-all.freebsd.org> List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>, <mailto:svn-src-all-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>; List-Post: <mailto:svn-src-all@freebsd.org> List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help> List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>, <mailto:svn-src-all-request@freebsd.org?subject=subscribe> X-List-Received-Date: Sat, 05 May 2018 05:31:14 -0000 On Fri, 4 May 2018, Mateusz Guzik wrote: > Log: > Allow the compiler to use __builtin_memcpy > > In particular this allows the compiler to avoid heavy-handed machinery > if the to be copied buffer is small. > > Reviewed by: jhb Bugs in this: > Modified: head/sys/sys/systm.h > ============================================================================== > --- head/sys/sys/systm.h Fri May 4 21:17:29 2018 (r333264) > +++ head/sys/sys/systm.h Fri May 4 22:33:54 2018 (r333265) > @@ -275,6 +275,7 @@ void bzero(void * _Nonnull buf, size_t len); > void explicit_bzero(void * _Nonnull, size_t); > > void *memcpy(void * _Nonnull to, const void * _Nonnull from, size_t len); > +#define memcpy(to, from, len) __builtin_memcpy(to, from, len) - space instead of tab after #define. systm.h has rotted to have several other instances of this bug - space instead of tab between macro and macro expansion. This bug is less common. - args not parenthesized. I wonder how memcpy() handles attributes like _Nonnull. FreeBSD doesn't declare any attributes for builtins (is this possible?) so it gets the ones that the compiler defaults to, if any. I haven't noticed any documentation of these defaults even for compilers like gcc that have documentation. > void *memmove(void * _Nonnull dest, const void * _Nonnull src, size_t n); > > int copystr(const void * _Nonnull __restrict kfaddr, > > Modified: head/sys/sys/zutil.h > ============================================================================== > --- head/sys/sys/zutil.h Fri May 4 21:17:29 2018 (r333264) > +++ head/sys/sys/zutil.h Fri May 4 22:33:54 2018 (r333265) > @@ -32,7 +32,6 @@ > #include <sys/param.h> > #include <sys/kernel.h> > # define HAVE_MEMCPY > -# define memcpy(d, s, n) bcopy((s), (d), (n)) > # define memset(d, v, n) bzero((d), (n)) > # define memcmp bcmp Example of parenthesizing args in old code. Example of other style bugs for #define (spaces after '#' to indent). Reduction of portability here. I think zutil.h tries to provide its own wrappers for everything. It still does this for memset() although this is wrong when (n) != 0. I think you removed memcpy() since memcpy() is now implemented as an inconsistent macro, while memset() is still a function in systm.h so there is no conflict for memset(). Since there is no conflict, the bug is not even detected. memcmp() here is even more broken that memset(). memcmp() cannot be implemented using bcmp(), since it is tri-state while bcmp() is boolean. The system memcmp() had the same bug for a long time. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180505142152.W2917>