495524 2024-10-29 10:11:11 +0000 Support CPU stats with RK3588 and other ARM CPUs 2024-11-13 13:20:50 +0000 1 1 3 Frameworks and Libraries ksystemstats General 6.2.2 Other Linux RESOLVED FIXED NOR crash --- 1 jhanmann plasma-bugs-null ahiemstra aykevanlaethem janne-kde josh m.kurz nate https://invent.kde.org/plasma/ksystemstats/-/commit/23f8291aec1c1123e240187280174c336dc9df57 6.2.4 0 oldest_to_newest 2370136 0 175330 jhanmann 2024-10-29 10:11:11 +0000 Created attachment 175330 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 2370401 1 janne-kde 2024-10-30 07:52:01 +0000 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() () 2370477 2 nate 2024-10-30 14:57:28 +0000 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 ? 2370507 3 jhanmann 2024-10-30 15:31:28 +0000 (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. 2370511 4 jhanmann 2024-10-30 15:34:20 +0000 (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? 2370512 5 jhanmann 2024-10-30 15:34:32 +0000 (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? 2370513 6 jhanmann 2024-10-30 15:36:45 +0000 (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... 2370535 7 jhanmann 2024-10-30 15:56:43 +0000 (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. 2370612 8 nate 2024-10-30 20:33:14 +0000 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 2370617 9 bug-janitor 2024-10-30 21:07:02 +0000 A possibly relevant merge request was started @ https://invent.kde.org/plasma/ksystemstats/-/merge_requests/92 2371647 10 m.kurz 2024-11-04 09:49:16 +0000 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! 2371660 11 m.kurz 2024-11-04 11:17:06 +0000 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... 2371701 12 jhanmann 2024-11-04 14:36:41 +0000 (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. 2373330 13 m.kurz 2024-11-11 08:54:43 +0000 *** Bug 494915 has been marked as a duplicate of this bug. *** 2373940 14 bug-janitor 2024-11-13 13:06:16 +0000 A possibly relevant merge request was started @ https://invent.kde.org/plasma/ksystemstats/-/merge_requests/93 2373943 15 marcan 2024-11-13 13:17:16 +0000 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 2373944 16 josh 2024-11-13 13:20:50 +0000 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 175330 2024-10-29 10:11:11 +0000 2024-10-29 10:11:11 +0000 Patch to make cpu information available ksystemstats-fix-cpuinfo-for-arm.patch text/plain 759 jhanmann ZGlmZiAtLWdpdCBhL3BsdWdpbnMvY3B1L2xpbnV4Y3B1cGx1Z2luLmNwcCBiL3BsdWdpbnMvY3B1 L2xpbnV4Y3B1cGx1Z2luLmNwcAppbmRleCBhN2VmMGZhLi4xZWQ0ZGJmIDEwMDY0NAotLS0gYS9w bHVnaW5zL2NwdS9saW51eGNwdXBsdWdpbi5jcHAKKysrIGIvcGx1Z2lucy9jcHUvbGludXhjcHVw bHVnaW4uY3BwCkBAIC05OCw3ICs5OCwxMSBAQCBMaW51eENwdVBsdWdpblByaXZhdGU6OkxpbnV4 Q3B1UGx1Z2luUHJpdmF0ZShDcHVQbHVnaW4gKnEpCiAgICAgICAgIC8vIElmIHdlIGRvbid0IGhh dmUgYSB2YWxpZCBJRCwgY29uc2lkZXIgdGhlIGVudGlyZSBDUFUgaW5mbyB0byBiZSBpbnZhbGlk LgogICAgICAgICAvLyBUaGlzIG1haW5seSBoYXBwZW5zIHdoZW4gdGhlcmUgaXMgYWRkaXRpb25h bCBub24tY29yZSByZWxhdGVkIGRhdGEgaW4KICAgICAgICAgLy8gL3Byb2MvY3B1aW5mby4KLSAg ICAgICAgaWYgKGluZm8uY3B1ICE9IC0xKSB7CisgICAgICAgIGlmICgoaW5mby5pZCAhPSAtMSkg fHwKKyAgICAgICAgICAgIChpbmZvLmNwdSAhPSAtMSkgfHwKKyAgICAgICAgICAgIChpbmZvLmNv cmUgIT0gLTEpIHx8CisgICAgICAgICAgICAoaW5mby5mcmVxdWVuY3kgIT0gMCkgfHwKKyAgICAg ICAgICAgIChpbmZvLnNpYmxpbmdzICE9IC0xKSkgewogICAgICAgICAgICAgY3B1cy5wdXNoX2Jh Y2soaW5mbyk7CiAgICAgICAgICAgICBjcHVDb3VudCA9IHN0ZDo6bWF4KGNwdUNvdW50LCBpbmZv LmNwdSk7CiAgICAgICAgIH0K