Page MenuHomeFreeBSD

hwpstate_amd: Refactor by brancless version
ClosedPublic

Authored by aokblast on Sun, Mar 1, 2:28 PM.
Tags
None
Referenced Files
F149567608: D55604.id174098.diff
Wed, Mar 25, 7:35 AM
F149559251: D55604.id173437.diff
Wed, Mar 25, 6:25 AM
Unknown Object (File)
Tue, Mar 24, 11:49 AM
Unknown Object (File)
Tue, Mar 24, 2:20 AM
Unknown Object (File)
Sat, Mar 21, 3:17 AM
Unknown Object (File)
Thu, Mar 19, 6:08 AM
Unknown Object (File)
Wed, Mar 18, 7:10 AM
Unknown Object (File)
Wed, Mar 18, 6:48 AM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Idea is great. Currently, however, this revision depends on D55477 (which is rejected), so it needs to be re-based to become independent.

sys/x86/cpufreq/hwpstate_amd.c
646–649

I'd rather keep the existing style here.

aokblast added inline comments.
sys/x86/cpufreq/hwpstate_amd.c
646–649

It is removed so I mark it as done.

olce requested changes to this revision.Mon, Mar 9, 8:59 AM

Some small changes to do. Main point is to preserve sysctl_cppc_dump_handler()'s goal which is to always query the hardware (and thus never rely on cached values).

sys/x86/cpufreq/hwpstate_amd.c
179–180

CAPABILITY_1 is 64-bit (even if the upper 32-bits are reserved for the time being).

This change should be done in another revision/commit. What is your goal here? IMO, it would be useful to create sysctl knobs exporting the capability bounds; we'll have to apply the same kind of workaround for an uninitialized/improperly initialized register as we are currently applying for min/max performance value (in enable_cppc_cb()).

297–298

We really want sysctl_cppc_dump_handler() to always query CPUs and not use cache values (useful for debugging), which this change is in contradiction with, so please remove it, along with the noted related ones below.

316–319

Same as above.

358

Same as above.

877–878

As the addition of caps1 in the softc, belongs to a separate commit. See the corresponding inline comment for more details.

This revision now requires changes to proceed.Mon, Mar 9, 8:59 AM
aokblast marked an inline comment as done.

Minor fixes

This revision is now accepted and ready to land.Thu, Mar 19, 6:21 AM

The commit log message should be a bit more explicit. Describing the implementation as "branchless" makes it sound like you are micro-optimizing the code to avoid branches, but really this change is just factoring out the backend of the driver into separate functions. I presume this is to make the code easier to read and modify.

sys/x86/cpufreq/hwpstate_amd.c
171
1022–1061

These should be static and const. .get = ... should be on the next line.

This revision now requires review to proceed.Fri, Mar 20, 2:53 AM
This revision is now accepted and ready to land.Fri, Mar 20, 3:09 AM