RFR(s): 8078513: [linux] Clean up code relevant to LinuxThreads implementation
Thomas Stüfe
thomas.stuefe at gmail.com
Wed May 20 09:28:18 UTC 2015
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>
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
>> <mailto: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