[8u60] RFR: 8078470: [Linux] Replace syscall use in os::fork_and_exec with glibc fork() and execve()
David Holmes
david.holmes at oracle.com
Wed Apr 29 23:25:27 UTC 2015
Hi Dan,
Thanks for the Review!
On 29/04/2015 2:04 AM, Daniel D. Daugherty wrote:
> > http://cr.openjdk.java.net/~dholmes/8078470/webrev.v2/
>
> src/os/linux/vm/os_linux.cpp
> So much cleaner now!
>
> src/share/vm/utilities/vmError.cpp
> L1017: out.print_cr("os::fork_and_exec failed: %s", strerror(errno));
> L1104: tty->print_cr("os::fork_and_exec failed: %s", strerror(errno));
> So how about adding the 'errno' value in parens: "(%d)"
> just in case strerror() can't handle the 'errno' param?
Will add.
> test/runtime/ErrorHandling/TestOnError.java
> L38: if (!Platform.isDebugBuild()) {
> Should this be the new @requires tag instead?
I don't see that existing yet:
http://openjdk.java.net/jtreg/tag-spec.html#requires_names
?
> L63: output.stdoutShouldMatch("^" + msg); // match start of line only
> Is the 'OnError' handler serialized? In other words, do we
> have some assurance that the OnError output will be on a line
> by itself and not mixed with other "crashing VM" output?
In the test yes - we trigger a synchronous crash during VM startup, and
the error handler is serialized.
> test/runtime/ErrorHandling/TestOnOutOfMemoryError.java
> L67: output.stdoutShouldMatch("^" + msg); // match start of line only
> Same question about output interleaving.
Again given the nature of the test (simple trigger by impossible array
size) the error handler will be called synchronously. Nothing else
should be happening to trigger other output. Probably not impossible but
seems very unlikely.
>
> I don't think any of my comments are critical so you can choose
> to make the changes or not.
Thanks. I'll make the (%d) change and push for 8u. Will revisit for 9
"backport" if @require changes show up.
David
> Thumbs up.
>
> Dan
>
>
> On 4/27/15 10:50 PM, David Holmes wrote:
>> Ping: still need a (R)eviewer please.
>>
>> Thanks,
>> David
>>
>> On 25/04/2015 7:45 AM, David Holmes wrote:
>>> Hi Thomas,
>>>
>>> Thanks for looking at this.
>>>
>>> Note to readers: Still need a (R)eviewer for this please.
>>>
>>> On 24/04/2015 7:00 PM, Thomas Stüfe wrote:
>>>> Hi David,
>>>>
>>>> Unfortunately printing out errno in VMError will not work so well,
>>>> as it
>>>> is not clear if it is from fork() or execve(), and it is vulnerable
>>>> against future changes of os::fork_and_exec(). Also, will not do
>>>> anything for you on Windows.
>>>
>>> I thought about doing something inside fork_and_exec but decided against
>>> it for a couple of reasons:
>>>
>>> 1. Needs to be done for every platform individually.
>>> 2. Far from clear which mechanism would be appropriate given the
>>> different contexts in which it can be used - which is also why I did not
>>> do something for its use when debugging.
>>>
>>> As for Windows ... not sure what it will do but I was following existing
>>> practice - see os_windows.cpp and perfMemory_windows.cpp.
>>>
>>>> I do not see a nice solution. Returning errno will not work, or be
>>>> ugly,
>>>> because you'd need to return GetLastError for windows, which has a
>>>> different semantic, and the caller must remember that.
>>>
>>> And a return value, unless encoded, won't tell you which of fork and
>>> execve failed.
>>>
>>>> On Solaris, os::fork_and_exec() prints a warning if PrintWarning() is
>>>> on, which in my opinion does not help. That printout is very verbose
>>>> and
>>>> you have to remember setting it before starting the process.
>>>>
>>>> In some places in error handling (e.g. "check_dump_limits()") we return
>>>> a string in a caller provided buffer describing the error which then is
>>>> printed into the error logs. So, that may be a pragmatic solution,
>>>> especially because we already have the scratch buffer in
>>>> VMError::report_and_die().
>>>>
>>>> On the other hand, in almost any os::... function we just swallow error
>>>> details, as we did in os::fork_and_exec() before, so maybe we just
>>>> leave
>>>> it at this.
>>>
>>> I'll leave what I have now, pending further comments from anyone. It is
>>> better than nothing I think, and would have saved me a bit of time when
>>> I first encountered the problem with the missing syscall.
>>>
>>>> Otherwise, change looks fine to me.
>>>
>>> Thanks,
>>> David
>>>
>>>> ..Thomas
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Apr 24, 2015 at 4:03 AM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>> Hi Thomas,
>>>>
>>>> On 23/04/2015 6:00 PM, Thomas Stüfe wrote:
>>>>
>>>> On Thu, Apr 23, 2015 at 9:50 AM, David Holmes
>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>>> <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>> wrote:
>>>> On 23/04/2015 5:24 PM, Thomas Stüfe wrote:
>>>> - would it be possible to use vfork() instead of
>>>> fork()? Might
>>>> increase
>>>> the chance of fork() succeeding in out-of-memory
>>>> scenarios. The way
>>>> fork/execve is used here is simple enough for vfork().
>>>>
>>>> I'm reluctant to use vfork() as it is an unknown. The use
>>>> of fork()
>>>> has been trialled by Google with no problem.
>>>>
>>>> I understand.
>>>>
>>>> We use vfork() by default on Linux and HP-UX since 3-4 years
>>>> for
>>>> Runtime.exec() without problems (but our implementation differs
>>>> wildly
>>>> from the standard one). But then, Runtime.exec does not get
>>>> called in
>>>> error situations, so the conditions may be different for
>>>> os::fork_and_exec().
>>>>
>>>>
>>>> Right - os::fork_and_exec is potentially a lot more fragile given
>>>> the potential execution context. Which prompted me to add a second
>>>> test for the general OnError handling case. I also moved the tests
>>>> to runtime/ErrorHandling to match the directory that exists in
>>>> JDK 9.
>>>>
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8078470/webrev.v2/
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>> - would it be possible to print out errno in case fork
>>>> fails?
>>>>
>>>>
>>>> Sure.
>>>>
>>>>
>>>> Thanks!
>>>>
>>>> ..Thomas
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Regards, Thomas
>>>>
>>>>
>>>> On Thu, Apr 23, 2015 at 8:40 AM, David Holmes
>>>> <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>
>>>> <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>
>>>> <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>
>>>>
>>>> <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>>> wrote:
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~dholmes/8078470/webrev/
>>>>
>>>> bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8078470
>>>>
>>>> For historical reasons, dating back to
>>>> LinuxThreads,
>>>> os::fork_and_exec uses direct syscalls to perform
>>>> fork and
>>>> execve
>>>> functions. The fork syscall has been
>>>> deprecated on
>>>> linux
>>>> for quite
>>>> some time and some distributions are now
>>>> shipping with
>>>> deprecated
>>>> syscalls removed - this results in a ENOSYS error
>>>> and the
>>>> "OnError"
>>>> commands that utilize os::fork_and_exec no longer
>>>> work.
>>>>
>>>> The problem was discussed here:
>>>>
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-April/017916.html
>>>>
>>>>
>>>> It seems the concerns surrounding direct use of
>>>> glibc
>>>> fork() and
>>>> exec() are no longer legitimate and Martin
>>>> Buchholz reports
>>>> that
>>>> Google replaced the syscalls with glibc calls
>>>> some
>>>> time ago
>>>> and have
>>>> not had any problems. So we propose to apply the
>>>> same change
>>>> uniformly on Linux.
>>>>
>>>> This is initially going into 8u60 via
>>>> jdk8u-hs-dev
>>>> due to time
>>>> constraints on the 8u60 schedule, and an internal
>>>> limitation
>>>> preventing me from pushing this to 9 right now.
>>>> But it will be
>>>> "backported" to 9 ASAP.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>
>>>>
>
More information about the hotspot-dev
mailing list