hishamhm / htop

htop is an interactive text-mode process viewer for Unix systems. It aims to be a better 'top'.
GNU General Public License v2.0
5.84k stars 581 forks source link

Add new display option to also show CPU frequency in CPU meters. #932

Closed Arnavion closed 4 years ago

Arnavion commented 5 years ago

The option is only implemented on Linux. On other platforms, and on Linuxes that do not expose the relevant sysfs file, the frequency will be "N/A".

There is also a new option to show the CPU usage number from the meter, which defaults to true but can be disabled to hide the numeric display.


Unlike #592, this uses the per-CPU sysfs file to directly read an unsigned int. Like #592, this uses /proc/cpuinfo instead of the per-CPU sysfs scaling_cur_freq files. The latter are slow to read, as documented here.

Also it defaults to appending the frequency to the usage percentage rather than replacing it, but provides an option to replace it by hiding the usage percentage.

Open questions:

MichaIng commented 5 years ago

@Arnavion Many thanks for this!

Is there demand to hide the usage percentage and just show the CPU frequency?

Would be actually nice to show/hide each separately. For some it might be even sufficient to see the meter only without any value. I personally would enable the absolute clock rate only, not the percentage.

Is there demand for the average CPU meter to show an average frequency? (I personally don't see the point.)

An average frequency has not much value IMO. But since the method to estimate and show it should code-wise overlapping with per-core frequency and average usage, I think it would cost not much coding and some users might find it useful?

Should there be any pre-checks

If the files do not exist, perhaps instead of 0 it should show N/A which should be clear enough. Hiding the option completely in the first place may confuse as well when users find some docs about how to enable this but then the option is simply not present.

Arnavion commented 5 years ago

Is there demand to hide the usage percentage and just show the CPU frequency?

Would be actually nice to show/hide each separately. For some it might be even sufficient to see the meter only without any value. I personally would enable the absolute clock rate only, not the percentage.

Okay, I've added an "Also show CPU percentage numerically" option that defaults to true.

Is there demand for the average CPU meter to show an average frequency? (I personally don't see the point.)

An average frequency has not much value IMO. But since the method to estimate and show it should code-wise overlapping with per-core frequency and average usage, I think it would cost not much coding and some users might find it useful?

No, the average CPU usage is read directly from the kernel (first line of /proc/stat) rather than any code to calculate it from the individual CPU usage data, so calculating the average CPU frequency would be new code.

Also this new code for the average meter would have to read all the individual CPU frequencies for itself, since the average meter does not know anything about the other meters who've already obtained their own frequencies. In fact the individual meters might not even exist, and even if they do they may not have been updated before the average meter this update cycle.

So unless I'm mistaken, an average frequency is more trouble than it's worth.

Should there be any pre-checks

If the files do not exist, perhaps instead of 0 it should show N/A which should be clear enough.

Yeah, that's a good idea. I think it should still allow "0Hz" in case the CPU frequency really is 0, so I set it to show "N/A" only for errors.

Hiding the option completely in the first place may confuse as well when users find some docs about how to enable this but then the option is simply not present.

Yes, I agree it could be confusing.

MichaIng commented 5 years ago

@Arnavion

So unless I'm mistaken, an average frequency is more trouble than it's worth.

Jep, thanks to your insights I totally agree. Also as said the value of an average frequency is very limited IMO. If one is interested in current frequencies, then usually to check governor and heat related throttling actions, which can only be meaningful derived by individual core usage and frequency. Average freq could perhaps be used as some sort of CPU power usage meter, but of course much more plays a role for this, most importantly voltage...

Arnavion commented 5 years ago

Actually, there is one "global" place for collecting CPU data that knows about all the CPUs - LinuxProcessList_scanCPUTime - the same place that reads /proc/stat and stores usage data for each CPU up-front. It could be changed to also read scaling_cur_freq for each CPU and store that, and also calculate and store the average.

Arnavion commented 5 years ago

I hit a funny snag moving the code into LinuxProcessList_scanCPUTime - Every update cycle comes with a UI lag of ~0.5s as the scaling_cur_freq files are parsed on my 24-CPU machine. It is easily visible by just scrolling through the process list with the up/down arrows held down. Every update the UI freezes for 0.5s, then the cursor jumps a few positions and resumes moving smoothly until the next update.

This is because if a scaling_cur_freq file hasn't been read for 1s, the next read blocks for up to 10ms inside the kernel (20ms in that commit, but changed to 10ms in the commit below.)

In contrast, reading /proc/cpuinfo only has a max 10ms delay, since this delay is applied once for the file as a whole. So the delay does not scale with the number of CPUs on the machine.

This is not a problem in the PR code as it is currently, where the scaling_cur_freq reads happen in each individual meter's CPUMeter_updateValues. This is because the meters are updated constantly while scrolling so the files are re-read constantly while scrolling, and the kernel delay only happens if the files have not been read for some time. So paradoxically the code that "wastefully" reads the files more often is faster than the code that only reads once every second.

I think LinuxProcessList_scanCPUTime is the better place for the code from a code organization POV. But if we do want to move the code to LinuxProcessList_scanCPUTime so that each scaling_cur_freq file is read at most once per update cycle, I think the code would have to switch to parsing /proc/cpuinfo like #592 did. I'll let hishamhm decide.


Prometheus also hit this in https://github.com/prometheus/node_exporter/issues/1086 and worked around it by reading the files in parallel (inside goroutines). But this isn't trivial for htop. It currently does not use threads at all, and you can't just spawn one thread for every CPU because there can be a large number of those.

Arnavion commented 5 years ago

Even with reading scaling_cur_freq in CPUMeter_updateValues, the lag was noticeable when beginning to scroll. So I decided to switch to /proc/cpuinfo after all.

ehildenb commented 4 years ago

Just chiming in that I would really like this feature. Currently I do 2 panes, one with htop, the other with watch grep MHz /proc/cpuinfo.

Arnavion commented 4 years ago

I guess hishamhm isn't maintaining this any more, so I made my own TUI monitor and removed the CPU graphs from htop. It's not as versatile as htop is (it's written in Rust, it's unlikely to be packaged by any OS repos, etc) but it's there if anyone wants it.

MichaIng commented 4 years ago

@Arnavion @hishamhm Are you sure? Any source/reference about this? Would be very sad news as this tool is simply fantastic. However many thanks for sharing yours.

natoscott commented 4 years ago

Merged here: htop-dev/htop@6b443c5

MichaIng commented 4 years ago

Awesome. I see you forked the website as well and use a GitHub organisation and started to merge a large number of open PRs. Is this a somehow official maintenance takeover (since it seems right that hishamhm stopped maintaining it) or so far a personal project to see where it goes? I'm asking since support by the original author might help to convince distributions to switch their sources, otherwise it might be hard/longer track to get it into e.g. the Debian APT repository šŸ˜‰.

natoscott commented 4 years ago

@MichaIng full details here: https://github.com/hishamhm/htop/issues/992

MichaIng commented 4 years ago

Many thanks for organising the fork and bringing together contributors, especially distro maintainers as well for upstream switch, that all reads fantastic, besides the fast that hishamhm for some reason took himself back from the project and any contact attempts. Let's hope he'll re-join, at best the new maintainer team to keep workload shared and the project more sustainable by this.

I'm no C coder but since we ship and use htop in your own project I'll be happy to join testing and feedback once the open PR walk-through is done. Okay enought OOT here now šŸ˜‰.

natoscott commented 4 years ago

[...] I'll be happy to join testing and feedback once the open PR walk-through is done.

Please do - we've tagged 'release candidate 1' today for 3.0.0 - planning to release it soon. Thanks for the kind words and I too hope Hisham finds time to rejoin htop development!