Linux/cpuinfo.c: grow buffer size as needed for 12+ core CPUs
ClosedPublic

Authored by jakobkummerow on Feb 13 2020, 11:22 AM.

Details

Summary

Getting CPU information starts with reading /proc/cpuinfo into a buffer; if that buffer is too small, then no information will be returned at all (updateCpuInfo() will return -1, so initCpuInfo() will return early); the consequence is that ksysguardd/ksysguard/plasmaengineexplorer won't know about "system/cores" or "cpu/system/AverageClock"; that in turn will make the "System Load Viewer" plasmoid completely dysfunctional (it won't show *any* data) because it relies on "system/cores" being available.

The buffer is currently 32KiB large, which is not enough on modern hardware (e.g. 12-core/24-thread systems exceed it). This patch lowers the initial size to 8KiB to be as memory-efficient as possible on low-end systems (dual/quadcore systems shouldn't ever need to grow it), and grows the buffer size as needed to accommodate arbitrarily many CPUs (tested on a 72-thread system, where the buffer grows four times, reaching 128KiB final size).

When the buffer needs to grow, its size is doubled, so the overall cost of the growth scales linearly with the file size (on average, each byte is copied to a larger buffer at most once). Also, the buffer size is remembered as long as the process runs, so the cost of the growth is only incurred once on startup (or if additional CPUs come online, which is probably rare).

I've verified locally that this patch fixes the issue: the System Log Viewer plasmoid shows data as expected afterwards.

BUG: 384515

Diff Detail

Repository
R106 KSysguard
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jakobkummerow created this revision.Feb 13 2020, 11:22 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 13 2020, 11:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jakobkummerow requested review of this revision.Feb 13 2020, 11:22 AM
cfeck added a subscriber: cfeck.Feb 13 2020, 11:42 AM

From that bug report:

Newer x86_64 kernels, however, allow up to 4096 CPUs, so either the buffer needs to be much bigger, or we should do interleaved /proc/cpuinfo reading and parsing.

cfeck: Sure, an even better long-term fix would be to stop depending on a hard-coded buffer size. Interleaved reading and parsing would be one way to achieve that; another way would be a dynamic buffer size: start small, and if it's too small then allocate a new buffer that's twice as large and try again.

That said, I think the wish for a more scalable solution shouldn't block a (trivially simple) short-term fix that restores functionality for users that are affected today.

ahiemstra requested changes to this revision.Feb 13 2020, 1:12 PM
ahiemstra added a subscriber: ahiemstra.

I agree with what @cfeck says. I can also already think of at least one case where even a 256KiB buffer would be too small, which is a dual-cpu Epyc system, where you can potentially have 128 cores / 256 threads.

That said, I think a short-term buffer size bump is fine, I'd just increase it a bit more than 256, maybe make it 1024.

Additionally, please add a comment regarding the limitations of the buffer size above the define, so that we can find some trace of this discussion in the future.

This revision now requires changes to proceed.Feb 13 2020, 1:12 PM
jakobkummerow retitled this revision from Linux/cpuinfo.c: bump CPUINFOBUFSIZE for 12+ core CPUs to Linux/cpuinfo.c: grow buffer size as needed for 12+ core CPUs.
jakobkummerow edited the summary of this revision. (Show Details)

OK, here's a version that scales the buffer size as needed.

Minor update: size_t instead of int.

ahiemstra accepted this revision.Feb 13 2020, 3:41 PM

That's even better, thanks. Do you have commit access?

This revision is now accepted and ready to land.Feb 13 2020, 3:41 PM
cfeck added a comment.Feb 13 2020, 3:44 PM

That looks clever. Since CpuInfoBufSize is a static variable, it will remember the last used size and not reallocate again (unless you plug in new CPUs).

Is realloc() an option? It should help the case where the library can actually grow the buffer without an alloc/copy/free cycle.

ahiemstra: Thanks! I don't have commit access, so I'd appreciate it if you could land this for me.

cfeck: Yes, realloc might be a nice improvement (if you think it even matters for this case). Wanna submit a followup PR?

ngraham edited the summary of this revision. (Show Details)Feb 13 2020, 10:05 PM
This revision was automatically updated to reflect the committed changes.

@jakobkummerow Ugh, do you have an email address and full name for me? I thought they were in the commit but apparently not (or arc ate them). Then I can fix the authoring information of that commit.

ahiemstra reopened this revision.Feb 17 2020, 10:32 AM
This revision is now accepted and ready to land.Feb 17 2020, 10:32 AM

Thanks for landing this! My name is Jakob Kummerow, email: jakob.kummerow@gmail.com

I created the diff via Phabricator's web interface (uploading a .diff file), would have expected it to take the author information from my account...

One would expect that, yes. But one would be expecting too much of Phabricator. :)

This revision was automatically updated to reflect the committed changes.

Now pushed with correct author information. :) Thanks!