-
Notifications
You must be signed in to change notification settings - Fork 182
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
Conversation
6d1e9a9
to
151c79a
Compare
LGTM any chance to add a test or two to exercise this code? |
d2d4d98
to
524a3b3
Compare
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! |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
09df61e
to
1b877ca
Compare
1b877ca
to
d63f40c
Compare
Add tests for files in topology directory for offline CPUs Signed-off-by: Kishen V <kishen.viswanathan@ibm.com>
d63f40c
to
e576a6a
Compare
Fixes: #361
Added check to fetch the physical_package_id/core_id after determining if the CPU is online.