Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262491: AArch64: CPU description should contain compatible board list #2759

Closed
wants to merge 8 commits into from

Conversation

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Feb 27, 2021

HotSpot generates CPU description when it is started. We can see it jdk.CPUInformation JFR event as below:

$ jfr print --events jdk.CPUInformation raspi4.jfr
jdk.CPUInformation {
  startTime = 22:57:13.521
  cpu = "AArch64"
  description = "AArch64 0x41:0x0:0xd08:3, simd, crc"
  sockets = 4
  cores = 4
  hwThreads = 4
}

description contains "AArch64", it is fixed value, we cannot guess the process was run on what machine (SoC).

In Linux, we can use compatibleproperty in device tree to guess the machine. The 'compatible' property contains a sorted list of strings starting with the exact name of the machine, followed by an optional list of boards it is compatible with sorted from most compatible to least.

After this change, we can get the description as below:

jdk.CPUInformation {
  startTime = 00:32:49.767
  cpu = "AArch64"
  description = "raspberrypi,4-model-b brcm,bcm2711 0x41:0x0:0xd08:3, simd, crc"
  sockets = 4
  cores = 4
  hwThreads = 4
}

In Linux on AMD64, we can see as following, then we can guess the CPU model from it. The same should do for AArch64.

jdk.CPUInformation {
  startTime = 17:28:03.907
  cpu = "AMD (null) (HT) SSE SSE2 SSE3 SSSE3 SSE4.1 SSE4.2 SSE4A AMD64"
  description = "Brand: AMD Ryzen 3 3300X 4-Core Processor , Vendor: AuthenticAMD
Family: <unknown> (0x17), Model: <unknown> (0x71), Stepping: 0x0
Ext. family: 0x8, Ext. model: 0x7, Type: 0x0, Signature: 0x00870f10
Features: ebx: 0x01020800, ecx: 0xfed83203, edx: 0x178bfbff
Ext. features: eax: 0x00870f10, ebx: 0x20000000, ecx: 0x004003f3, edx: 0x2fd3fbff
Supports: On-Chip FPU, Virtual Mode Extensions, Debugging Extensions, Page Size Extensions, Time Stamp Counter, Model Specific Registers, Physical Address Extension, Machine Check Exceptions, CMPXCHG8B Instruction, On-Chip APIC, Fast System Call, Memory Type Range Registers, Page Global Enable, Machine Check Architecture, Conditional Mov Instruction, Page Attribute Table, 36-bit Page Size Extension, CLFLUSH Instruction, Intel Architecture MMX Technology, Fast Float Point Save and Restore, Streaming SIMD extensions, Streaming SIMD extensions 2, Hyper Threading, Streaming SIMD Extensions 3, PCLMULQDQ, Supplemental Streaming SIMD Extensions 3, Fused Multiply-Add, CMPXCHG16B, Streaming SIMD extensions 4.1, Streaming SIMD extensions 4.2, MOVBE, Popcount instruction, AESNI, XSAVE, OSXSAVE, AVX, F16C, LAHF/SAHF instruction support, Core multi-processor leagacy mode, Advanced Bit Manipulations: LZCNT, SSE4A: MOVNTSS, MOVNTSD, EXTRQ, INSERTQ, Misaligned SSE mode, SYSCALL/SYSRET, Execute Disable Bit, RDTSCP, Intel 64 Architecture"
  sockets = 1
  cores = 2
  hwThreads = 2
}

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262491: AArch64: CPU description should contain compatible board list

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2759/head:pull/2759
$ git checkout pull/2759

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 27, 2021

👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 27, 2021

@YaSuenag The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the hotspot label Feb 27, 2021
@YaSuenag YaSuenag marked this pull request as ready for review Feb 27, 2021
@openjdk openjdk bot added the rfr label Feb 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 27, 2021

Loading

@@ -51,7 +51,28 @@ void VM_Version_Ext::initialize_cpu_information(void) {
_no_of_threads = _no_of_cores;
_no_of_sockets = _no_of_cores;
snprintf(_cpu_name, CPU_TYPE_DESC_BUF_SIZE - 1, "AArch64");
snprintf(_cpu_desc, CPU_DETAILED_DESC_BUF_SIZE, "AArch64 %s", _features_string);

int fd = open("/proc/device-tree/compatible", O_RDONLY);
Copy link
Member

@AntonKozlov AntonKozlov Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be done here in os-independent src/hotspot/cpu/aarch64. src/hotspot/os_cpu/linux_aarch64 looks like a better place for this. Hotspot supports at least Windows/AArch64 in addition to Linux/AArch64.

Loading

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Mar 1, 2021

Many server-class AArch64 machines use ACPI instead of Device Tree so won't have /proc/device-tree.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 1, 2021

Many server-class AArch64 machines use ACPI instead of Device Tree so won't have /proc/device-tree.

What file should we refer to detect SoC on server-class machine? Can we detect SoC in same way? (e.g. sysfs)
If we cannot implement it in same way, I want to fix it for device tree at first.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 1, 2021

I refactored the change to separate Linux AArch64 code from common code in new commit.
If we cannot open /proc/device-tree, it fallbacks "AArch64" like current implementation.

Loading

Copy link
Member

@AntonKozlov AntonKozlov left a comment

Looks better, thanks for addressing. Please consider few notes from someone not in the reviewer role.

Loading

@@ -167,3 +167,31 @@ void VM_Version::get_os_cpu_info() {
fclose(f);
}
}

void VM_Version::get_compatible_board(char *buf, int buflen) {
const char *aarch64_label = "AArch64";
Copy link
Member

@AntonKozlov AntonKozlov Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All platforms seem to declare themselves AArch64, this probably can be in the shared aarch64 code.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VM_Version is not inherited, and platform-specific functions are declared in os_linux and os_windows. So I think it is difficult to declare shared (default) function for this purpose.
It is the best if we declare shared function, and override it like a virtual function. But it seems to be difficult. Do you have any idea?

Loading

Copy link
Member

@AntonKozlov AntonKozlov Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can assume this function to optionally return a board name. IMHO ideally an empty string should a valid output. Now the output from this function is the prefix and the features string is the suffix of the complete string CPU description. What if we start with the constant AArch64 prefix in the shared CPU code, append output from this function, and then append the features string?

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean VM_Version::get_compatible_board() should return NULL instead of AArch64 to add it as a prefix at the caller?
I don't want to do so because we should return board name as possible like a x86.

In Windows AArch64, it seems to have SoC name in registry. Now I do not have Windows AArch64, so I just set "AArch64" for it, but I hope someone work for it in future.
https://stackoverflow.com/questions/60588765/how-to-get-cpu-brand-information-in-arm64

Loading

Copy link
Member

@AntonKozlov AntonKozlov Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant NULL or just "". Got it, you mean "AArch64" to be a placeholder. But description now starts with AArch64 prefix, probably worth to keep it. Although it's fine in the current implementation.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added "AArch64" as a prefix in CPU description in new commit.
We can see it as following:

jdk.CPUInformation {
  startTime = 11:51:44.147
  cpu = "AArch64"
  description = "AArch64 raspberrypi,4-model-b brcm,bcm2711 0x41:0x0:0xd08:3, simd, crc"
  sockets = 4
  cores = 4
  hwThreads = 4
}```

Loading

struct stat statbuf;
fstat(fd, &statbuf);
if (buflen < statbuf.st_size) {
strncpy(buf, aarch64_label, buflen);
Copy link
Member

@AntonKozlov AntonKozlov Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated multiple times in this function, please consider reorganizing the code so we certainly copy the string before return from this function.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed them in new commit.

Loading

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Mar 3, 2021

What file should we refer to detect SoC on server-class machine? Can we detect SoC in same way? (e.g. sysfs)
If we cannot implement it in same way, I want to fix it for device tree at first.

Maybe you could use the files under /sys/devices/virtual/dmi/id/ like board_vendor and board_name?

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 3, 2021

What file should we refer to detect SoC on server-class machine? Can we detect SoC in same way? (e.g. sysfs)
If we cannot implement it in same way, I want to fix it for device tree at first.

Maybe you could use the files under /sys/devices/virtual/dmi/id/ like board_vendor and board_name?

It does not exist on Raspberry Pi OS.

pi@raspberrypi:~/github-forked/jdk $ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux 10 (buster)
Release:        10
Codename:       buster
pi@raspberrypi:~/github-forked/jdk $ uname -a
Linux raspberrypi 5.10.11-v8+ #1399 SMP PREEMPT Thu Jan 28 12:14:03 GMT 2021 aarch64 GNU/Linux
pi@raspberrypi:~/github-forked/jdk $ ls /sys/devices/virtual/dmi/id/
ls: cannot access '/sys/devices/virtual/dmi/id/': No such file or directory
pi@raspberrypi:~/github-forked/jdk $ ls /sys/devices/virtual/dmi/
ls: cannot access '/sys/devices/virtual/dmi/': No such file or directory
pi@raspberrypi:~/github-forked/jdk $ ls /sys/devices/virtual/
bcm2708_vcio     block     graphics  mem   raw             rpivid-intcmem  thermal  vc-mem
bcm2835-gpiomem  devlink   input     misc  rpivid-h264mem  rpivid-vp9mem   tty      vtconsole
bdi              dma_heap  leds      net   rpivid-hevcmem  sound           vc       workqueue

Loading

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Mar 3, 2021

Maybe you could use the files under /sys/devices/virtual/dmi/id/ like board_vendor and board_name?

It does not exist on Raspberry Pi OS.

That's because the default Raspberry Pi firmware uses Device Tree. You'll only get those files if the system was booted from UEFI (x86-style firmware).

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 3, 2021

Maybe you could use the files under /sys/devices/virtual/dmi/id/ like board_vendor and board_name?

It does not exist on Raspberry Pi OS.

That's because the default Raspberry Pi firmware uses Device Tree. You'll only get those files if the system was booted from UEFI (x86-style firmware).

As I said in before comment, I want to fix it for device tree at first if we cannot refer board name in same way between devce tree and ACPI. If we cannot refer device tree, "AArch64" still uses for CPU description - it is same behavior with current implementation.

Maybe I can improve this change to refer /sys/devices/virtual/dmi/id/ if I re-install UEFI supported OS (e.g. Fedora) to my Pi 4, but I cannot do it now. So I want to work for it in another issue.

Loading

if (fd != -1) {
struct stat statbuf;
fstat(fd, &statbuf);
ssize_t read_sz = read(fd, buf, statbuf.st_size);
Copy link
Contributor

@theRealAph theRealAph Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong: the read() call should use buflen.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it in new commit.

Loading


void VM_Version::get_compatible_board(char *buf, int buflen) {
assert(buf != NULL, "invalid argument");
*buf = '\0';
Copy link
Contributor

@theRealAph theRealAph Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong too: it should check buflen.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added assert for buflen in new commit.

Loading

}
close(fd);
}
}
Copy link
Contributor

@theRealAph theRealAph Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still wrong. If the read() call has filled the whole buffer, the string is not zero-terminated when this function returns.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I set \0 to the tail of buf at L178.

Loading

Copy link
Contributor

@theRealAph theRealAph Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I set \0 to the tail of buf at L178.

Which you then overwrite on Line 183. You really do need to move Line 174 to the end.
I ran a test with the read fulling the whole buffer.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment! I pushed new commit.

I issue read() with buflen - 1 bytes, then I set \0' to tail of bufafterread(). And also \0will be converted to blank will happen whenread()` return 1 or greater. How about it?

Loading

Copy link
Contributor

@theRealAph theRealAph Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need this to cope with errors, empty files, and so on:

void VM_Version::get_compatible_board(char *buf, int buflen) {
  assert(buf != NULL, "invalid argument");
  assert(buflen >= 1, "invalid argument");
  *buf = '\0';
  int fd = open("/proc/device-tree/compatible", O_RDONLY);
  ssize_t read_sz = read(fd, buf, buflen - 1);
  if (read_sz >= 0) {
    buf[read_sz] = '\0';
    // Replace '\0' to ' '                                                                                                                                                                                                                                                                   
    for (char *ch = buf; ch < buf + read_sz; ch++) {
      if (*ch == '\0') {
        *ch = ' ';
      }
    }
  } else { // read() retuned an error                                                                                                                                                                                                                                                        
    buf[0] = '\0';
  }
=>close(fd);
}

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated PR, but it is different from your suggestion a bit. This version would return empty string when open() or read() returns error, and also close() would be called when open() succeeded.

Did you say we can call read() and close() even if open() failed? According to manpage, it seems to work (just return EBADF), but I feel strange a bit. If it is ok, I will do so.

Loading

Copy link
Contributor

@theRealAph theRealAph Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if open() fails we should return straight away, with an empty string. That needs an addition.
We must, however, terminate the string with 0 at the correct point, at the end of the bytes read. Otherwise strlen()reads uninitialized memory. If the read() fails, we must return an empty string.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my latest commit includes your suggestion:

  • returns empty string ( \0 ) when open() failed.
  • returns empty string when read() failed or read nothing (returns 0 )
  • add \0 to buf[read_sz] just after read() call, and skip it at the loop - it can be assumed \0 is set to tail of buf

Or should I change as following for readability?

int fd = open("/proc/device-tree/compatible", O_RDONLY);
if (fd == -1) {
  *buf = '\0';
  return;
}

ssize_t read_sz = read(fd, buf, buflen - 1);
close(fd);
if (read_sz <= 0) {
  *buf = '\0';
  return;
}

// Add '\0' to the tail
buf[read_sz] = '\0';
// Replace '\0' to ' '
for (char *ch = buf; ch < buf + read_sz; ch++) {
  if (*ch == '\0') {
    *ch = ' ';
  }
}

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested current PR manually with this Gist code, it works fine without buffer overrun.
https://gist.github.com/YaSuenag/bdc73c3540335a096fa289ead36ca8b5

Loading

Copy link
Contributor

@theRealAph theRealAph Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks right.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 10, 2021

Ping! Could you review this PR?

Loading

@theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Mar 10, 2021

Ping! Could you review this PR?

Fix the out-of-bounds memory access and I'll approve it.

Loading

}
close(fd);
}
}
Copy link
Contributor

@theRealAph theRealAph Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks right.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@YaSuenag This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8262491: AArch64: CPU description should contain compatible board list

Reviewed-by: akozlov, aph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 176 new commits pushed to the master branch:

  • aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
  • 9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
  • 15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
  • 7ed46bd: 8241716: Jpackage functionality to let users choose whether to create shortcuts
  • ... and 166 more: https://git.openjdk.java.net/jdk/compare/d7efb4cc35a7c4a2d084ed865519cbb40eeb6d51...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Mar 12, 2021
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 12, 2021

@theRealAph Thank you for the review!

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 13, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 13, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 13, 2021
@YaSuenag YaSuenag deleted the JDK-8262491 branch Mar 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 13, 2021

@YaSuenag Since your change was applied there have been 192 commits pushed to the master branch:

  • 86e4c75: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording
  • 0b68ced: 8263548: runtime/cds/appcds/SharedRegionAlignmentTest.java fails to compile after JDK-8263412
  • 43524cc: 8243455: Many SA tests can fail due to trying to get the stack trace of an active method
  • e834f99: 8263412: ClassFileInstaller can't be used by classes outside of default package
  • bf9b5fa: 8263501: compiler/oracle/TestInvalidCompileCommand.java fails with release VMs
  • 0c8350e: 8263460: DynamicArchiveRelocationTest.java fails in product VM
  • b2f7c58: 8263055: hsdb Command Line Debugger does not properly direct output for some commands
  • ecfa712: 8263326: Remove ReceiverTypeData check from serviceability/sa/TestPrintMdo.java
  • b932a62: 8263470: Consolidate copies of getClassBytes in various tests
  • 0ea48d9: 8194129: Regression automated Test '/open/test/jdk/java/awt/Window/ShapedAndTranslucentWindows/TranslucentChoice.java' fails
  • ... and 182 more: https://git.openjdk.java.net/jdk/compare/d7efb4cc35a7c4a2d084ed865519cbb40eeb6d51...master

Your commit was automatically rebased without conflicts.

Pushed as commit a528771.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants