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

Read core and physical package ID of the CPUs that are online #362

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

kishen-v
Copy link
Contributor

Fixes: #361

Added check to fetch the physical_package_id/core_id after determining if the CPU is online.

@ffromani
Copy link
Collaborator

ffromani commented Mar 6, 2024

LGTM

any chance to add a test or two to exercise this code?

@kishen-v kishen-v force-pushed the determine-cpu-online-sts branch 4 times, most recently from d2d4d98 to 524a3b3 Compare March 14, 2024 15:43
@kishen-v
Copy link
Contributor Author

Hi @ffromani, I've added a couple of tests to check if the warning is being raised in the case of missing files (core_id/physical_package_id) in the topology directory. Could you PTAL when time permits?

Thanks!

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.

Hi @kishen-v thanks very much for your contribution! One little suggestion inline for you...

@@ -64,6 +65,10 @@ func processorsGet(ctx *context.Context) []*Processor {
continue
}

onlineFilePath := filepath.Join(paths.SysDevicesSystemCPU, fmt.Sprintf("cpu%d", lpID), onlineFile)
if util.SafeIntFromFile(ctx, onlineFilePath) == 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

this will return -1 if the file does not exist... perhaps it's worth changing this to:

if util.SafeIntFromFile(ctx, onlineFilePath) != 1 {

in order to account for when there are problems locating the /online file?

Copy link
Contributor Author

@kishen-v kishen-v Mar 21, 2024

Choose a reason for hiding this comment

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

I agree. Addressed the comment.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was observed that this change didn't fit well for the ARM architecture, as the online file does not exist under the cpuX directory. This lead to the mismatch in the expected vs observed tests results causing a breaking change.

The online file exists for the amd64/ppc64le architecture where, the corresponding files are in the topology directory may/may not exist based on the bit set in the online file. I have made changes to account only if the cpu is offline, before the files under the topology directory are checked. This doesn't seem to affect the arm64 architecture as the -1 returned from the caller doesn't affect the rest of the code execution.

Thanks!

pkg/cpu/cpu_linux.go Show resolved Hide resolved
@kishen-v kishen-v force-pushed the determine-cpu-online-sts branch 2 times, most recently from 09df61e to 1b877ca Compare March 21, 2024 05:48
Add tests for files in topology directory for offline CPUs

Signed-off-by: Kishen V <kishen.viswanathan@ibm.com>
@jaypipes jaypipes merged commit 633fd9f into jaypipes:main Apr 7, 2024
12 checks passed
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.

Retrieval of core_id and physical_package_id for CPUs that are offline.
3 participants