RFR(s): 8078513: [linux] Clean up code relevant to LinuxThreads implementation
David Holmes
david.holmes at oracle.com
Wed May 20 08:42:24 UTC 2015
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