Page MenuHomeFreeBSD

Summary: RFC: Add support for Intel Speed Shift
Needs ReviewPublic

Authored by bwidawsk on Sun, Nov 18, 1:09 AM.

Details

Reviewers
kib
jhb
Summary

More work is needed to actually plug in cpufreq, and to disable acpi_perf, but I
wanted to know what folks thought before doing anything else on this.

Test Plan

some performance test?

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 20903
Build 20271: arc lint + arc unit

Event Timeline

bwidawsk created this revision.Sun, Nov 18, 1:09 AM
kib added a comment.Sun, Nov 18, 7:02 AM

You can commit the MSRs indexes and CPUID bits definition ahead of the other stuff.

sys/x86/cpufreq/hwpstate_intel.c
221

Can we get symbolic names for the bits ?

236

Don't you leak the binding after the end of the loop ?

sys/x86/include/specialreg.h
542 ↗(On Diff #50545)

Use tab after #define. Enable whitespace mode in emacs or list in vi, you will see surround lines' style.

kib added inline comments.Sun, Nov 18, 8:02 AM
sys/x86/cpufreq/hwpstate_intel.c
55

The header is already included at line 43 above. Is the second time needed ?

57

If you export the value with SYSCTL_UINT, then the type of the var better be uint. Or use SYSCTL_U32.

124

You can save a lot of vertical space by oring the conditions instead of using separate ifs.

142

Please add a symbol for the bit name.

163

Style: put all declarations at the start of the function. Sorry.

190

Too many blank lines.

244

You need a blank line after '{' if there is no locals.

bwidawsk marked 11 inline comments as done.Tue, Nov 20, 12:45 AM

Some of this has been moved into D18050

sys/x86/cpufreq/hwpstate_intel.c
124

I personally like to have things broken out individually so it makes things very obvious. However, I'm fine to change it if generally, people condense things. Can you please advise?

bwidawsk updated this revision to Diff 50618.Tue, Nov 20, 12:47 AM

Moved bit definitions to another differential. Took all of kib@ feedback. Will continue to add to this.

bwidawsk updated this revision to Diff 50619.Tue, Nov 20, 12:47 AM

Forgot to add the amd part in the last update

kib added inline comments.Tue, Nov 20, 10:00 AM
sys/x86/cpufreq/hwpstate_intel.c
124

Style(9) only uses vertical space before multi line comment and after the block which was started by that comment.

236

It is enough to do it only once, after the loop.

In fact, very careful code might save cpu binding, if any, before doing this, and restore it after the loop. See e.g. sys/dev/cpuctl for example. I do not think it is warranted there.