RFR: 8264412: AArch64: CPU description should refer DMI

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8264412: AArch64: CPU description should refer DMI

Yasumasa Suenaga-7
`jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.

[JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.

We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.

For example, we can get following CPU description on AWS A1 instance after this change:

jdk.CPUInformation {
  startTime = 05:28:24.506
  cpu = "AArch64"
  description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
  sockets = 8
  cores = 8
  hwThreads = 8
}

-------------

Commit messages:
 - refactoring
 - 8264412: AArch64: CPU description should refer DMI

Changes: https://git.openjdk.java.net/jdk/pull/3259/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264412
  Stats: 22 lines in 1 file changed: 13 ins; 3 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3259/head:pull/3259

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Aleksey Shipilev-5
On Tue, 30 Mar 2021 07:53:59 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>
> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>
> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>
> For example, we can get following CPU description on AWS A1 instance after this change:
>
> jdk.CPUInformation {
>   startTime = 05:28:24.506
>   cpu = "AArch64"
>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>   sockets = 8
>   cores = 8
>   hwThreads = 8
> }

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 196:

> 194:   if (!read_fully("/proc/device-tree/compatible", buf, buflen)) {
> 195:     if (!read_fully("/sys/devices/virtual/dmi/id/board_name", buf, buflen)) {
> 196:       read_fully("/sys/devices/virtual/dmi/id/product_name", buf, buflen);

I think it would be nicer to write:

  if (read_fully("/proc/device-tree/compatible", buf, buflen)) {
    return;
  }
  if (read_fully("/sys/devices/virtual/dmi/id/board_name", buf, buflen)) {
    return;
  }
  read_fully("/sys/devices/virtual/dmi/id/product_name", buf, buflen);
...in case we would add more lines afterwards.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Yasumasa Suenaga-7
On Tue, 30 Mar 2021 08:45:37 GMT, Aleksey Shipilev <[hidden email]> wrote:

>> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>>
>> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
>> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>>
>> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>>
>> For example, we can get following CPU description on AWS A1 instance after this change:
>>
>> jdk.CPUInformation {
>>   startTime = 05:28:24.506
>>   cpu = "AArch64"
>>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>>   sockets = 8
>>   cores = 8
>>   hwThreads = 8
>> }
>
> src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 196:
>
>> 194:   if (!read_fully("/proc/device-tree/compatible", buf, buflen)) {
>> 195:     if (!read_fully("/sys/devices/virtual/dmi/id/board_name", buf, buflen)) {
>> 196:       read_fully("/sys/devices/virtual/dmi/id/product_name", buf, buflen);
>
> I think it would be nicer to write:
>
>   if (read_fully("/proc/device-tree/compatible", buf, buflen)) {
>     return;
>   }
>   if (read_fully("/sys/devices/virtual/dmi/id/board_name", buf, buflen)) {
>     return;
>   }
>   read_fully("/sys/devices/virtual/dmi/id/product_name", buf, buflen);
> ...in case we would add more lines afterwards.

I changed it in new commit.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Andrew Haley-2
In reply to this post by Yasumasa Suenaga-7
On Tue, 30 Mar 2021 07:53:59 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>
> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>
> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>
> For example, we can get following CPU description on AWS A1 instance after this change:
>
> jdk.CPUInformation {
>   startTime = 05:28:24.506
>   cpu = "AArch64"
>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>   sockets = 8
>   cores = 8
>   hwThreads = 8
> }

This patch is basically OK, except the question about a somewhat obscure bit of code.

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 178:

> 176:     ssize_t read_sz = read(fd, buf, buflen - 1);
> 177:     close(fd);
> 178:     if ((read_sz > 0) && isgraph(*buf)) {

What is `isgraph(*buf)` doing here? Is the problem that the file might contain binary data?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Yasumasa Suenaga-7
On Tue, 30 Mar 2021 16:18:45 GMT, Andrew Haley <[hidden email]> wrote:

>> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>>
>> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
>> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>>
>> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>>
>> For example, we can get following CPU description on AWS A1 instance after this change:
>>
>> jdk.CPUInformation {
>>   startTime = 05:28:24.506
>>   cpu = "AArch64"
>>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>>   sockets = 8
>>   cores = 8
>>   hwThreads = 8
>> }
>
> src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 178:
>
>> 176:     ssize_t read_sz = read(fd, buf, buflen - 1);
>> 177:     close(fd);
>> 178:     if ((read_sz > 0) && isgraph(*buf)) {
>
> What is `isgraph(*buf)` doing here? Is the problem that the file might contain binary data?

In case of AWS A1 2x.large, board_name has `\n` (0x0A) only. This code intent to ignore it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Andrew Haley-2
On Tue, 30 Mar 2021 23:43:47 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 178:
>>
>>> 176:     ssize_t read_sz = read(fd, buf, buflen - 1);
>>> 177:     close(fd);
>>> 178:     if ((read_sz > 0) && isgraph(*buf)) {
>>
>> What is `isgraph(*buf)` doing here? Is the problem that the file might contain binary data?
>
> In case of AWS A1 2x.large, board_name has `\n` (0x0A) only. This code intent to ignore it.

Please be more explicit. This is (perhaps obviously?) a workaround for a bug, And t.his code is itself a bug, because it doesn't check for the condition you care about. So, please check explicitly for this condition: `\n` only, and nothing else. And leave a comment explaining why.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI

Andrew Haley-2
On Wed, 31 Mar 2021 08:43:33 GMT, Andrew Haley <[hidden email]> wrote:

>> In case of AWS A1 2x.large, board_name has `\n` (0x0A) only. This code intent to ignore it.
>
> Please be more explicit. This is (perhaps obviously?) a workaround for a bug, And t.his code is itself a bug, because it doesn't check for the condition you care about. So, please check explicitly for this condition: `\n` only, and nothing else. And leave a comment explaining why.

One other thing I missed last time around: why is there a permanently-allocated 4k buffer for this code that it only used for jfr?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v2]

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>
> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>
> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>
> For example, we can get following CPU description on AWS A1 instance after this change:
>
> jdk.CPUInformation {
>   startTime = 05:28:24.506
>   cpu = "AArch64"
>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>   sockets = 8
>   cores = 8
>   hwThreads = 8
> }

Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:

  refactoring

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3259/files
  - new: https://git.openjdk.java.net/jdk/pull/3259/files/342907ef..cb413927

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=00-01

  Stats: 23 lines in 1 file changed: 10 ins; 1 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3259/head:pull/3259

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v2]

Yasumasa Suenaga-7
In reply to this post by Andrew Haley-2
On Wed, 31 Mar 2021 08:45:06 GMT, Andrew Haley <[hidden email]> wrote:

>> Please be more explicit. This is (perhaps obviously?) a workaround for a bug, And t.his code is itself a bug, because it doesn't check for the condition you care about. So, please check explicitly for this condition: `\n` only, and nothing else. And leave a comment explaining why.
>
> One other thing I missed last time around: why is there a permanently-allocated 4k buffer for this code that it only used for jfr?

I pushed new commit with some refactoring.

> Please be more explicit. This is (perhaps obviously?) a workaround for a bug, And t.his code is itself a bug, because it doesn't check for the condition you care about. So, please check explicitly for this condition: \n only, and nothing else. And leave a comment explaining why.

I've done so in new commit.

> One other thing I missed last time around: why is there a permanently-allocated 4k buffer for this code that it only used for jfr?

Some features (e.g. hs_err log, VM.info dcmd) dumps /proc/cpuinfo, so I guess this feature is used JFR only now. However CPU description has different information (for AArch64 at least), so we might be able to use it in other place, but it is out of scope this PR.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v2]

Gerard Ziemski-2
In reply to this post by Yasumasa Suenaga-7
On Wed, 31 Mar 2021 13:23:44 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>>
>> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
>> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>>
>> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>>
>> For example, we can get following CPU description on AWS A1 instance after this change:
>>
>> jdk.CPUInformation {
>>   startTime = 05:28:24.506
>>   cpu = "AArch64"
>>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>>   sockets = 8
>>   cores = 8
>>   hwThreads = 8
>> }
>
> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>
>   refactoring

2 small nitpicks, but otherwise the functionality of the code looks good to me, though I will have to trust you on using the files to get the CPU description, since I'm no Linux expert:

     "/proc/device-tree/compatible",
     "/sys/devices/virtual/dmi/id/board_name",
     "/sys/devices/virtual/dmi/id/product_name"

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 174:

> 172:   assert(buf != NULL, "invalid argument");
> 173:   assert(buflen >= 1, "invalid argument");
> 174:   int fd = open(fname, O_RDONLY);

Should be:

` int fd = os::open(fname, O_RDONLY);`

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 176:

> 174:   int fd = open(fname, O_RDONLY);
> 175:   if (fd != -1) {
> 176:     ssize_t read_sz = read(fd, buf, buflen);

Should be:

`ssize_t read_sz = os::read(fd, buf, buflen);`

-------------

Changes requested by gziemski (Committer).

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v2]

Gerard Ziemski-2
In reply to this post by Yasumasa Suenaga-7
On Wed, 31 Mar 2021 13:23:44 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>>
>> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
>> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>>
>> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>>
>> For example, we can get following CPU description on AWS A1 instance after this change:
>>
>> jdk.CPUInformation {
>>   startTime = 05:28:24.506
>>   cpu = "AArch64"
>>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>>   sockets = 8
>>   cores = 8
>>   hwThreads = 8
>> }
>
> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>
>   refactoring

src/hotspot/os_cpu/linux_aarch64/vm_version_linux_aarch64.cpp line 183:

> 181:     // (e.g. /sys/devices/virtual/dmi/id/board_name)
> 182:     if ((read_sz > 0) && (*buf != '\n')) {
> 183:       buf[read_sz - 1] = '\0';

Personally, I would move it after we process the characters. It logically belongs at the end of the processing code, just before we return "true".

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>
> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>
> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>
> For example, we can get following CPU description on AWS A1 instance after this change:
>
> jdk.CPUInformation {
>   startTime = 05:28:24.506
>   cpu = "AArch64"
>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>   sockets = 8
>   cores = 8
>   hwThreads = 8
> }

Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:

  Use functions in os class

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3259/files
  - new: https://git.openjdk.java.net/jdk/pull/3259/files/cb413927..e5b75dc5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=01-02

  Stats: 6 lines in 1 file changed: 2 ins; 1 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3259/head:pull/3259

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v2]

Yasumasa Suenaga-7
In reply to this post by Gerard Ziemski-2
On Mon, 5 Apr 2021 18:14:44 GMT, Gerard Ziemski <[hidden email]> wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   refactoring
>
> 2 small nitpicks, but otherwise the functionality of the code looks good to me, though I will have to trust you on using the files to get the CPU description, since I'm no Linux expert:
>
>      "/proc/device-tree/compatible",
>      "/sys/devices/virtual/dmi/id/board_name",
>      "/sys/devices/virtual/dmi/id/product_name"

Thanks @gerard-ziemski for the comment!

I used `os::open()`, `os::read()`, and `os::close()` in new commit.
And also I moved `buf[read_sz - 1]` to just before `return true`.

> 2 small nitpicks, but otherwise the functionality of the code looks good to me, though I will have to trust you on using the files to get the CPU description, since I'm no Linux expert:
>
> ```
>  "/proc/device-tree/compatible",
>  "/sys/devices/virtual/dmi/id/board_name",
>  "/sys/devices/virtual/dmi/id/product_name"
> ```

They are suggested in https://github.com/openjdk/jdk/pull/2759#issuecomment-789391247

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Gerard Ziemski-2
In reply to this post by Yasumasa Suenaga-7
On Tue, 6 Apr 2021 01:22:48 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>>
>> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
>> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>>
>> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>>
>> For example, we can get following CPU description on AWS A1 instance after this change:
>>
>> jdk.CPUInformation {
>>   startTime = 05:28:24.506
>>   cpu = "AArch64"
>>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>>   sockets = 8
>>   cores = 8
>>   hwThreads = 8
>> }
>
> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>
>   Use functions in os class

Marked as reviewed by gziemski (Committer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Yasumasa Suenaga-7
On Tue, 6 Apr 2021 14:58:56 GMT, Gerard Ziemski <[hidden email]> wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Use functions in os class
>
> Marked as reviewed by gziemski (Committer).

Thanks @gerard-ziemski !
@theRealAph @shipilev How about this change?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Andrew Haley-2
In reply to this post by Yasumasa Suenaga-7
On Wed, 31 Mar 2021 13:31:19 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> One other thing I missed last time around: why is there a permanently-allocated 4k buffer for this code that it only used for jfr?
>
> I pushed new commit with some refactoring.
>
>> Please be more explicit. This is (perhaps obviously?) a workaround for a bug, And t.his code is itself a bug, because it doesn't check for the condition you care about. So, please check explicitly for this condition: \n only, and nothing else. And leave a comment explaining why.
>
> I've done so in new commit.
>
>> One other thing I missed last time around: why is there a permanently-allocated 4k buffer for this code that it only used for jfr?
>
> Some features (e.g. hs_err log, VM.info dcmd) dumps /proc/cpuinfo, so I guess this feature is used JFR only now. However CPU description has different information (for AArch64 at least), so we might be able to use it in other place, but it is out of scope this PR.

Looks better. I guess the problem here is that we don't know how robust we need to make this against future bugs.
Maybe we should just concatenate all three files?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Yasumasa Suenaga-7
On Wed, 7 Apr 2021 08:32:50 GMT, Andrew Haley <[hidden email]> wrote:

> Maybe we should just concatenate all three files?

Do you mean we should concatenate "compatible", "board_name", and "product_name"? It's impossible. We can see "compatible" file on the device which uses device tree, however we cannot see both "board_name" and "product_name" on it. Thus I prefer to attempt to read them sequentially.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v3]

Andrew Haley-2
On Wed, 7 Apr 2021 13:16:13 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> > Maybe we should just concatenate all three files?
>
> Do you mean we should concatenate "compatible", "board_name", and "product_name"? It's impossible.

Of course it isn't impossible. But if you really want to skip a file which is just "\n", then please check for the file being just "\n", not a file with "\n" as its first character.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v4]

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
> `jdk.CPUInformation` event on AArch64 has valid CPU description in [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491), however it does not work on UEFI booted machine.
>
> [JDK-8262491](https://bugs.openjdk.java.net/browse/JDK-8262491) refers device tree to get board name, however it does not exist on UEFI. We need to refer DMI.
> However we need to have root privilege, so we refer /sys/devices/virtual/dmi/id to avoid it.
>
> We can get board name from /sys/devices/virtual/dmi/id/board_name, but some machine set empty string to it. So we will refer /sys/devices/virtual/dmi/id/product_name as a fallback.
>
> For example, we can get following CPU description on AWS A1 instance after this change:
>
> jdk.CPUInformation {
>   startTime = 05:28:24.506
>   cpu = "AArch64"
>   description = "AArch64 a1.2xlarge  0x41:0x0:0xd08:3, simd, crc, aes, sha1, sha256"
>   sockets = 8
>   cores = 8
>   hwThreads = 8
> }

Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:

  check the contents whether it is just "\n"

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3259/files
  - new: https://git.openjdk.java.net/jdk/pull/3259/files/e5b75dc5..a1b94754

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3259&range=02-03

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3259.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3259/head:pull/3259

PR: https://git.openjdk.java.net/jdk/pull/3259
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264412: AArch64: CPU description should refer DMI [v4]

Yasumasa Suenaga-7
In reply to this post by Andrew Haley-2
On Wed, 7 Apr 2021 17:02:55 GMT, Andrew Haley <[hidden email]> wrote:

>>> Maybe we should just concatenate all three files?
>>
>> Do you mean we should concatenate "compatible", "board_name", and "product_name"? It's impossible. We can see "compatible" file on the device which uses device tree, however we cannot see both "board_name" and "product_name" on it. Thus I prefer to attempt to read them sequentially.
>
>> > Maybe we should just concatenate all three files?
>>
>> Do you mean we should concatenate "compatible", "board_name", and "product_name"? It's impossible.
>
> Of course it isn't impossible. But if you really want to skip a file which is just "\n", then please check for the file being just "\n", not a file with "\n" as its first character.

I pushed new commit to check the contents whether it is just "\n". Could you review again?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3259
12