Re: RFR: 8350939: Revisit Windows PDH buffer size calculation for OperatingSystemMXBean
On Tue, 4 Mar 2025 08:14:52 GMT, Kevin Walls wrote: >> src/jdk.management/windows/native/libmanagement_ext/OperatingSystemImpl.c >> line 296: >> >>> 294: /* PDH format patterns, and lengths of their constant component. */ >>> 295: static const char* const OBJECT_COUNTER_FMT = "\\%s\\%s"; >>> 296: static const size_t OBJECT_COUNTER_FMT_LEN = 2; >> >> Pre-existing but as per earlier discussions on this, what exactly do these >> lengths mean?? > > Yes was trying to clarify that with the comment: the "constant component", > the number of fixed characters. So allocation size is e.g. > OBJECT_COUNTER_FMT_LEN (==2) to account for the two backslashes in that > format. The lengths of the two strings, plus the terminator, are added in > the method that constructs the counter path.. Ah! Makes sense now. :) - PR Review Comment: https://git.openjdk.org/jdk/pull/23861#discussion_r1979271076
Re: RFR: 8351165: Remove unused includes from vmStructs [v2]
> Please review this trivial change to remove some unneeded includes and type > declarations from vmStructs. The SystemDictionary type is unused because > classes are looked up in the ClassLoaderDataGraph in the SA, and they haven't > been in a single SystemDictionary for a very long time. > Tested with SA tests locally. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: One more. - Changes: - all: https://git.openjdk.org/jdk/pull/23897/files - new: https://git.openjdk.org/jdk/pull/23897/files/c66907b6..e33d5066 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=00-01 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/23897.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23897/head:pull/23897 PR: https://git.openjdk.org/jdk/pull/23897
Re: RFR: 8351165: Remove unused includes from vmStructs [v3]
> Please review this trivial change to remove some unneeded includes and type > declarations from vmStructs. The SystemDictionary type is unused because > classes are looked up in the ClassLoaderDataGraph in the SA, and they haven't > been in a single SystemDictionary for a very long time. > Tested with SA tests locally. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Change the SystemDictionary section header comment too. - Changes: - all: https://git.openjdk.org/jdk/pull/23897/files - new: https://git.openjdk.org/jdk/pull/23897/files/e33d5066..86443005 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/23897.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23897/head:pull/23897 PR: https://git.openjdk.org/jdk/pull/23897
RFR: 8351165: Remove unused includes from vmStructs
Please review this trivial change to remove some unneeded includes and type declarations from vmStructs. The SystemDictionary type is unused because classes are looked up in the ClassLoaderDataGraph in the SA, and they haven't been in a single SystemDictionary for a very long time. Tested with SA tests locally. - Commit messages: - 8351165: Remove unused includes from vmStructs Changes: https://git.openjdk.org/jdk/pull/23897/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351165 Stats: 17 lines in 2 files changed: 0 ins; 15 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/23897.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23897/head:pull/23897 PR: https://git.openjdk.org/jdk/pull/23897
Re: RFR: 8350524: Some hotspot/jtreg/serviceability/dcmd/vm tier1 tests fail on static JDK
On Sat, 22 Feb 2025 03:31:53 GMT, Jiangli Zhou wrote: > Please review the fix in following tests to not check for shared libraries > when running on static JDK: > > test/hotspot/jtreg/serviceability/dcmd/vm/DynLibsTest.java > test/hotspot/jtreg/serviceability/dcmd/vm/SystemDumpMapTest.java > test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTest.java I'm not familiar with details of the output from the System.map command but the change looks reasonable, hopefully @tstuefe or someone familar with this command and these tests can review. - PR Comment: https://git.openjdk.org/jdk/pull/23734#issuecomment-2698631487
Re: RFR: 8351165: Remove unused includes from vmStructs [v3]
On Tue, 4 Mar 2025 13:22:24 GMT, Coleen Phillimore wrote: >> Please review this trivial change to remove some unneeded includes and type >> declarations from vmStructs. The SystemDictionary type is unused because >> classes are looked up in the ClassLoaderDataGraph in the SA, and they >> haven't been in a single SystemDictionary for a very long time. >> Tested with SA tests locally. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Change the SystemDictionary section header comment too. I think that if classes are being removed from here that the friendship with VMStructs by those classes should also be removed. - Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23897#pullrequestreview-2657808981
Re: RFR: 8351165: Remove unused includes from vmStructs [v3]
On Tue, 4 Mar 2025 14:30:49 GMT, Kim Barrett wrote: > I think that if classes are being removed from here that the friendship with VMStructs by those classes should also be removed. Good point. Thanks for remembering these. - PR Comment: https://git.openjdk.org/jdk/pull/23897#issuecomment-2698293824
Re: RFR: 8351165: Remove unused includes from vmStructs [v4]
> Please review this trivial change to remove some unneeded includes and type > declarations from vmStructs. The SystemDictionary type is unused because > classes are looked up in the ClassLoaderDataGraph in the SA, and they haven't > been in a single SystemDictionary for a very long time. > Tested with SA tests locally. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Remove some friends. - Changes: - all: https://git.openjdk.org/jdk/pull/23897/files - new: https://git.openjdk.org/jdk/pull/23897/files/86443005..f2bb9364 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23897&range=02-03 Stats: 9 lines in 5 files changed: 0 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23897.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23897/head:pull/23897 PR: https://git.openjdk.org/jdk/pull/23897
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v17]
On Fri, 28 Feb 2025 20:40:37 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request incrementally with one > additional commit since the last revision: > > updated copyright headers @sercher Your change (at version bae78ff7c43e0446ef583db1c43b4a3d6c06ad4f) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/21808#issuecomment-2697178573
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v17]
On Fri, 28 Feb 2025 20:40:37 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request incrementally with one > additional commit since the last revision: > > updated copyright headers Marked as reviewed by mbaesken (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21808#pullrequestreview-2656386374
Re: RFR: 8350939: Revisit Windows PDH buffer size calculation for OperatingSystemMXBean
On Tue, 4 Mar 2025 06:42:09 GMT, David Holmes wrote: >> Following on from JDK-8350820, which backed out the _snprintf to snprintf >> change (JDK-8336289) in OperatingSystemImpl.c on Windows, because the >> counter names were being truncated (so CPU monitoring was not possible). >> >> This change moves to snprintf again, but the counter names are not truncated. >> >> snprintf must need the null terminator to fit inside the buffer length >> given. It does not, and snprintf truncates (and always add the null >> terminator). > > src/jdk.management/windows/native/libmanagement_ext/OperatingSystemImpl.c > line 296: > >> 294: /* PDH format patterns, and lengths of their constant component. */ >> 295: static const char* const OBJECT_COUNTER_FMT = "\\%s\\%s"; >> 296: static const size_t OBJECT_COUNTER_FMT_LEN = 2; > > Pre-existing but as per earlier discussions on this, what exactly do these > lengths mean?? Yes was trying to clarify that with the comment: the "constant component", the number of fixed characters. So allocation size is e.g. OBJECT_COUNTER_FMT_LEN (==2) to account for the two backslashes in that format, plus the lengths of the two strings, plus the terminator. - PR Review Comment: https://git.openjdk.org/jdk/pull/23861#discussion_r1978858171
