-
-
Notifications
You must be signed in to change notification settings - Fork 603
Use CPU_METER_STEAL for 'virtualized' CPU time in non-detailed mode #1922
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
base: main
Are you sure you want to change the base?
Changes from all commits
d77fae9
c4d9978
c076c65
7b8233f
b04df08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -370,8 +370,14 @@ double Platform_setCPUValues(Meter* this, unsigned int cpu) { | |
| v[CPU_METER_IOWAIT] = cpuData->ioWaitPeriod / total * 100.0; | ||
| } else { | ||
| v[CPU_METER_KERNEL] = cpuData->systemAllPeriod / total * 100.0; | ||
| v[CPU_METER_IRQ] = (cpuData->stealPeriod + cpuData->guestPeriod) / total * 100.0; | ||
| this->curItems = 4; | ||
| this->curItems = 3; | ||
|
|
||
| v[CPU_METER_IRQ] = 0.0; // Accounted in 'kernel' | ||
| v[CPU_METER_SOFTIRQ] = 0.0; // Accounted in 'kernel' | ||
| v[CPU_METER_STEAL] = (cpuData->stealPeriod + cpuData->guestPeriod) / total * 100.0; | ||
| if (settings->accountGuestInCPUMeter) { | ||
| this->curItems = 6; | ||
| } | ||
|
Comment on lines
+373
to
+380
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-detailed virtualized CPU mapping is currently dropped/misreported after moving it to You now write virtualized time to
Based on the provided CPUMeter display snippet ( 📍 Affects 2 files
|
||
| } | ||
|
|
||
| percent = sumPositiveValues(v, this->curItems); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -574,9 +574,15 @@ static double Platform_setOneCPUValues(Meter* this, const Settings* settings, pm | |||||||||||
| v[CPU_METER_IOWAIT] = values[CPU_IOWAIT_PERIOD].ull / total * 100.0; | ||||||||||||
| } else { | ||||||||||||
| v[CPU_METER_KERNEL] = values[CPU_SYSTEM_ALL_PERIOD].ull / total * 100.0; | ||||||||||||
| this->curItems = 3; | ||||||||||||
|
|
||||||||||||
| v[CPU_METER_IRQ] = 0.0; | ||||||||||||
| v[CPU_METER_SOFTIRQ] = 0.0; | ||||||||||||
| value = values[CPU_STEAL_PERIOD].ull + values[CPU_GUEST_PERIOD].ull; | ||||||||||||
| v[CPU_METER_IRQ] = value / total * 100.0; | ||||||||||||
| this->curItems = 4; | ||||||||||||
| v[CPU_METER_STEAL] = value / total * 100.0; | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "value" here is not "steal" time though - its the sum of steal+guest now. So this hasn't really solved things for your unmerged graphing PR. IOW, there's a problem switching on/off detailed CPU mode because the historical data in the steal slot transitions between sum/not-sum/sum/not-sum ... so the "steal" value is incorrect whenever detailed mode changes state.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't get it… What are you suggesting? I think the historical data having "sum/not-sum/sum/not-sum", will not present any visual display glitch. Because "steal" and "guest" are presented as the same color in all color schemes of htop
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | Because "steal" and "guest" are presented as the same color Good point, it'll work by luck until someone wants to separate out those categories like in most tools.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I won't call it working "by luck", because I can anticipate that the "steal" and "guest" colors be separate like this: [CPU_STEAL] = A_BOLD | ColorPair(Cyan, Black),
[CPU_GUEST] = ColorPair(Cyan, Black),But then, how is it wrong by merging the "steal" and "guest" into a single item in the non-detailed CPU meter mode? Isn't that the whole point of the non-detailed mode? That the unnecessary details of CPU time items can be merged (just like "irq" and "sort-irq" merged with "kernel")? I really don't get what you are suggesting.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | I won't call it working "by luck", It is just luck - we very rarely have two items sharing a color in a Meter. | But then, how is it wrong by merging the "steal" and "guest" into a single item Its not wrong - it just remains obfuscated - like the existing code has always been, overloading one CPU values slot with multiple values from another. The bug can be fixed by a trivial change compared to this proposal, so its a nack for me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would claim the use of IRQ slot for virtual CPU time is plain wrong despite your fix looks trivial. The CPU meter code has fixed ("fixing" for the "making things unmovable" meaning) that item/slot as #714 relies on an assumption that the meanings of item indices of a meter to remain stable. It's also so for the meter character allocation ( |
||||||||||||
| if (settings->accountGuestInCPUMeter) { | ||||||||||||
| this->curItems = 6; | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+583
to
+585
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Remove braces around this trivial single-line This block is a single assignment and should follow the repository control-flow style. Suggested patch- if (settings->accountGuestInCPUMeter) {
- this->curItems = 6;
- }
+ if (settings->accountGuestInCPUMeter)
+ this->curItems = 6;As per coding guidelines, "Omit braces around simple single statements (return, break, continue, trivial assignments); never put control flow body on same line as condition; if any block in an if/else chain needs braces, all blocks get braces". 📝 Committable suggestion
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| percent = sumPositiveValues(v, this->curItems); | ||||||||||||
|
|
||||||||||||
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.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Drop braces for the single-statement
if.Use brace-less form here to match project control-flow style for trivial single statements.
Suggested patch
As per coding guidelines, "Omit braces around simple single statements (return, break, continue, trivial assignments); never put control flow body on same line as condition; if any block in an if/else chain needs braces, all blocks get braces".
📝 Committable suggestion