| Summary: | Support CPU stats with RK3588 and other ARM CPUs | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] ksystemstats | Reporter: | Jonathan L Hanmann <jhanmann> |
| Component: | General | Assignee: | Plasma Bugs List <plasma-bugs> |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | CC: | ahiemstra, aykevanlaethem, janne-kde, josh, m.kurz, nate |
| Priority: | NOR | ||
| Version: | 6.2.2 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/plasma/ksystemstats/-/commit/23f8291aec1c1123e240187280174c336dc9df57 | Version Fixed In: | 6.2.4 |
| Sentry Crash Report: | |||
| Attachments: | Patch to make cpu information available | ||
wishlist doesn't seem the appropriate priority as this avoids crashes on all or many non-x86 systems. That is systems without "physical id" in `/proc/cpuinfo`. See below for a single CPU core entry from a Apple M1 Ultra: > processor : 0 > BogoMIPS : 48.00 > Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha512 asimdfhm dit uscat ilrcpc flagm ssbs sb paca pacg dcpodp flagm2 frint > CPU implementer : 0x61 > CPU architecture: 8 > CPU variant : 0x2 > CPU part : 0x028 > CPU revision : 0 It prevents a nullptr dereference in `LinuxCpuPluginPrivate::update()` when the constructor skipped all CPUs. Backtrace: > #0 __pthread_kill_implementation (threadid=281470396449024, signo=signo@entry=11, no_tid=no_tid@entry=0) at pthread_kill.c:44 > #1 0x0000fffeee2e8850 [PAC] in __pthread_kill_internal (threadid=<optimized out>, signo=11) at pthread_kill.c:78 > #2 0x0000fffeee295a00 in __GI_raise (sig=11) at ../sysdeps/posix/raise.c:26 > #3 0x0000fffeef062adc [PAC] in KCrash::defaultCrashHandler (sig=11) at /usr/src/debug/kf6-kcrash-6.7.0-1.fc40.aarch64/src/kcrash.cpp:596 > #4 0x0000fffeef104840 [PAC] in <signal handler called> () > #5 0x0000fffeef00b820 in KSysGuard::SensorObject::isSubscribed (this=this@entry=0x0) at /usr/include/c++/14/bits/unique_ptr.h:193 > #6 0x0000fffee921b5bc [PAC] in LinuxCpuObject::update (this=0x0, system=341648, user=427329, wait=15886, idle=78750572) at /usr/src/debug/ksystemstats-6.2.1-1.fc40.aarch64/plugins/cpu/linuxcpu.cpp:63 > #7 LinuxCpuPluginPrivate::update (this=0xaaab5a9eaf50) at /usr/src/debug/ksystemstats-6.2.1-1.fc40.aarch64/plugins/cpu/linuxcpuplugin.cpp:163 > #8 0x0000aaab4d535f80 [PAC] in Daemon::sendFrame() () If it actually crashes, that does raise the priority. Since you've got a patch, would you like to submit it at https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? (In reply to Nate Graham from comment #2) > If it actually crashes, that does raise the priority. > > Since you've got a patch, would you like to submit it at > https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? Sure. I will do that. My apologies for not responding more promptly to the original crash fix. I was traveling and unable to test it until returning. That is when I noticed it was incomplete (at least in my own view) since it only fixed the original crash and not the physical ID issue. (In reply to Jonathan L Hanmann from comment #3) > (In reply to Nate Graham from comment #2) > > If it actually crashes, that does raise the priority. > > > > Since you've got a patch, would you like to submit it at > > https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? > > Sure. I will do that. My apologies for not responding more promptly to the > original crash fix. I was traveling and unable to test it until returning. > That is when I noticed it was incomplete (at least in my own view) since it > only fixed the original crash and not the physical ID issue. Actually, perhaps you didn't mean me? I don't have a user setup for our gitlab being just an ordinary user? (In reply to Jonathan L Hanmann from comment #3) > (In reply to Nate Graham from comment #2) > > If it actually crashes, that does raise the priority. > > > > Since you've got a patch, would you like to submit it at > > https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? > > Sure. I will do that. My apologies for not responding more promptly to the > original crash fix. I was traveling and unable to test it until returning. > That is when I noticed it was incomplete (at least in my own view) since it > only fixed the original crash and not the physical ID issue. Actually, perhaps you didn't mean me? I don't have a user setup for our gitlab being just an ordinary user? (In reply to Jonathan L Hanmann from comment #5) > (In reply to Jonathan L Hanmann from comment #3) > > (In reply to Nate Graham from comment #2) > > > If it actually crashes, that does raise the priority. > > > > > > Since you've got a patch, would you like to submit it at > > > https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? > > > > Sure. I will do that. My apologies for not responding more promptly to the > > original crash fix. I was traveling and unable to test it until returning. > > That is when I noticed it was incomplete (at least in my own view) since it > > only fixed the original crash and not the physical ID issue. > > Actually, perhaps you didn't mean me? I don't have a user setup for our > gitlab being just an ordinary user? Nevermind, I should have spend a few more seconds looking at it. I will register and submit the patch. Sorry for the spam... (In reply to Jonathan L Hanmann from comment #6) > (In reply to Jonathan L Hanmann from comment #5) > > (In reply to Jonathan L Hanmann from comment #3) > > > (In reply to Nate Graham from comment #2) > > > > If it actually crashes, that does raise the priority. > > > > > > > > Since you've got a patch, would you like to submit it at > > > > https://invent.kde.org/plasma/ksystemstats/-/merge_requests ? > > > > > > Sure. I will do that. My apologies for not responding more promptly to the > > > original crash fix. I was traveling and unable to test it until returning. > > > That is when I noticed it was incomplete (at least in my own view) since it > > > only fixed the original crash and not the physical ID issue. > > > > Actually, perhaps you didn't mean me? I don't have a user setup for our > > gitlab being just an ordinary user? > > Nevermind, I should have spend a few more seconds looking at it. I will > register and submit the patch. Sorry for the spam... Oh my. :-( I just got this response to my email. Unfortunately, your email message to GitLab could not be processed. You are not allowed to perform this action. If you believe this is in error, contact a staff member. I thought I did it correctly and used the email addresss the GitLab system provided to me. You have to set up an account on our Gitlab instance (invent.kde.org), fork the relevant repo, and send a merge request from your fork. Very similar to the standard Github workflow. Our docs can be found here: https://community.kde.org/Infrastructure/GitLab A possibly relevant merge request was started @ https://invent.kde.org/plasma/ksystemstats/-/merge_requests/92 I am on Asahi Linux (Arch linux ARM) and ksystemstats crashes for me (using v6.2.2). With the patch in https://invent.kde.org/plasma/ksystemstats/-/merge_requests/92 it no longer crashes. There was a chat about this also in the Asahi IRC: https://oftc.irclog.whitequark.org/asahi/2024-10-29#1730241059-1730242297 Can the merge request please be reviewed/merged and backported to branch Plasma/6.2? Thanks! I see that Plasma 6.2.3 is due tomorrow (Tue 2024-11-05 according to https://community.kde.org/Schedules/Plasma_6#Future_releases) Would be nice if you can take a look at this before... (In reply to m.kurz from comment #11) > I see that Plasma 6.2.3 is due tomorrow (Tue 2024-11-05 according to > https://community.kde.org/Schedules/Plasma_6#Future_releases) Would be nice > if you can take a look at this before... I did provide the requested merge request. I believe you know that since I saw your comment on that request. It appears to be in the pipeline. I don't believe there is anything more I can do to assist or expedite this but wanted to respond just in case I was supposed to do something else. I wouldn't want to ignore something and be the delaying factor. *** Bug 494915 has been marked as a duplicate of this bug. *** A possibly relevant merge request was started @ https://invent.kde.org/plasma/ksystemstats/-/merge_requests/93 Git commit 2f0903224c21e9e1107063d3ae4adabed25f555b by Hector Martin.
Committed on 13/11/2024 at 13:12.
Pushed by redstrate into branch 'master'.
plugins/cpu: Test for the proper CPU property, skip nonexistent CPUs
Commit 478e766d19b3 regressed ksystemstats on many ARM systems, since it
mistakenly tests the "cpu" property of the CPU object (which is is the
"physical id") and not the "id" property (which is the actual CPU
number). For systems without a "physical id", this ignores all CPUs and
ends up crashing on a NULL deref later when it tries to look up CPU
numbers from /proc/stat.
Fix the incorrect check and add an extra NULL guard, so if we fail to
parse /proc/cpuinfo properly we don't crash. We only need an "id"
property to consider the CPU object valid, and that property is required
to identify the CPU in the first place, so it is the only one that needs
to be tested.
Also guard insertion into m_cpusBySystemIds if the keys do not exist
(this is what wants `cpu` and `core`). This isn't strictly necessary,
but more correct. Consumers of that already test for entries correctly
and don't segfault if missing.
Fixes: 478e766d19b3 ("plugins/cpu: Ignore invalid CPU info when reading /proc/cpuinfo")
M +10 -4 plugins/cpu/linuxcpuplugin.cpp
https://invent.kde.org/plasma/ksystemstats/-/commit/2f0903224c21e9e1107063d3ae4adabed25f555b
Git commit 23f8291aec1c1123e240187280174c336dc9df57 by Joshua Goins, on behalf of Hector Martin.
Committed on 13/11/2024 at 13:17.
Pushed by redstrate into branch 'Plasma/6.2'.
plugins/cpu: Test for the proper CPU property, skip nonexistent CPUs
Commit 478e766d19b3 regressed ksystemstats on many ARM systems, since it
mistakenly tests the "cpu" property of the CPU object (which is is the
"physical id") and not the "id" property (which is the actual CPU
number). For systems without a "physical id", this ignores all CPUs and
ends up crashing on a NULL deref later when it tries to look up CPU
numbers from /proc/stat.
Fix the incorrect check and add an extra NULL guard, so if we fail to
parse /proc/cpuinfo properly we don't crash. We only need an "id"
property to consider the CPU object valid, and that property is required
to identify the CPU in the first place, so it is the only one that needs
to be tested.
Also guard insertion into m_cpusBySystemIds if the keys do not exist
(this is what wants `cpu` and `core`). This isn't strictly necessary,
but more correct. Consumers of that already test for entries correctly
and don't segfault if missing.
Fixes: 478e766d19b3 ("plugins/cpu: Ignore invalid CPU info when reading /proc/cpuinfo")
(cherry picked from commit 2f0903224c21e9e1107063d3ae4adabed25f555b)
M +10 -4 plugins/cpu/linuxcpuplugin.cpp
https://invent.kde.org/plasma/ksystemstats/-/commit/23f8291aec1c1123e240187280174c336dc9df57
|
Created attachment 175330 [details] Patch to make cpu information available SUMMARY Left over issue after fix for crash in commit #c5bc0067b31f6c2bdee127702c6cc4717c770808. There is no no crash, and while this is great, the cpu stats are still missing. The attached patch corrects this problem but you may prefer a different solution based on your knowledge of the code. While I've tested this only on a Rock-5b my examination of the /proc/cpuinfo on an Odroid N2+ and a Raspberry Pi 4b+ indicates the same missing physical ID that is at least one source of the problem. I added more conditionals to my patch to be somewhat comprehensive and may have overdone it but it works. STEPS TO REPRODUCE 1. Build latest ksystemstats 2. Try to view cpu stats on Rock-5b running latest kernel with Noble. OBSERVED RESULT No available CPU stats. EXPECTED RESULT CPU stats to choose for graphs. SOFTWARE/OS VERSIONS Windows: macOS: (available in the Info Center app, or by running `kinfo` in a terminal window) Linux/KDE Plasma: Linux rock-5b-3 6.1.75-vendor-rk35xx KDE Plasma Version: 6.2.80 KDE Frameworks Version: 6.8.0 Qt Version: 6.7.2 ADDITIONAL INFORMATION