Re: [pve-devel] [PATCH pve-manager 2/3] Fix #5708: Add CPU raw counters

[Sascha Westermann via pve-devel <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 14165 bytes --]

[-- Attachment #1.1.1: Type: text/plain, Size: 2315 bytes --]

If the data is also to be processed by external metric servers, I think
the integration in `PVE::ProcFSTools::read_proc_stat()` makes sense. The
term `cpustat` would no longer conflict in this case, as the content
would be virtually the same. I would create a new patch series for this,
but when looking at `read_proc_stat` a few questions arose for me:

> sub read_proc_stat {
>     my $res = { user => 0, nice => 0, system => 0, idle => 0 , iowait => 0, irq => 0, softirq => 0, steal => 0, guest => 0, guest_nice => 0, sum => 0};

In order to remain consistent with the structure, the same fields per
CPU would also need to be initialized with 0. However, this is only
done when I find an entry of the form cpu<num>, which implicitly gives
me values - so the initialization would not do anything. In this
context, I wonder whether there are any situations where no values can
be set? I would actually say that this is not the case and that the
initialization can be removed.

Side note: `sum` is not used, it probably means `total`, right?

>    if (my $fh = IO::File->new ("/proc/stat", "r")) {
>	while (defined (my $line = <$fh>)) {
>	    if ($line =~ m|^cpu\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)(?:\s+(\d+)\s+(\d+))?|) {
>		$res->{user} = $1 - ($9 // 0);
>		$res->{nice} = $2 - ($10 // 0);
>		$res->{system} = $3;

`user` contains `guest` and `nice` contains `guest_nice`, so I
understand that the values are subtracted. Wouldn't it be better to use
the original values here? Especially when these are passed out as raw
values via API, it would certainly be helpful if they correspond 1:1
with the documentation from /proc/stat.

The values are output as a string in the JSON output. Is there anything
against casting them to int?

>    my $diff = ($ctime - $last_proc_stat->{ctime}) * $clock_ticks * $cpucount;
>
>    if ($diff > 1000) { # don't update too often

I don't understand the condition. `$ctime - $last_proc_stat->{ctime}`
corresponds to the elapsed time in seconds as a float, `clock_ticks`
would normally be 100. So that would mean that on a system with one CPU
core an update may only take place every 10 seconds, but with 8 cores
every 1.25 seconds? Is that an error? Is it even necessary to suppress
updates if $diff > 0?

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel