Skip to content

Conversation

glimchb
Copy link
Contributor

@glimchb glimchb commented Mar 29, 2023

Authors

Co-authored by: Xingyou Chen rockrush@rockwork.org

Details

  • collected snapshot data from my EC2 aarch64 instance
  • Add new const for /sys/devices/system/cpu
  • Cherry-picked Add aarch64 and RISCV64 support #303
  • Fixed review comments
  • added tests based on ARM snapshot

Tests

tested on 8 core machine:

$ uname -m
aarch64

$ go run cmd/ghwc/main.go cpu
cpu (1 physical package, 8 cores, 8 hardware threads)
 physical package #36 (8 cores, 8 hardware threads)
  processor core #0 (1 threads), logical processors [0]
  processor core #1 (1 threads), logical processors [1]
  processor core #2 (1 threads), logical processors [2]
  processor core #3 (1 threads), logical processors [3]
  processor core #4 (1 threads), logical processors [4]
  processor core #5 (1 threads), logical processors [5]
  processor core #6 (1 threads), logical processors [6]
  processor core #7 (1 threads), logical processors [7]
  capabilities: [sha1 sha2 crc32 cpuid

collected snapshot data from my EC2 aarch64 instance
we can use it for running tests

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

@ffromani this could help with #199 and #303

Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

@ffromani would you mind unpacking this tarball and giving it a once over to make sure it's all good to include? @glimchb do you want to add some test cases that use this snapshot?

@ffromani
Copy link
Collaborator

@jaypipes sure thing! I'll have a look and comment here ASAP. Thanks @glimchb to offer this

@jaypipes
Copy link
Owner

@jaypipes sure thing! I'll have a look and comment here ASAP. Thanks @glimchb to offer this

@ffromani grazie mille!

@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

@glimchb do you want to add some test cases that use this snapshot?

@jaypipes the CPU tests will fail until we merged #303 WDYT ? we can add other tests...

@jaypipes
Copy link
Owner

@jaypipes the CPU tests will fail until we merged #303 WDYT ? we can add other tests...

It's a shame that @rockrush did not want to respond to our PR review comments on that one. @glimchb what do you think about creating a PR that combines your snapshot addition with #303, addresses the PR reviews, and adds some simple unit tests for AARCH64 platforms?

@ffromani
Copy link
Collaborator

ffromani commented Mar 29, 2023

My 2c: the snapshot provided is surely a nice idea, and we will carefully evaluate it. I for myself would prefer a developer-accessible environment, because this seems an area on which we should expect some active development.

Unfortunately this is a bit of a busy period for me, but I'll get back and comment ASAP anyway.

EDIT: reportedly gh actions will add arm64 support "in the future" (actions/runner-images#5631 (comment)). I'm seriously considering adding a workaround using QEMU (was also suggested in the past IIRC) as stopgap solution.

EDIT2: what I'm trying to say is: for amd64, we pretty much all have access to "basic" machines with the most common stuff. Using snapshot is great to test exotic configs or to prevent regressions. But as complement to having easy access to machines to develop. Depending only on snapshot (arm64) is suboptimal. Sure, it's a step forward, but we still have major maintainability concerns. Note this is my own take with my maintainer hat on, this does not represent the project vision.

@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

@glimchb what do you think about creating a PR that combines your snapshot addition with #303, addresses the PR reviews, and adds some simple unit tests for AARCH64 platforms?

Sure, I can do that for sure if we agree on the strategy - snapshot vs qemu or both...
Another approach I used so far is run 1 single ci job for ARM64 on https://www.travis-ci.com/

@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

@ffromani not sure we can use QEMU for our use acse due to actions/runner-images#183
I'm using a lot of qemu for multi platform docker builds but we need arm kernel not just instruction set emulation to see lscpu and /proc/cpuinfo correctly...

glimchb and others added 2 commits March 29, 2023 19:47
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
Signed-off-by: Xingyou Chen <rockrush@rockwork.org>
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb glimchb marked this pull request as draft March 29, 2023 16:49
- use filepath.Join() to build paths
- handle error from ReadDir()

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb glimchb changed the title Add aarch64 Neoverse-N1 from AWS EC2 Add aarch64 support Mar 29, 2023
Code review comment fix from jaypipes#303

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

@jaypipes @ffromani address all comments and broke to smaller commits, please review

@glimchb glimchb marked this pull request as ready for review March 29, 2023 17:40
Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

This is looking much better, thank you @glimchb!

One more suggestion inline for you. Also, if you might edit your PR summary and put the following in there:

Co-authored by: Xingyou Chen [rockrush@rockwork.org](mailto:rockrush@rockwork.org)

I'm sure @rockrush would appreciate that!

Best,
-jay

@glimchb
Copy link
Contributor Author

glimchb commented Mar 29, 2023

One more suggestion inline for you. Also, if you might edit your PR summary and put the following in there:
I'm sure @rockrush would appreciate that!

Absolutely. Done, thanks for suggestion.
The commit 34f45f4 was cherry-picked, so it has proper Signed-off-by: Xingyou Chen <rockrush@rockwork.org>
But as you suggested, also added this to PR description

Code review comment fix

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb glimchb requested a review from jaypipes March 29, 2023 18:05
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you so much @glimchb for your very timely PR and tests! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants