From: Daniel Kral <d.kral@proxmox.com>
To: Sascha Westermann <sascha.westermann@hl-services.de>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-manager 2/3] Fix #5708: Add CPU raw counters
Date: Thu, 3 Oct 2024 11:40:47 +0200 [thread overview]
Message-ID: <88cc9386-c892-4070-9483-da4f187d909b@proxmox.com> (raw)
In-Reply-To: <63c737f2-21cd-4fff-bf86-2369de65f886@hl-services.de>
On 9/30/24 08:17, Sascha Westermann wrote:
> 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.
Those fields seem to be initialized to zero here, as they are summed
later for `$res->{total}` [0]. If the `/proc/stat` file couldn't be read
or for some reason no `cpu` was captured, those would be initialized to
`undef`. As far as I'm aware, an addition with a `undef` value would
convert it to `0` in numerical contexts, so removing it shouldn't be a
problem, but I could also be missing something here.
---
At least for me, I think you wouldn't need to initialize the `cpu<num>`
fields with zero, but just make sure that any capture group that uses a
prefixed `?` could become `undef` [1], so that case needs to be handled,
just like `$res->{guest}` and `$res->{guest_nice}` is.
>
> Side note: `sum` is not used, it probably means `total`, right?
As far as I'm aware, I couldn't find a usage for the `sum` property
anywhere the `read_proc_stat` is used and could be deleted.
>
>> 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.
I can only point you in the direction of the original author's commit,
it seems like it was inspired by other monitoring tools, but I haven't
checked the latter two references in the commit message [2].
I get your point, but there might already be many (external) users,
which depend on these values as they are calculated here (thinking of
InfluxDB/Grafana dashboards, API users, ...). So if it doesn't hurt your
use case too badly (and others that would use your new data points
too!), I would adopt that for the `cpu<num>` datapoints as well.
>
> The values are output as a string in the JSON output. Is there anything
> against casting them to int?
Good catch! It would be great if any numerical value is also treated
like that. Be aware that Perl doesn't really have an explicit way to
'cast' variables as types are handled by context. The builtin `int`
function only makes sure that we only get the integer part of an
expression [3]. So be aware, but if it doesn't break something (or
change probable existing expectations for API users), go ahead.
>
>> 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?
Unfortunately, as some other things here, these changes predate our git
repository and I couldn't tell you exactly why it is that way.
As far as I can say, as this is retrieved from the WebGUI's node status
page around every second, this seems like it should "buffer" changes to
these two data points, so that there are no spikes for the cpu% and
wait%. But I don't quite get why it is multiplied with `$clock_ticks`
and `$cpu_count` either.
If you really want to change this, I would do that in another patch
(series) and mark it as an "RFC" (Read for Comments), so it gets a
little more discussion if that change conflicts with anyone.
---
Hope these remarks clear up some problems. Looking forward to your patches!
Cheers,
Daniel
[0] https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=5224b31b
[1] https://perldoc.perl.org/perlvar#$%3Cdigits%3E-($1,-$2,-...)
[2] https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=c140206b
[3] https://perldoc.perl.org/functions/int
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2024-10-03 9:41 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240917055020.10507-1-sascha.westermann@hl-services.de>
2024-09-17 5:50 ` [pve-devel] [PATCH pve-common 1/3] " Sascha Westermann via pve-devel
2024-09-17 5:50 ` [pve-devel] [PATCH pve-manager 2/3] " Sascha Westermann via pve-devel
2024-09-24 12:25 ` Daniel Kral
2024-09-24 14:00 ` Lukas Wagner
2024-09-30 6:17 ` Sascha Westermann via pve-devel
[not found] ` <63c737f2-21cd-4fff-bf86-2369de65f886@hl-services.de>
2024-10-03 9:40 ` Daniel Kral [this message]
2024-09-17 5:50 ` [pve-devel] [PATCH qemu-server 3/3] " Sascha Westermann via pve-devel
2024-09-24 12:25 ` Daniel Kral
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=88cc9386-c892-4070-9483-da4f187d909b@proxmox.com \
--to=d.kral@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=sascha.westermann@hl-services.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox