Use CPU_METER_STEAL for 'virtualized' CPU time in non-detailed mode#1922
Use CPU_METER_STEAL for 'virtualized' CPU time in non-detailed mode#1922Explorer09 wants to merge 5 commits into
Conversation
| 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; |
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
Good point, it'll work by luck until someone wants to separate out those categories like in most tools.
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.
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
| 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.
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 CPU_METER_IRQ and it shouldn't be overloaded with another use such as "virtualized". It breaks forward compatibility - such as with the not-yet-merged #714.
#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 (|#*@$%&.) in the monochrome mode. It would be more of a UX surprise when the @ character in CPU meter suddenly means different things with the "detailed CPU time" toggle. I would rather have "virtual" CPU time permanently represented by % and & and document such in the manual.
|
| It breaks forward compatibility - such as with the not-yet-merged #714 Feel free to queue your proposed change up in that PR then - if that's ever merged, that'd be an appropriate time to add this complexity. In the meantime its not warranted. |
I already proposed this PR for the exact problem. I don't need to file another one. And this PR effectively supercedes your PR (if I make the changes on top of your PR they would effectively revert your changes anyway). |
|
OK, sounds good. |
Explorer09
force-pushed
the
cpu-meter-items
branch
from
53868d2 to
b108b1f
Compare
Explorer09
changed the title
Sigh... I've told that this PR (#1922) requires reverting #1921 in order to build on top of it. What's the silly management decision that decided to merge #1921 first? |
Explorer09
force-pushed
the
cpu-meter-items
branch
3 times, most recently
from
6f72b42 to
b7eb686
Compare
Explorer09
force-pushed
the
cpu-meter-items
branch
3 times, most recently
from
c8b6017 to
daf01bd
Compare
Explorer09
force-pushed
the
cpu-meter-items
branch
3 times, most recently
from
fae0cc2 to
18d95a2
Compare
Explorer09
force-pushed
the
cpu-meter-items
branch
2 times, most recently
from
820a417 to
6cc70ef
Compare
Explorer09
force-pushed
the
cpu-meter-items
branch
from
6cc70ef to
78d5fc4
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughThis PR routes “virtualized” CPU time to Non-detailed CPU meter mapping
Meter/help rendering
Code hygiene / change flowThe implementation stays logically clean by splitting responsibilities by file: UI/help text + rendering ( WalkthroughThis PR consolidates CPUMeter attributes into a single array (now including CPU_IOWAIT), removes runtime switching between detailed/summary attribute sets, updates actionHelp to render virtualization markers based on CRT_colorScheme (monochrome "/-/-/", others "/"), and aligns platform CPU value assignment (Linux, NetBSD, OpenBSD, PCP) so non-detailed mode zeros IRQ/softIRQ, places steal+guest into STEAL, and adjusts curItems counts accordingly. Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e70bc258-53d5-4f74-badd-92edcf46b298
📒 Files selected for processing (6)
Action.cCPUMeter.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Platform.c
💤 Files with no reviewable changes (1)
- CPUMeter.c
Explorer09
force-pushed
the
cpu-meter-items
branch
from
78d5fc4 to
cec7965
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3607d072-290d-400a-bb4c-78db2a41ee41
📒 Files selected for processing (6)
Action.cCPUMeter.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Platform.c
💤 Files with no reviewable changes (1)
- CPUMeter.c
| if (settings->accountGuestInCPUMeter) { | ||
| this->curItems = 6; | ||
| } |
There was a problem hiding this comment.
🛠️ 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
- 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (settings->accountGuestInCPUMeter) { | |
| this->curItems = 6; | |
| } | |
| if (settings->accountGuestInCPUMeter) | |
| this->curItems = 6; |
| if (settings->accountGuestInCPUMeter) { | ||
| this->curItems = 6; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove braces around this trivial single-line if.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (settings->accountGuestInCPUMeter) { | |
| this->curItems = 6; | |
| } | |
| if (settings->accountGuestInCPUMeter) | |
| this->curItems = 6; |
When both the options "Add guest time in CPU meter percentage" and "Detailed CPU time" are turned off, the CPU meter used to show the "virtual" CPU time in the bar display as a glitch. Now fix it. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
Explorer09
force-pushed
the
cpu-meter-items
branch
from
cec7965 to
e4584b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f069663-1ccc-44a0-930e-18ec6d4a5b17
📒 Files selected for processing (6)
Action.cCPUMeter.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Platform.c
💤 Files with no reviewable changes (1)
- CPUMeter.c
| 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; | ||
| } |
There was a problem hiding this comment.
Non-detailed virtualized CPU mapping is currently dropped/misreported after moving it to CPU_METER_STEAL.
You now write virtualized time to CPU_METER_STEAL, but default curItems = 3 excludes that slot from sumPositiveValues() and bar rendering, and the non-detailed CPUMeter text path still reads CPU_METER_IRQ for vir:. Result: virtualized time is either hidden or shown as 0.0 in non-detailed mode.
linux/Platform.c#L373-L380: include the STEAL slot in non-detailedcurItemswhenever virtualized time is populated, so totals/bar include it.pcp/Platform.c#L577-L585: apply the samecurItemsfix so non-detailed totals/bar do not drop STEAL.- Also update the non-detailed CPUMeter consumer (
vir:source index) to read from the same slot you now populate for virtualized time.
Based on the provided CPUMeter display snippet (CPUMeter.c:152-193), non-detailed vir: still consumes CPU_METER_IRQ.
📍 Affects 2 files
linux/Platform.c#L373-L380(this comment)pcp/Platform.c#L577-L585
Explorer09
force-pushed
the
cpu-meter-items
branch
from
e4584b9 to
234bcce
Compare
The "curItems" member of the CPUMeter will be set to not larger than necessary. (Note: NetBSD and OpenBSD code currently do not support "softirq", "steal", "guest" and "iowait" times.) The meter values of "softirq", "steal", "guest" and "iowait" are now unconditionally set to 0.0 on update, just to be safe. No user-visible change in behavior is intended. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
…d mode" This reverts commit 47aeb0a.
The time displayed is (steal+guest), not just guest CPU time. Regression from 3d8fa0b Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
Linux and PCP platform code used to write the 'virtualized' (steal+guest) CPU time into the CPU_METER_IRQ item when the "detailed CPU time" option is off, which would result in a wrong color painted for virtual CPU time in bar mode. Write the 'virtualized' CPU time to CPU_METER_STEAL instead, which is more appropriate for the job. Also update the "virtualized" CPU meter item in the help screen: (1) The color is now CPU_STEAL for consistency. (2) In monochrome mode, two dummy items are displayed before the "virtualized" word so that the bar meter symbol mapping is correct. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
Explorer09
force-pushed
the
cpu-meter-items
branch
from
234bcce to
b04df08
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
linux/Platform.c (1)
373-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-detailed CPU meter undercounts virtualized time due
curItemsrange mismatch. Both implementations now write virtualized CPU toCPU_METER_STEAL(index 5), but defaultcurItems = 3truncates aggregation/rendering to indices 0..2.
linux/Platform.c#L373-L380: set non-detailedcurItemshigh enough to include index 5 wheneverCPU_METER_STEALis populated (typically6), keeping IRQ/SOFTIRQ as zero-filled placeholders.pcp/Platform.c#L577-L585: apply the samecurItemsrule sosumPositiveValues()and bar rendering include virtualized CPU in non-detailed mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7464b53e-84f6-4eb8-8551-9489b62c7b63
📒 Files selected for processing (6)
Action.cCPUMeter.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Platform.c
2 participants
The virtualized CPU time will now write to the
CPU_METER_STEALitem in the non-detailed CPU meter mode. Also fix the CPU meter text in the help screen.This pull request also includes code cleanups such as unnecessary assignments of
v[CPU_METER_IRQ]in NetBSD and OpenBSD platforms.I think my approach is better than what's proposed in #1921.
Resolves #1920.