RFR(s): 8078513: [linux] Clean up code relevant to LinuxThreads implementation

Volker Simonis volker.simonis at gmail.com
Wed Jun 3 13:07:48 UTC 2015


Hi Thomas,

the changes look good.

Just a few more minor cleanups:)

agent/src/os/linux/proc_service.h
- can you please just update the copyright line

src/os/linux/vm/os_linux.{hpp,cpp}
- you removed 'pthread_getattr_func_type' because it's not used
anymore but there's still one unused definition in
src/os/bsd/vm/os_bsd.hpp (probably a porting leftover). Can you please
remove that one as well.
- you removed safe_cond_timedwait in os_linux.{hpp,cpp}. Can you
please also remove the commented out version in os_aix.cpp and also
remove it in os_bsd.cpp because it's not required there as well
(probably a porting leftover again).
- same for _thread_safety_check() - this can also be removed from
os_bsd.cpp because it always returns true anyway
- same for _highest_vm_reserved_address - this can also be removed
from os_bsd.cpp because it is never used

Thank you and best regards,
Volker


On Wed, Jun 3, 2015 at 7:52 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 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