Hi David, Thank you for looking at this issue.
The intend has never been to provide a command to easily crash the JVM :-) The patch was provided without any safety measures in purpose: to enable explorations in order to see how badly things could go south. The final design must include a per-command flag or method specifying if the command can be executed inside a safepoint. This information has to be provided by JVM developers, as there’s no automatic way to determine if a command is safepoint safe or not. So far, we already have the following rules to decide if a command can be executed at a safepoint or not: - it must not perform Java callbacks - it must not take locks - it must not invoke VMOp relying on the execution of their prologue and epilogue Let’s see: 1 - if there are more restrictions to consider 2 - what can be achieved with respect of these restrictions (which commands can run at a safepoint, which commands can easily be modified to comply with the restrictions above) Regards, Fred > On Jun 17, 2018, at 22:08, David Holmes <david.hol...@oracle.com> wrote: > > Hi Fred, > > Thanks for this and the analysis below. > > My 2c is that it is far too risky to provide a general purpose facility like > this where arbitrary command combinations can result in crashing the target > VM! > > Certainly no command that requires a JavaThread should be allowed to be > requested - only commands actually implemented via safepointing VMOps should > be considered. And then we should perhaps constrain things further so that no > commands that require prolog or epilog logic are accepted. > > Turning things around, perhaps we should flag which Dcmds, that operate at a > safepoint, are considered "combinable" and only allow those commands to be > specified? > > If we want a general purpose way of expressing a list of Dcmds to execute > then perhaps it should be specified to process each command in turn, with a > sequence of combinable safepoint ops processed at a single safepoint? > > Thanks, > David > > On 16/06/2018 1:32 AM, Frederic Parain wrote: >> Hi, >> Here’s a first prototype to explore running multiple commands in a single >> safepoint: >> http://cr.openjdk.java.net/~fparain/DCmdRunAtSafepoint/webrev.00/ >> The syntax is very simple, with the character ‘+’ used as the delimiter >> between commands. >> $ ./build/macosx-x64-debug/images/jdk/bin/jcmd 21164 VM.run_at_safepoint >> VM.version + VM.uptime >> 21164: >> Java HotSpot(TM) 64-Bit Server VM version >> 11-internal+0-2018-05-25-2202478.fparain.DCmd >> JDK 11.0.0 >> 44.143 s >> Playing a bit with it, it quickly shows what are the issues when trying to >> run >> diagnostic commands at a safepoint. >> Issue 1: One important point to remember is that only the VMThread can run >> at a safepoint, >> all JavaThreads are strictly forbidden to run during the safepoint. So >> executing commands at a >> safepoint means they will be executed by the VMThread. Unfortunately, the >> VMThread is >> subject to restrictions and cannot execute all code that a JavaThread can >> execute. This >> issue is visible with the VM.info command, which takes the Heap_lock. The >> VMThread >> is not allowed to grab this lock because it could cause the VMThread to be >> blocked by >> a JavaThread. So far, VM.info cannot be run at a safepoint. >> Issue 2: Executing a code at a safepoint requires a VMOperation. >> VMOperations are >> made of a prologue, a doit() method witch contains the code executed at the >> safepoint, >> and an epilogue. The prologue and the epilogue are executed by the requester >> of the >> VMOperation, usually a JavaThread, and they are not executed at a safepoint. >> Prologues >> and epilogues are designed to execute code the VMThread cannot execute, like >> taking >> locks or loading classes. When the VMThread itself wants to execute a >> VMOperation, >> what is called a nested VMOperation, it only executes the doit() method, >> skipping the >> prologue and the epilogue. This is an issue for diagnostic commands using a >> VMOp >> in their implementation: when these commands are called while the JVM is >> already >> at a safepoint, their prologue and epilogue cannot be executed. This issue >> impacts >> Thread.print and GC.run. >> There are probably other issues still to be found, so I encourage people to >> test >> this patch to see if the combinations of commands they would like to run at a >> safepoint can indeed be run in this mode. >> Note that the support for invoking the new command from the DiagnosticCommand >> MBean has not be implemented. This is a not negligible work in order to >> correctly >> check Java Permissions before executing the commands. >> Let me know if you consider this code useful or if the limitations described >> above >> make this feature not as pretty as we wanted it to be. >> Regards, >> Fred >>> On May 29, 2018, at 15:54, Frederic Parain <frederic.par...@oracle.com> >>> wrote: >>> >>> Hi Thomas, >>> >>> >>>> On May 26, 2018, at 04:07, Thomas Stüfe <thomas.stu...@gmail.com> wrote: >>>> >>> <snip> >>>>> >>>>> I think that it is a fundamental piece of the design. Adding a new >>>>> diagnostic >>>>> command to group several commands under a unique safepoint: >>>>> 1 - requires explicit action from the user (and acknowledgment of the >>>>> risks) >>>>> 2 - doesn’t touch the current syntax/semantic at all >>>>> >>>>> Another reason I’m pushing for a specific diagnostic command rather than >>>>> changing the syntax is remote execution. jcmd is not the only way to >>>>> invoke >>>>> a diagnostic command, they can also be invoke remotely using JMX and the >>>>> com.sun.management.DiagnosticCommand dynamic MBean. Through the >>>>> platform MBean, users do not have access to the jcmd syntax, they see >>>>> each diagnostic command as a MBean operation (providing that they have >>>>> been registered with the DCmd_Source_MBean flag). The initial support >>>>> for remote execution was taking a single String and processed it just like >>>>> a string passed by jcmd, but people developing consoles for remote >>>>> management complained that this was not convenient for them, and asked >>>>> for something they could use more programmatically. >>>>> If the safepoint bundling is implemented in the syntax, JMX clients won’t >>>>> be able to use the feature. If we implement it with a new diagnostic >>>>> command, >>>>> it will be visible from the MBean and they will be able to use it. >>>> >>>> That is interesting. Do you have examples for such programs? I was >>>> looking for ways of how to use jcmd (or equivalent) remotely. >>> >>> If you’re looking for a console application to invoke diagnostic command >>> on a remote JVM, Mission Control has a dedicated support for that in the >>> “Diagnostic Commands” tab. >>> If you’re looking for exemples of source code doing remote invocations, >>> there’re several unit tests for the DiagnosticCommand MBean in the directory >>> test/jdk/com/sun/management/DiagnosticCommandMBean. >>> >>>> >>>>> >>>>> At last, creating a specific diagnostic command for safepoint bundling >>>>> would >>>>> show the way on how to group commands for consistency. Safepoints is not >>>>> the only way to get consistency. For some JVM internals reasons, you might >>>>> want to group commands under a single lock, like the Threads_lock. Adding >>>>> a diagnostic command for each type of bundling seems more extensible. >>>>> >>>> >>>> You make good points. >>>> >>>> Adding a new command has also the advantage that it does not affect >>>> downward compatibility and hence would not require a CSR. CSR seems to >>>> slow down things a bit (I have a very simple CSR for jcmd open >>>> undecided since 12 days: >>>> https://bugs.openjdk.java.net/browse/JDK-8203043). So, while I think >>>> CSR are a necessary process, I am happy to avoid them if possible. >>>> >>>> Lets spin this out a bit more: >>>> >>>> -> However the user specifies "please execute these commands at a >>>> single safepoint", if one of the enclosed commands cannot be run >>>> inside a safepoint the whole bundle should fail, agreed? So, >>>> specifying e.g. JFR.start as part of such a command bundle would cause >>>> the whole bundle to not being executed. >>> >>> Yes. The new command N would ask for the execution of a list of >>> commands { A, B, C, … }, and would fail if: >>> - any of the specified commands { A, B, C, … } is not a registered >>> command >>> - any of the specified commands { A, B, C, … } doesn’t support >>> execution at safepoint >>> - any of the specified commands { A, B, C, … } is not exported to >>> the DCmdSource used to invoke N (*) >>> >>> (*) Not all commands are exported to all interfaces (jcmd,JMX), for >>> instance, some commands related to the JMX agent are cannot >>> be invoked through the JMX. Even if the new command can be >>> invoked from all interfaces, it must not allow to circumvent this >>> restriction. This should not be an issue today, because all >>> JMX agent commands are calling back Java code, and are not >>> suitable for execution at a safepoint. But this would ensure the >>> consistency of the model in the long term (and the test is cheap). >>> >>>> >>>> -> If we introduce a command like you propose (lets call it "VM.run" >>>> for the sake of the argument) we still need to decide how to delimit >>>> the commands at the command line. That brings us back to the syntax >>>> question: separation by a explicit delimiter, if possible one that >>>> does not clash with common unix shells? >>>> >>>> Lets try colon ':' : >>>> >>>> jcmd VM.run VM.uptime : VM.metaspace show-loaders show-classes >>>> by-chunktype : VM.classloaders show-classes : VM.info >>>> >>>> Of course, we can wrap all arguments to jcmd in quotes, then the >>>> delimiter sign does not affect things as much, unless we use quotes :) >>>> We could e.g. use semicolon: >>>> >>>> jcmd ' VM.run VM.uptime ; VM.metaspace show-loaders show-classes >>>> by-chunktype ; VM.classloaders show-classes ; VM.info ‘ >>>> >>> >>> I thought about colon and semi-colon too, because they are the most >>> obvious delimiters. However, they are already as delimiters in classpaths >>> (semi-colon on Unix, colon on Windows), so parsing to find the right >>> delimiters would be tricky. >>> >>> I’d start with ‘!’ or ‘+’ in a first time, to make some progress on the >>> other >>> aspects of the command, and buy some time to discuss the delimiter >>> issue in parallel. >>> >>> The last resort would be to force each command in the bundle to >>> be specified with quotes (arguments included). But it would make the >>> syntax very heavy, I’d prefer a simpler delimiter. >>> >>> >>>> Or, we go back to my original idea of letting the command keyword be >>>> the indicator for a new command start. Honestly, at this point I have >>>> no preference. Both approaches seem reasonable. >>>> >>>> The only niggle I have with an explicit command is the syntax, which I >>>> find a bit awkward. My thinking before reading your response was that >>>> I'd prefer a jcmd flag to specify the same behavior: that all commands >>>> should be executed at a single safepoint. E.g. [-s/--stacked] >>>> >>>> jcmd -s ' VM.uptime ; VM.metaspace show-loaders show-classes >>>> by-chunktype ; VM.classloaders show-classes ; VM.info ' >>>> >>>> But I see the disadvantage: depending on how it is implemented this >>>> would require consistent changes in both jcmd and hotspot, and not >>>> work with other clients using MBeans. Whereas the hypothetical >>>> "VM.run" would work out of the box for any old jcmd version and any >>>> other client. >>>> >>>> Of course one could keep the "VM.run" idea and still give jcmd a >>>> "-s/--stacked" option, as a shallow convenience feature, which just >>>> translates to "VM.run" and runs that. >>>> >>>> -> A variation of your "have special commands to run a command bundle >>>> in one particular context" theme could be having only one command, >>>> e.g. "VM.run", specifying the conditions under which to run the bundle >>>> as sub options. E.g. "VM.run -s <commands>" for "run commands at >>>> safepoint", "VM.run -t <commands>" for "run commands under >>>> Threads_lock". That would even allow to combine these contexts. I do >>>> not know if that makes sense. >>>> >>>> - Another advantage of having an explicit command just occurring to me >>>> is that it enables more evolved command files, where one can >>>> intersperse command bundles and single commands. For instance, I could >>>> give this to a customer: >>>> >>>> <command file> >>>> 1 VM.uptime >>>> 2 VM.run_commands VM.metaspace show-loaders show-classes by-chunktype >>>> VM.classloaders details VM.classloader_stats >>>> 3 GC.run >>>> 4 VM.run_commands VM.metaspace show-loaders show-classes by-chunktype >>>> VM.classloaders details VM.classloader_stats >>>> >>>> to execute a command bundle before and after triggering a GC. >>> >>> This would work fine . The parser of the jcmd string will split the >>> string based on ‘\n’ characters, then each line of command will be executed >>> sequentially, and lines containing the VM.run_command will then create the >>> safepoint and trigger the execution of the different commands specified in >>> its argument list. >>> >>>> >>>>> We have a three days week-end in the US, but I should be able to code >>>>> something next week. >>>>> >>>> >>>> Oh, you want to take over the implementation, sure! Fine by me. >>> >>> I’m not promising an implementation, I haven’t worked on this code >>> since I left the serviceability team. I need to code at least a skeleton >>> of the command to refresh my memory about all aspects of the >>> diagnostic command framework. >>> >>>> >>>>> Thank you for your ideas and your interest in diagnostic commands. >>>> >>>> Sure. Why I am interested in this: We strongly rely on tools similar >>>> to jcmd for our support. We have quite capable tools in our (licensed, >>>> non-open) VM port, which are similar to jcmd in many aspects. So we >>>> try to bring some of the capabilities upstream, in order to ease the >>>> merge costs for us and to help the OpenJDK community. >>> >>> The diagnostic command framework was designed to support an >>> extensible set of commands. Feel free to extend, and thank you >>> for contributing to the OpenJDK community. >>> >>> Fred >>> >>>>> >>>>> >>>>>> >>>>>>> >>>>>>> A new separator, more convenient than newline, would be require to have >>>>>>> a single line syntax. >>>>>>> >>>>>>> My 2 cents, >>>>>>> >>>>>> >>>>>> Certainly worth more than 2 cents :) Thanks for your thoughts! >>>>>> >>>>>> ..Thomas >>>>>> >>>>>>> Fred >>>>>>> >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Erik >>>>>>>>> >>>>>>>>> Hi Thomas, >>>>>>>>> >>>>>>>>> very positive suggestion. >>>>>>>>> >>>>>>>>> One observation: >>>>>>>>> There already seems to be some different interpretation of the command >>>>>>>>> line >>>>>>>>> in different Java versions: >>>>>>>>> For instance when I try to dump a flight recording in >>>>>>>>> 1.8.0_152-ea-b05 : >>>>>>>>> I >>>>>>>>> can use: >>>>>>>>> jcmd 33014 JFR.dump filename="recording1.jfr" recording=1 >>>>>>>>> but in build 9+181 , the same command must be: >>>>>>>>> jcmd 33014 JFR.dump filename="recording1.jfr",recording=1 >>>>>>>>> (the comma to separate sub-options you mentioned earlier) >>>>>>>>> >>>>>>>>> Therefore the suggestion without curly brackets, giving several >>>>>>>>> commands >>>>>>>>> after one another seems good with regard to backwards compatibility. >>>>>>>>> >>>>>>>>> Motivation: hawt.io uses the MBean >>>>>>>>> com.sun.management:type=DiagnosticCommand >>>>>>>>> to access the same functionality as jcmd. Variations in option syntax >>>>>>>>> across >>>>>>>>> Java versions is already an issue (only affecting sub-options though, >>>>>>>>> as >>>>>>>>> each command is a separate JMX operation). So syntax compatibility is >>>>>>>>> highly >>>>>>>>> appreciated :-) >>>>>>>>> >>>>>>>>> Martin >>>>>>>>> >>>>>>>>> lør. 12. mai 2018 kl. 20:11 skrev Kirk Pepperdine >>>>>>>>> <kirk.pepperd...@gmail.com>: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> On May 10, 2018, at 11:26 AM, Thomas Stüfe <thomas.stu...@gmail.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> On Thu, May 10, 2018 at 9:13 AM, Kirk Pepperdine >>>>>>>>>>> <kirk.pepperd...@gmail.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> The stacking at the safe point would be a huge win. Right now >>>>>>>>>>>> thread >>>>>>>>>>>> dump consistency can really confuse people as the tooling will show >>>>>>>>>>>> two >>>>>>>>>>>> threads owning the same lock at seemingly the same time. Reality, >>>>>>>>>>>> it’s >>>>>>>>>>>> just >>>>>>>>>>>> that people didn’t realize you get a safe point for each thread >>>>>>>>>>>> therefor >>>>>>>>>>>> there is motion in the system as you’re collecting the data. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I am a bit confused. What tooling are you talking about? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> jstack. I’ve not seen it with jcmd. But I often see 2 threads holding >>>>>>>>>> the >>>>>>>>>> same lock at the “same time” which is of course non-sense. I can dig >>>>>>>>>> one >>>>>>>>>> up >>>>>>>>>> for you if you’re still confused. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> As an aside, it’s amazing how many dev’s aren’t aware of jcmd. Just >>>>>>>>>>>> yesterday after my session at Devoxx I had someone ask me about >>>>>>>>>>>> using >>>>>>>>>>>> jfr >>>>>>>>>>>> from the command line, many in that session had not seen jcmd >>>>>>>>>>>> before. >>>>>>>>>>>> The >>>>>>>>>>>> feedback was, wow, this is very useful and I wished I had of known >>>>>>>>>>>> about it >>>>>>>>>>>> earlier. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yes, jcmd is quite useful. I also really like the simple design, >>>>>>>>>>> which >>>>>>>>>>> is by default up- and downward compatible (so you can talk to any >>>>>>>>>>> VM, >>>>>>>>>>> new, old, it should not matter). That is just good design. We - Sap >>>>>>>>>>> - >>>>>>>>>>> work to extend jcmd, to help our support folks. E.g. the whole new >>>>>>>>>>> VM.metaspace command chain. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> And simple it is….well done!!! >>>>>>>>>> >>>>>>>>>> — Kirk >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>> >>>