Re: Removing dbregs
On 13.07.2018 23:32, Jason Thorpe wrote: > > >> On Jul 13, 2018, at 2:22 PM, Kamil Rytarowski wrote: >> >> On 13.07.2018 23:10, Jaromír Doleček wrote: >>> 2018-07-13 22:54 GMT+02:00 Kamil Rytarowski : I disagree with disabling it. The code is not broken, it's covered by tests, it's in use. >>> >>> This looks like perfect candidate for optional (default off) feature. >>> It is useless and dangerous for general purpose use by virtue of being >>> root only, but useful for specialized use. >>> >>> Honestly, I don't see any reason to have this on by default. > > A sysctl is kind of lame, because it can also result in a branch prediction > miss. > > Protect the sysctl in an #ifdef to enable being able to enable the feature? > This sysctl does not disable the feature in the kernel, it is designed to control the security extension value whether a user is allowed to set these registers. A user is still allowed to read them always. security.models.extensions.user_set_dbregs = 0 # <- here DB registers It's integrated into the security framework in the kernel that checks whether write operation is allowed. #ifdefing it out in a non-benchmarking application (I was checking ones that do something with syscalls) is more negligible than 0,3% of overhead in the kernel in a loop. However if there is a general consensus that this is a must-have and urgent, I can dedicate time for this.. CC: Christos / Alistair. signature.asc Description: OpenPGP digital signature
Re: Removing dbregs
> On Jul 13, 2018, at 2:22 PM, Kamil Rytarowski wrote: > > On 13.07.2018 23:10, Jaromír Doleček wrote: >> 2018-07-13 22:54 GMT+02:00 Kamil Rytarowski : >>> I disagree with disabling it. The code is not broken, it's covered by >>> tests, it's in use. >> >> This looks like perfect candidate for optional (default off) feature. >> It is useless and dangerous for general purpose use by virtue of being >> root only, but useful for specialized use. >> >> Honestly, I don't see any reason to have this on by default. A sysctl is kind of lame, because it can also result in a branch prediction miss. Protect the sysctl in an #ifdef to enable being able to enable the feature? >> >> Jaromir >> > > We have got this state now. It's a trade-off that we disable it for > user, and keep for root and those who use it (sysctl switch). > > I agree that there might be a microoptimization with not reloading DR, > but there is a bigger fish to fry at this point rather than winning 0,3% > of performance by killing a feature that is in use. > > If anything, the proper solution is to optimize the register switch. > -- thorpej
Re: Removing dbregs
On 13.07.2018 23:10, Jaromír Doleček wrote: > 2018-07-13 22:54 GMT+02:00 Kamil Rytarowski : >> I disagree with disabling it. The code is not broken, it's covered by >> tests, it's in use. > > This looks like perfect candidate for optional (default off) feature. > It is useless and dangerous for general purpose use by virtue of being > root only, but useful for specialized use. > > Honestly, I don't see any reason to have this on by default. > > Jaromir > We have got this state now. It's a trade-off that we disable it for user, and keep for root and those who use it (sysctl switch). I agree that there might be a microoptimization with not reloading DR, but there is a bigger fish to fry at this point rather than winning 0,3% of performance by killing a feature that is in use. If anything, the proper solution is to optimize the register switch. signature.asc Description: OpenPGP digital signature
Re: Removing dbregs
2018-07-13 22:54 GMT+02:00 Kamil Rytarowski : > I disagree with disabling it. The code is not broken, it's covered by > tests, it's in use. This looks like perfect candidate for optional (default off) feature. It is useless and dangerous for general purpose use by virtue of being root only, but useful for specialized use. Honestly, I don't see any reason to have this on by default. Jaromir
Re: Removing dbregs
On 13.07.2018 22:44, Maxime Villard wrote: > Le 13/07/2018 à 21:54, Kamil Rytarowski a écrit : >> On 13.07.2018 21:31, Maxime Villard wrote: >>> I don't like the dbregs code. We unconditionally write into %dr6 and >>> %dr7 in >>> userret, that is, during each interruptible kernel->user transition. >>> >>> Some measurements with PMCs show that the loads of dr6/dr7 are one of >>> the >>> first sources of recovery cycles during a build.sh - the cycles the CPU >>> spends >>> recovering from a mispredicted branch or a machine clear event. >>> >>> We lose many cycles down there. The code should have been written to >>> just >>> switch dbregs during context switches, and not on each transition. >>> >>> It could be fixed, but since the PopSS vuln, we actually disabled >>> dbregs by >>> default (they are now privileged), and I don't think we will ever >>> re-enable >>> them, given the risk, even though we do have a mitigation for PopSS. >>> >>> So why not remove the code? >> >> I strongly object to the removal because of the debugging purposes. > > For root only? There is nearly zero use case. You did write dbregs, and of > course you couldn't know that something like PopSS would come out. But now > that we have this bug, the interest of the feature has severely diminished, > to the point where it's just totally pointless. > I disagree. I keep using it. And for everybody there is still an option to read the registers (or rather their cache). The POP SS x86 design flaw is irrelevant to the functionality. >> Their functionality is possible to simulate to some extend and with >> significant performance overhead (like execution in the single step move) >> >> I was benchmarking the existing code with DTrace and the overhead was >> negligible (like: 0,05% DR6 + 0,23% DR7). > > You may have had these results with your test case, but the fact is that > reloading DR6/DR7 all the time for processes that don't even use dbregs > is cycle-consuming, and it must be avoided. > It's a micro-optimization at this point, unless someone is doing dummy context switch benchmarking. >> http://netbsd.org/~kamil/will-it-scale/open1_processes.svg >> >> I've initially started it with the FreeBSD-like approach to set them >> during cpu_switchto(9) but at that time I haven't solved mysterious >> panics. >> >> I still have some intermediate draft patch with the cpu_switch(9) >> implementation: >> >> http://netbsd.org/~kamil/dbg9.txt > > This code seems still wrong. You are reloading DR6/DR7 even if the process > was not using dbregs. We don't want to touch anything if we do a switch > from > a process that did not use dbregs to a process that did not use dbregs > either. > > During the PopSS thing I remember someone told me that several > generations of > CPUs had numerous bugs with DR6 and DR7, and that the erratas were a > headache. I don't have the email handy, but googling the thing, several > erratas indeed seem to exist for different dbregs bugs. > The only errata or buggy implementation I recall is not masking reserved bits. We do mask them. FreeBSD used to have this exposed and NetBSD is safe. > If you really want to keep the code, I suggest we disable it entirely > (via a > #ifdef), until the support is rewritten not to do reloads all the time. > This > can be done later, and won't reduce the functionality meanwhile (which is > already not there since it's privileged...). > > Maxime I disagree with disabling it. The code is not broken, it's covered by tests, it's in use. signature.asc Description: OpenPGP digital signature
Re: Removing dbregs
Le 13/07/2018 à 21:54, Kamil Rytarowski a écrit : On 13.07.2018 21:31, Maxime Villard wrote: I don't like the dbregs code. We unconditionally write into %dr6 and %dr7 in userret, that is, during each interruptible kernel->user transition. Some measurements with PMCs show that the loads of dr6/dr7 are one of the first sources of recovery cycles during a build.sh - the cycles the CPU spends recovering from a mispredicted branch or a machine clear event. We lose many cycles down there. The code should have been written to just switch dbregs during context switches, and not on each transition. It could be fixed, but since the PopSS vuln, we actually disabled dbregs by default (they are now privileged), and I don't think we will ever re-enable them, given the risk, even though we do have a mitigation for PopSS. So why not remove the code? I strongly object to the removal because of the debugging purposes. For root only? There is nearly zero use case. You did write dbregs, and of course you couldn't know that something like PopSS would come out. But now that we have this bug, the interest of the feature has severely diminished, to the point where it's just totally pointless. Their functionality is possible to simulate to some extend and with significant performance overhead (like execution in the single step move) I was benchmarking the existing code with DTrace and the overhead was negligible (like: 0,05% DR6 + 0,23% DR7). You may have had these results with your test case, but the fact is that reloading DR6/DR7 all the time for processes that don't even use dbregs is cycle-consuming, and it must be avoided. http://netbsd.org/~kamil/will-it-scale/open1_processes.svg I've initially started it with the FreeBSD-like approach to set them during cpu_switchto(9) but at that time I haven't solved mysterious panics. I still have some intermediate draft patch with the cpu_switch(9) implementation: http://netbsd.org/~kamil/dbg9.txt This code seems still wrong. You are reloading DR6/DR7 even if the process was not using dbregs. We don't want to touch anything if we do a switch from a process that did not use dbregs to a process that did not use dbregs either. During the PopSS thing I remember someone told me that several generations of CPUs had numerous bugs with DR6 and DR7, and that the erratas were a headache. I don't have the email handy, but googling the thing, several erratas indeed seem to exist for different dbregs bugs. If you really want to keep the code, I suggest we disable it entirely (via a #ifdef), until the support is rewritten not to do reloads all the time. This can be done later, and won't reduce the functionality meanwhile (which is already not there since it's privileged...). Maxime
Re: Removing dbregs
On 13.07.2018 21:31, Maxime Villard wrote: > I don't like the dbregs code. We unconditionally write into %dr6 and > %dr7 in > userret, that is, during each interruptible kernel->user transition. > > Some measurements with PMCs show that the loads of dr6/dr7 are one of the > first sources of recovery cycles during a build.sh - the cycles the CPU > spends > recovering from a mispredicted branch or a machine clear event. > > We lose many cycles down there. The code should have been written to just > switch dbregs during context switches, and not on each transition. > > It could be fixed, but since the PopSS vuln, we actually disabled dbregs by > default (they are now privileged), and I don't think we will ever re-enable > them, given the risk, even though we do have a mitigation for PopSS. > > So why not remove the code? I strongly object to the removal because of the debugging purposes. Their functionality is possible to simulate to some extend and with significant performance overhead (like execution in the single step move) I was benchmarking the existing code with DTrace and the overhead was negligible (like: 0,05% DR6 + 0,23% DR7). http://netbsd.org/~kamil/will-it-scale/open1_processes.svg I've initially started it with the FreeBSD-like approach to set them during cpu_switchto(9) but at that time I haven't solved mysterious panics. I still have some intermediate draft patch with the cpu_switch(9) implementation: http://netbsd.org/~kamil/dbg9.txt Feel free to optimize it, without the loss in functionality. signature.asc Description: OpenPGP digital signature
Re: Removing dbregs
> On Jul 13, 2018, at 12:31 PM, Maxime Villard wrote: > > So why not remove the code? No objection from me. -- thorpej
Removing dbregs
I don't like the dbregs code. We unconditionally write into %dr6 and %dr7 in userret, that is, during each interruptible kernel->user transition. Some measurements with PMCs show that the loads of dr6/dr7 are one of the first sources of recovery cycles during a build.sh - the cycles the CPU spends recovering from a mispredicted branch or a machine clear event. We lose many cycles down there. The code should have been written to just switch dbregs during context switches, and not on each transition. It could be fixed, but since the PopSS vuln, we actually disabled dbregs by default (they are now privileged), and I don't think we will ever re-enable them, given the risk, even though we do have a mitigation for PopSS. So why not remove the code?
Re: 8.0 performance issue when running build.sh?
On Fri, Jul 13, 2018 at 06:50:02AM -0400, Thor Lancelot Simon wrote: > Only the master does much if any disk (actually SSD) I/O. It must be > either the mpt driver or the scsi subsystem. At a _guess_, mpt? I > don't have time to look right now. I couldn't spot any special dma map allocations in mpt. > This is annoying, but seems unlikely to be the cause of the performance > regression; I agree, it is just .. strange. > I don't think the master runs builds any more, does it? So > it shouldn't be doing much and should have tons of CPU and memory > bandwidth available to move that data. It does run builds (but only a single instance). spz compared dmesg output with the old kernel and there was nothing obvious in there either. Martin
Re: 8.0 performance issue when running build.sh?
On Fri, Jul 13, 2018 at 09:37:26AM +0200, Martin Husemann wrote: > > Do you happen to know why > > vmstat -e | fgrep "bus_dma bounces" > > shows a > 500 rate e.g. on b45? I never see a single bounce on any of my > amd64 machines. The build slaves seem to do only a few of them though. Only the master does much if any disk (actually SSD) I/O. It must be either the mpt driver or the scsi subsystem. At a _guess_, mpt? I don't have time to look right now. This is annoying, but seems unlikely to be the cause of the performance regression; I don't think the master runs builds any more, does it? So it shouldn't be doing much and should have tons of CPU and memory bandwidth available to move that data. -- Thor Lancelot Simont...@panix.com "The two most common variations translate as follows: illegitimi non carborundum = the unlawful are not silicon carbide illegitimis non carborundum = the unlawful don't have silicon carbide."
Re: 8.0 performance issue when running build.sh?
On Fri, 13 Jul 2018, Martin Husemann wrote: On Fri, Jul 13, 2018 at 05:55:17AM +0800, Paul Goyette wrote: I have a system with (probably) enough RAM - amd64, 8/16 core/threads, with 128GB, running 8.99.18 - to test if someone wants to provide an explicit test scenario that can run on top of an existing "production" environment. If your production environment is based on -7, it might work. Steps to reproduce (I hope, as I couldn't): - run a netbsd-7 system Ooops - that would be a show-stopper. My one-and-only environment is 8.99.18 and I have no other bootable system (and no painless way to create one). - check out netbsd-7 src and xsrc - run something (with proper -j) : build.sh -m alpha -j N build release iso-image" - save the last ~20 lines of the log somewhere - clean up all build results, tools, obj, (if on tmpfs this is not needed) - reboot to a netbsd-8 kernel - (if on tmpfs, redo the checkout) - run the same build.sh again +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org | +--+--++
Re: 8.0 performance issue when running build.sh?
On Fri, Jul 13, 2018 at 05:55:17AM +0800, Paul Goyette wrote: > I have a system with (probably) enough RAM - amd64, 8/16 core/threads, > with 128GB, running 8.99.18 - to test if someone wants to provide an > explicit test scenario that can run on top of an existing "production" > environment. If your production environment is based on -7, it might work. Steps to reproduce (I hope, as I couldn't): - run a netbsd-7 system - check out netbsd-7 src and xsrc - run something (with proper -j) : build.sh -m alpha -j N build release iso-image" - save the last ~20 lines of the log somewhere - clean up all build results, tools, obj, (if on tmpfs this is not needed) - reboot to a netbsd-8 kernel - (if on tmpfs, redo the checkout) - run the same build.sh again Martin
Re: 8.0 performance issue when running build.sh?
On Fri, Jul 13, 2018 at 04:18:39AM +0700, Robert Elz wrote: > But it is only really serious on the netbsd-7(-*) builds, -6 -8 and HEAD > are not affected (or not nearly as much). They are, but only by ~20%, while the -7 builds are > 400%. My test builds on local hardware show +/-3% (I'd call that noise). And they are not always slower when running -8. Martin
Re: 8.0 performance issue when running build.sh?
On Thu, Jul 12, 2018 at 03:18:08PM -0400, Thor Lancelot Simon wrote: > On Thu, Jul 12, 2018 at 07:39:10PM +0200, Martin Husemann wrote: > > On Thu, Jul 12, 2018 at 12:30:39PM -0400, Thor Lancelot Simon wrote: > > > Are you running the builds from tmpfs to tmpfs, like the build cluster > > > does? > > > > No, I don't have enough ram to test it that way. > > So if 8.0 has a serious tmpfs regression... we don't yet know. I *did* use tempfs, but not for everything. Do you happen to know why vmstat -e | fgrep "bus_dma bounces" shows a > 500 rate e.g. on b45? I never see a single bounce on any of my amd64 machines. The build slaves seem to do only a few of them though. Martin
