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

David Holmes david.holmes at oracle.com
Tue Jun 23 01:20:30 UTC 2015

CCC approved and changes pushed.

David

On 19/06/2015 1:32 PM, David Holmes wrote:

On 15/06/2015 10:57 PM, Thomas Stüfe wrote:

Hi Volker, David,

Volker, thanks for the review! new version: http://cr.openjdk.java.net/~stuefe/webrevs/8078513/webrev.04/webrev/ I added all changes Volker wanted. The copyright notice change in agent/src/os/linux/procservice.h has the wrong format - needs comma after second year and has to remain on a single line. I'll fix before pushing. Oh, and I forgot to add Volker as a Reviewer, please add him when pushing. Will do. I also forgot that a CCC request needed to be put in to deprecate ThreadSafetyMargin :( That will delay the push a while I'm afraid. Thanks, David Kind Regards, Thomas

On Mon, Jun 15, 2015 at 11:23 AM, Thomas Stüfe <thomas.stuefe at gmail.com_ _<mailto:thomas.stuefe at gmail.com>> wrote: Hi, sorry, dropped the ball on this one. Will post an updated version soon. Thomas On Fri, Jun 12, 2015 at 4:02 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote: Hi Thomas, If you can address Volker's comments we can move forward with this. I'll be primary Reviewer and Volker a second reviewer. Thanks, David On 3/06/2015 3:52 PM, Thomas Stüfe wrote: Ping... Still need a second reviewer. Thanks! On Wednesday, May 20, 2015, Thomas Stüfe <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com> <mailto:thomas.stuefe at gmail.com_ _<mailto: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_ _<mailto:david.holmes at oracle.com> <javascript:e(%7B%7D,'cvml','david.holmes at oracle.com_ _<mailto: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 oslinux.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> <javascript:e(%7B%7D,'cvml','david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com>');> <mailto:david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com> <javascript:e(%7B%7D,'cvml','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: oslinux.cpp: The whole MAPGROWSDOWN 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 MAPNORESERVE), so the memory is committed right away. See glibc sources, nptl/allocatestack.c. In former times (LinuxThreads) thread stacks were allocated using mmap(MAPGROWSDOWN), 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 oslinux.cpp, "os::Linux::manuallyexpandstack". I think there may still be cases where we run on threads whose stacks are allocated with mmap(MAPGROWSDOWN), 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(MAPGROWSDOWN) 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(MAPGROWSDOWN) 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 oslinux.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 createThreadlock (now unused) seems to have been copied into src/os/aix/vm/osaix.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/oslinux.hpp Copyright needs updating. done --- src/oscpu/linuxx86/vm/oslinuxx86.cpp Copyright needs updating. done os::Linux::supportsvariablestacksize() is now dead code (value is true for all arch's AFAICS) so can be removed from all src/oscpu/linuxXXX/oslinuxXXX.cpp. I think supportsvariablestacksize() 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/oscpu/linuxx86/vm/threadLSlinuxx86.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 initialpid and returned in os::currentprocessid(). 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::sunjavalauncherpid()). Both workaround were removed and os::currentprocessid() 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/javamdsolinux.c void SetJavaLauncherPlatformProps() { /* Linux only */ #ifdef linux const char *substr = "-Dsun.java.launcher.pid="; char *pidpropstr = (char *)JLIMemAlloc(JLIStrLen(substr) + MAXPIDSTRSZ + 1); sprintf(pidpropstr, "%s%d", substr, getpid()); AddOption(pidpropstr, 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 MAPGROWSDOWN 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 MAPFIXED - may at some point trash the java heap. To prevent this, every allocation done with os::reservememory() and friends recorded a "highestvmreservedaddress". There was a function called "threadsafetycheck" 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-dev mailing list