RFR(s): 8078513: [linux] Clean up code relevant to LinuxThreads implementation
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Jun 3 05:52:43 UTC 2015
Ping...
Still need a second reviewer.
Thanks!
On Wednesday, May 20, 2015, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> David, thanks!
>
> to all: could I have a second reviewer, please?
>
> Thanks & Kind Regards, Thomas
>
> On Wed, May 20, 2015 at 10:42 AM, David Holmes <david.holmes at oracle.com
> <javascript:_e(%7B%7D,'cvml','david.holmes at oracle.com');>> wrote:
>
>> Hi Thomas,
>>
>> Summary: ok - need second reviewer ( I think Coleen is away)
>>
>> More inline ...
>>
>> On 19/05/2015 10:26 PM, Thomas Stüfe wrote:
>>
>>> Hi David,
>>>
>>> new webrev:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8078513/webrev.02/webrev/
>>>
>>> Only two things changed. I reverted my comment change in the SA and in
>>> os_linux.cpp I massaged the "expand-thread-stack" comment text a bit.
>>>
>>> More details inline.
>>>
>>> On Thu, May 14, 2015 at 4:40 AM, David Holmes <david.holmes at oracle.com
>>> <javascript:_e(%7B%7D,'cvml','david.holmes at oracle.com');>
>>> <mailto:david.holmes at oracle.com
>>> <javascript:_e(%7B%7D,'cvml','david.holmes at oracle.com');>>> wrote:
>>>
>>> Hi Thomas,
>>>
>>> On 14/05/2015 2:32 AM, Thomas Stüfe wrote:
>>>
>>> Hi David,
>>>
>>> here the new version:
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8078513/webrev.01/webrev/
>>>
>>> Differences to the old version are just comment changes, and I
>>> also
>>> modified some comments in the SA as Staffan requested.
>>>
>>>
>>> agent/src/os/linux/libproc.h
>>>
>>> Modified comments still refer to "both thread libraries" but now the
>>> two libraries have not been mentioned, so there is no context to
>>> know what the two libraries are.
>>>
>>>
>>> I reverted my comment change to the original version. I think I do not
>>> know enough about the SA internals to make the comment clear and
>>> concise. Someone with more SA knowledge may be better suited to change
>>> that comment.
>>>
>>
>> Ok. I think you were pretty close anyway, just need to get rid of the
>> "both thread libraries" references.
>>
>>
>>
>>> More commentary inline below (with trimming)...
>>>
>>>
>>> See remarks inline:
>>>
>>> os_linux.cpp:
>>>
>>> The whole MAP_GROWSDOWN discussion and code also seems to
>>> be no
>>> longer relevant.
>>>
>>>
>>> I spent a lot of time digging around in the history of this
>>> thing. I am
>>> unsure whether that stack expansion coding can be removed. There
>>> are
>>> some rare case where they may still be needed:
>>>
>>> That whole coding revolves around the question whether thread
>>> stacks
>>> need to be expanded manually.
>>>
>>> Nowadays, normal threads allocated with a modern pthread library
>>> (NPTL)
>>> just allocate the whole stack with mmap(without
>>> MAP_NORESERVE), so the
>>> memory is committed right away. See glibc sources,
>>> nptl/allocate_stack.c.
>>>
>>> In former times (LinuxThreads) thread stacks were allocated using
>>> mmap(MAP_GROWSDOWN), which needed cooperation from the linux
>>> kernel:
>>> kernel caught faults caused by expanding the stack and
>>> transparently
>>> mapped the next page. It needed to distinguish real page faults
>>> from
>>> stack expansion page faults, and seems the logic did not always
>>> work, so
>>> manual expansion was needed - see os_linux.cpp,
>>> "os::Linux::manually_expand_stack".
>>>
>>> I think there may still be cases where we run on threads whose
>>> stacks
>>> are allocated with mmap(MAP_GROWSDOWN), even though LinuxThreads
>>> is gone:
>>>
>>> - with primordial threads (not sure, did not test). We do not
>>> run on
>>> primordial thread but someone may write a launcher and start a
>>> VM from
>>> the primordial thread or attach the primordial thread to the VM.
>>>
>>>
>>> This is already a very grey area in the VM. See:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-6516512
>>>
>>> "HotSpot:thread terminology should be clearer or cleaned up."
>>>
>>> So until that is investigated and cleaned up this code can't really
>>> be touched.
>>>
>>>
>>> Interesting and well written bug report. Unfortunately, the issue seems
>>> to be in deep sleep.
>>>
>>
>> Sadly often true :(
>>
>> At SAP, we explicitly forbid all our internal users to invoke the VM on
>>> the primordial thread, and have done so since a long time. We tell all
>>> our users to spawn off an own pthread for VM creation. I think the AIX
>>> port will not even come up, we just assert.
>>>
>>> But this is more of a political question. If we pull support for running
>>> on the primordial thread in the OpenJDK, there may be applications which
>>> break. On the other hand, does this scenario - running on the primordial
>>> thread - get tested? Would we even notice if it were not to work anymore?
>>>
>>
>> There may be some JNI tests that use the primordial thread but I can't
>> say for sure.
>>
>> - someone may write its own thread implementation using clone and
>>> mmap(MAP_GROWSDOWN) allocated stacks.
>>>
>>> These cases are probably extremely rare, and I would have no
>>> qualms
>>> removing support for them, but I do not want to decide that.
>>>
>>>
>>> Fair enough. As I said I think this expands into a cleanup up of the
>>> whole "initial thread" business anyway.
>>>
>>> Sidenote: These cases are inherently unsafe because
>>> mmap(MAP_GROWSDOWN)
>>> is too. Ulrich Drepper tried without success to remove support
>>> for these
>>> flags from the linux kernel: https://lwn.net/Articles/294001/
>>>
>>> I added some comment to os_linux.cpp to explain this a bit
>>> better.
>>>
>>>
>>> The two comments don't rely flow together due to the "Force Linux
>>> kernel to expand current thread stack." lead-in of the second
>>> comment (which isn't actually a grammatically correct sentence). But
>>> I can't really think of anything to do that doesn't start making
>>> numerous changes to what is already a confusing and disjointed
>>> comment block.
>>>
>>>
>>> I tried to improve the comment a bit, to clearly distinguish it from the
>>> older comment and to provide enough information for whenever we decide
>>> to clean up this expand-thread-stack coding.
>>>
>>
>> Revised comment is much clearer - thanks!
>>
>> David
>> -----
>>
>>
>> Kind Regards, Thomas
>>>
>>>
>>>
>>>
>>> The gcc 2.x workarounds can also be removed I think.
>>>
>>> I wonder if we can also eradicate
>>> WorkAroundNPTLTimedWaitHang :)
>>>
>>>
>>> I do not dare to do that - I do not know enough about this issue.
>>>
>>>
>>> Separate RFE. I doubt we care about the old systems that had this
>>> problem.
>>>
>>>
>>> Aside: The createThread_lock (now unused) seems to have
>>> been copied
>>> into src/os/aix/vm/os_aix.hpp as part of the AIX port and
>>> should be
>>> removed at some point.
>>>
>>>
>>> Yes I know :-) It is not the only Linux-specific code which
>>> crept into
>>> our AIX port. We actually have a bug open to deal with that:
>>> https://bugs.openjdk.java.net/browse/JDK-8079125
>>>
>>>
>>> Good to know :)
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>> ---
>>>
>>> src/os/linux/vm/os_linux.hpp
>>>
>>> Copyright needs updating.
>>>
>>>
>>> done
>>>
>>> ---
>>>
>>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>>
>>> Copyright needs updating.
>>>
>>> done
>>>
>>> os::Linux::supports_variable_stack_size() is now dead
>>> code (value
>>> is true for all arch's AFAICS) so can be removed from all
>>> src/os_cpu/linux_XXX/os_linux_XXX.cpp.
>>>
>>> I think supports_variable_stack_size() is also always true
>>> on other
>>> platforms (AIX, BSD) and so could be cleaned up in that
>>> code too
>>> (separate clean-up CR okay for that).
>>>
>>>
>>> I agree, lets do this in a different RFR because this would touch
>>> non-linux sources and I want to keep this linux specific. I
>>> opened
>>> https://bugs.openjdk.java.net/browse/JDK-8080298 for this.
>>>
>>>
>>> Thanks for filing.
>>>
>>>
>>>
>>> ---
>>>
>>> src/os_cpu/linux_x86/vm/threadLS_linux_x86.cpp
>>>
>>> Copyright needs updating.
>>>
>>> done
>>>
>>> ---
>>>
>>> I'm happy to sponsor this for you.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>>
>>> This change removes (I hope all) remnants of code
>>> dealing with
>>> LinuxThreads
>>> from the hotspot.
>>>
>>> Discussion of the changes:
>>>
>>> 1) on LinuxThreads, each thread had an own pid, so
>>> getpid()
>>> would return a
>>> unique pid for every thread it was invoked in. On NPTL,
>>> this is
>>> not an
>>> issue anymore; getpid() works as expected returning a
>>> global pid.
>>> For LinuxThreads, there was a workaround where at VM
>>> startup the
>>> pid was
>>> captured (on the thread invoking CreateJavaVM) and this
>>> pid was
>>> stored in
>>> static variable _initial_pid and returned in
>>> os::current_process_id().
>>> Later another workaround was added because CreateJavaVM
>>> was not
>>> invoked on
>>> the primordial thread anymore, so the pid was handed in
>>> via a
>>> define (see
>>> Arguments::sun_java_launcher_pid()).
>>>
>>> Both workaround were removed and
>>> os::current_process_id() was
>>> changed to
>>> just return getpid().
>>>
>>>
>>> We should remove the code on the JDK side as well -
>>> possibly as a
>>> follow up clean up (but via the same forest as this will be
>>> pushed):
>>>
>>> ./java.base/unix/native/libjli/java_md_solinux.c
>>>
>>> void SetJavaLauncherPlatformProps() {
>>> /* Linux only */
>>> #ifdef __linux__
>>> const char *substr = "-Dsun.java.launcher.pid=";
>>> char *pid_prop_str = (char
>>> *)JLI_MemAlloc(JLI_StrLen(substr) +
>>> MAX_PID_STR_SZ + 1);
>>> sprintf(pid_prop_str, "%s%d", substr, getpid());
>>> AddOption(pid_prop_str, NULL);
>>> #endif /* __linux__ */
>>>
>>> }
>>>
>>>
>>> Unfortunately, sun.launcher.pid gets used in the wild, e.g.:
>>>
>>>
>>> http://stackoverflow.com/questions/35842/how-can-a-java-program-get-its-own-process-id
>>>
>>> Kind Regards, Thomas
>>>
>>>
>>>
>>>
>>> 2) os::Linux::gettid(): Linux uses the kernel thread id
>>> for
>>> OSThread thread
>>> id - I did not change that. But by now the system call
>>> gettid
>>> should be
>>> available in every Linux kernel, so I removed the
>>> fallback handling.
>>>
>>> 3) A number of changes dealt with the way LinuxThreads
>>> allocated
>>> thread
>>> stacks (with mmap and the MAP_GROWSDOWN flag). On
>>> LinuxThreads,
>>> it was not
>>> possible to change the size of thread stacks (to start
>>> a new
>>> thread with a
>>> different stack size). NPTL can do this and all the
>>> places where
>>> we dealt
>>> with fixed stacks I changed.
>>>
>>> 4) On LinuxThreads, there is the problem that the
>>> thread stacks
>>> - which
>>> grow down and are allocated with MAP_FIXED - may at
>>> some point
>>> trash the
>>> java heap. To prevent this, every allocation done with
>>> os::reserve_memory()
>>> and friends recorded a "_highest_vm_reserved_address".
>>> There was
>>> a function
>>> called "_thread_safety_check" which prevented start of
>>> new
>>> threads if
>>> thread stacks came dangerously close to the highest
>>> reserved
>>> address + a
>>> gap; latter gap could be set via parameter
>>> ThreadSafetyMargin.
>>>
>>> All this coding was removed. Note that this coding
>>> probably was
>>> already
>>> broken since a long time, because there are many
>>> allocations
>>> which were not
>>> tracked.
>>>
>>> 5) Recognition of glibc version and pthread library
>>> version were
>>> very
>>> elaborate to deal with recognition of LinuxThreads
>>> implementation. Those
>>> were dumbed down and now just assert in the highly
>>> unlikely case
>>> that we
>>> encounter a LinuxThreads implementation.
>>>
>>> The rest of the stuff is more straight-forward.
>>>
>>> ---
>>>
>>> I built the changes on Linux x64, x86 and ppc64 and ran
>>> jtreg tests
>>> (hotspot/test) on these platforms too. I did not see
>>> any issues,
>>> but I'd
>>> like to get a couple of reviews for this.
>>>
>>> Kind Regards,
>>> Thomas
>>>
>>>
>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list