[8u60] RFR: 8078470: [Linux] Replace syscall use in os::fork_and_exec with glibc fork() and execve()
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Apr 28 16:04:15 UTC 2015
> 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?
test/runtime/ErrorHandling/TestOnError.java
L38: if (!Platform.isDebugBuild()) {
Should this be the new @requires tag instead?
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?
test/runtime/ErrorHandling/TestOnOutOfMemoryError.java
L67: output.stdoutShouldMatch("^" + msg); // match start of line only
Same question about output interleaving.
I don't think any of my comments are critical so you can choose
to make the changes or not.
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