Re: svn commit: r333266 - head/sys/amd64/amd64
On Fri, 4 May 2018, Warner Losh wrote: On Fri, May 4, 2018 at 5:12 PM, Mateusz Guzik wrote: On Sat, May 5, 2018 at 12:58 AM, Steven Hartland < steven.hartl...@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???m sure there is a good reason, but without confirmation we???re just left guessing. The knock on to this is if some assumption that caused the why 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 over 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 (seems 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. 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 think 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% = 2%. After optimizing bcopy() to be twice as fast (which is difficult), you have speeded up applications by 1% at most. Bruce___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
On Fri, 4 May 2018, Warner Losh wrote:
While i wholeheartedly agree, an earlier commit explained why in this case.
It allows the compiler to inline for small copies.
[This should have been killfiled due to top-posting.]
No, it has nothing to do with that. The compiler can inline either bcopy()
or memcpy(), and does so after related recent changes that implement bcopy()
using builtins.
What this does is work around bugs in the recent changes, and compiler bugs.
bcopy() handles overlapping copies, but memcpy() doesn't, so it is not just
a style bug to use memcpy() instead of bcopy(). However, if bcopy() is
exposed to the compiler using builtins
(note that -ffreestanding prevents even memcpy() being known to
the compiler unless memcpy() is spelled __builtin_memcpy(), and
there are similar but larger complications for bcopy())
then the compiler can almost always see when overlap occurs and further
reduce bcopy() to __builtin_memcpy(). The full reduction should be:
bcopy() -> __builtin_memmove() -> __builtin_memcpy() -> { either
inline code or a call to __memcpy() }
but the actual reduction is:
bcopy() -> { __builtin_memmove() for small sizes, else bcopy() (bug1) }
-> { __builtin_memcpy() if inlined, else a call to memmove() (bugs2,3) },
else bcopy()
where bug1 is an implementation bug (not telling the compiler about large
copies), bug2 is a compiler bug (forgetting that it reduced to *memcpy when
calling the library to handle large sizes), and bug3 is a compiler bug
(calling the library function memmove() which might be unrelated in the
freestanding case).
Fixing the implementation and compiler would optimize thousands of calls
to bcopy() without churning the source code, but I'm a bit worried about
security bugs from this. The security bugs are larger for bzero(). When
bzero() was not exposed to the compiler as __builtin_memset(), it was
guaranteed to always zero memory so there was no need for explicit_bzero()
or explicit_memset() in the kernel, and using these was just a style bug,
as is using memset() to 0 instead of bzero(). Usually the compiler doesn't
have enough information to optimize away __builtin_memset(). But changing
thousands of bzero()s to __builtin_memset()s is likely to find a couple of
cases where the compiler does do this optimization and it breaks security.
Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
On Fri, May 4, 2018 at 5:12 PM, Mateusz Guzik wrote: > On Sat, May 5, 2018 at 12:58 AM, Steven Hartland < > steven.hartl...@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’m sure there is a good reason, but without confirmation we’re just left >> guessing. The knock on to this is if some assumption that caused the why >> 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 over > memcpy (which does not accept such buffers) only puts avoidable > constraints on the optimizer. > 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 '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. Plus there's been an irrational encouragement of using bcopy over mem* which has lead to the current state of affairs. For the vast majority of uses, it hasn't really mattered much in the past. It hasn't shown up on radar. However, as its optimization has moved into the compiler I'm guessing that's changed. It's that change of heart I think that are taking people by surprise. Warner ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
On 4 May 2018 at 16:12, Mateusz Guzik wrote: > On Sat, May 5, 2018 at 12:58 AM, Steven Hartland < > steven.hartl...@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’m sure there is a good reason, but without confirmation we’re just left >> guessing. The knock on to this is if some assumption that caused the why >> 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 over > memcpy (which does not accept such buffers) only puts avoidable > constraints on the optimizer. > > This is a rather pedestrian change which can be made in many places, > I don't see the point of repeating the explanation in each one. Although > I guess it would make sense to point at a specific commit which explains > things. > I feel like the second paragraph in particular suggests a methodical project (maybe in a branch?) to convert or mark all instances as not-to-be-converted based on their context and practical concerns (like performance) might be better than doing a bunch of one-off commits, with batched commits to -CURRENT from time-to-time. Then it's easy to say "Phase IV of bcopy analysis in kernel: convert to memcpy for all non-overlapping small copies" with a whole bunch of changes lumped together. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
[ Charset UTF-8 unsupported, converting... ]
> On Sat, May 5, 2018 at 12:58 AM, Steven Hartland <
> steven.hartl...@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?m sure there is a good reason, but without confirmation we?re just left
> > guessing. The knock on to this is if some assumption that caused the why
> > 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 over
> memcpy (which does not accept such buffers) only puts avoidable
> constraints on the optimizer.
>
> This is a rather pedestrian change which can be made in many places,
> I don't see the point of repeating the explanation in each one. Although
> I guess it would make sense to point at a specific commit which explains
> things.
Though this makes sense at the present time when one is reading
commits one after the other, in 2 years time when I am looking at
just the log file of just one of the effected files that context
is lost, so please document things in a way that the information
in the log is usefull, part of the purpose of the log is to help
us locate and to understand why something was done to the code.
It is really rather painful to be reading logs and have to do
30 svn diffs rX to see what the log entry is trying to
explain, and in this case even that does not tell why it was
done.
>
> > On Fri, 4 May 2018 at 23:41, Mateusz Guzik wrote:
> >
> >> Author: mjg
> >> Date: Fri May 4 22:41:12 2018
> >> New Revision: 333266
> >> URL: https://svnweb.freebsd.org/changeset/base/333266
> >>
> >> Log:
> >> amd64: syscall path bcopy -> memcpy
> >>
> >> Modified:
> >> head/sys/amd64/amd64/trap.c
> >>
> >> Modified: head/sys/amd64/amd64/trap.c
> >>
> >> ==
> >> --- head/sys/amd64/amd64/trap.c Fri May 4 22:33:54 2018(r333265)
> >> +++ head/sys/amd64/amd64/trap.c Fri May 4 22:41:12 2018(r333266)
> >> @@ -908,7 +908,7 @@ cpu_fetch_syscall_args(struct thread *td)
> >> error = 0;
> >> argp = &frame->tf_rdi;
> >> argp += reg;
> >> - bcopy(argp, sa->args, sizeof(sa->args[0]) * 6);
> >> + memcpy(sa->args, argp, sizeof(sa->args[0]) * 6);
> >> if (sa->narg > regcnt) {
> >> KASSERT(params != NULL, ("copyin args with no params!"));
> >> error = copyin(params, &sa->args[regcnt],
> >>
> >>
>
>
> --
> Mateusz Guzik
--
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
On Sat, May 5, 2018 at 12:58 AM, Steven Hartland <
steven.hartl...@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’m sure there is a good reason, but without confirmation we’re just left
> guessing. The knock on to this is if some assumption that caused the why
> 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 over
memcpy (which does not accept such buffers) only puts avoidable
constraints on the optimizer.
This is a rather pedestrian change which can be made in many places,
I don't see the point of repeating the explanation in each one. Although
I guess it would make sense to point at a specific commit which explains
things.
> On Fri, 4 May 2018 at 23:41, Mateusz Guzik wrote:
>
>> Author: mjg
>> Date: Fri May 4 22:41:12 2018
>> New Revision: 333266
>> URL: https://svnweb.freebsd.org/changeset/base/333266
>>
>> Log:
>> amd64: syscall path bcopy -> memcpy
>>
>> Modified:
>> head/sys/amd64/amd64/trap.c
>>
>> Modified: head/sys/amd64/amd64/trap.c
>>
>> ==
>> --- head/sys/amd64/amd64/trap.c Fri May 4 22:33:54 2018(r333265)
>> +++ head/sys/amd64/amd64/trap.c Fri May 4 22:41:12 2018(r333266)
>> @@ -908,7 +908,7 @@ cpu_fetch_syscall_args(struct thread *td)
>> error = 0;
>> argp = &frame->tf_rdi;
>> argp += reg;
>> - bcopy(argp, sa->args, sizeof(sa->args[0]) * 6);
>> + memcpy(sa->args, argp, sizeof(sa->args[0]) * 6);
>> if (sa->narg > regcnt) {
>> KASSERT(params != NULL, ("copyin args with no params!"));
>> error = copyin(params, &sa->args[regcnt],
>>
>>
--
Mateusz Guzik
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
While i wholeheartedly agree, an earlier commit explained why in this case.
It allows the compiler to inline for small copies.
Warner
On Fri, May 4, 2018, 4:58 PM Steven Hartland <
steven.hartl...@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’m sure there is a good reason, but without confirmation we’re just left
> guessing. The knock on to this is if some assumption that caused the why
> changes, anyone looking at this will not be able to make an informed
> descision that that was the case.
>
> On Fri, 4 May 2018 at 23:41, Mateusz Guzik wrote:
>
>> Author: mjg
>> Date: Fri May 4 22:41:12 2018
>> New Revision: 333266
>> URL: https://svnweb.freebsd.org/changeset/base/333266
>>
>> Log:
>> amd64: syscall path bcopy -> memcpy
>>
>> Modified:
>> head/sys/amd64/amd64/trap.c
>>
>> Modified: head/sys/amd64/amd64/trap.c
>>
>> ==
>> --- head/sys/amd64/amd64/trap.c Fri May 4 22:33:54 2018(r333265)
>> +++ head/sys/amd64/amd64/trap.c Fri May 4 22:41:12 2018(r333266)
>> @@ -908,7 +908,7 @@ cpu_fetch_syscall_args(struct thread *td)
>> error = 0;
>> argp = &frame->tf_rdi;
>> argp += reg;
>> - bcopy(argp, sa->args, sizeof(sa->args[0]) * 6);
>> + memcpy(sa->args, argp, sizeof(sa->args[0]) * 6);
>> if (sa->narg > regcnt) {
>> KASSERT(params != NULL, ("copyin args with no params!"));
>> error = copyin(params, &sa->args[regcnt],
>>
>>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r333266 - head/sys/amd64/amd64
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’m sure there is a good reason, but without confirmation we’re just left
guessing. The knock on to this is if some assumption that caused the why
changes, anyone looking at this will not be able to make an informed
descision that that was the case.
On Fri, 4 May 2018 at 23:41, Mateusz Guzik wrote:
> Author: mjg
> Date: Fri May 4 22:41:12 2018
> New Revision: 333266
> URL: https://svnweb.freebsd.org/changeset/base/333266
>
> Log:
> amd64: syscall path bcopy -> memcpy
>
> Modified:
> head/sys/amd64/amd64/trap.c
>
> Modified: head/sys/amd64/amd64/trap.c
>
> ==
> --- head/sys/amd64/amd64/trap.c Fri May 4 22:33:54 2018(r333265)
> +++ head/sys/amd64/amd64/trap.c Fri May 4 22:41:12 2018(r333266)
> @@ -908,7 +908,7 @@ cpu_fetch_syscall_args(struct thread *td)
> error = 0;
> argp = &frame->tf_rdi;
> argp += reg;
> - bcopy(argp, sa->args, sizeof(sa->args[0]) * 6);
> + memcpy(sa->args, argp, sizeof(sa->args[0]) * 6);
> if (sa->narg > regcnt) {
> KASSERT(params != NULL, ("copyin args with no params!"));
> error = copyin(params, &sa->args[regcnt],
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r333266 - head/sys/amd64/amd64
Author: mjg
Date: Fri May 4 22:41:12 2018
New Revision: 333266
URL: https://svnweb.freebsd.org/changeset/base/333266
Log:
amd64: syscall path bcopy -> memcpy
Modified:
head/sys/amd64/amd64/trap.c
Modified: head/sys/amd64/amd64/trap.c
==
--- head/sys/amd64/amd64/trap.c Fri May 4 22:33:54 2018(r333265)
+++ head/sys/amd64/amd64/trap.c Fri May 4 22:41:12 2018(r333266)
@@ -908,7 +908,7 @@ cpu_fetch_syscall_args(struct thread *td)
error = 0;
argp = &frame->tf_rdi;
argp += reg;
- bcopy(argp, sa->args, sizeof(sa->args[0]) * 6);
+ memcpy(sa->args, argp, sizeof(sa->args[0]) * 6);
if (sa->narg > regcnt) {
KASSERT(params != NULL, ("copyin args with no params!"));
error = copyin(params, &sa->args[regcnt],
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
