Page MenuHomeFreeBSD

Summary: Add support for Intel Speed Shift
Needs ReviewPublic

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

Details

Reviewers
kib
jhb
manu
Summary

Intel Speed Shift is Intel's technology to control frequency in hardware, with hints from software.

Test Plan

some performance test?

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22452
Build 21607: arc lint + arc unit

Event Timeline

bwidawsk created this revision.Nov 18 2018, 1:09 AM
kib added a comment.Nov 18 2018, 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.Nov 18 2018, 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.Nov 20 2018, 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.Nov 20 2018, 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.Nov 20 2018, 12:47 AM

Forgot to add the amd part in the last update

kib added inline comments.Nov 20 2018, 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.

237

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.

lwhsu added a subscriber: lwhsu.Tue, Jan 22, 1:30 AM
bwidawsk updated this revision to Diff 53817.Tue, Feb 12, 12:26 AM
bwidawsk marked an inline comment as done.

Add support for Intel Speed Shift

This is supporting Intel's hardware P-State technology. A HW autonomous control
of frequency.

github repo:
https://github.com/bwidawsk/freebsd/tree/hwp

  • cpufreq: Rename hwpstate to hwpstate_amd.
  • cpufreq: Remove confusing indirection to dev
  • cpufreq: Extract cf_get functionality
  • cpufreq: Extract level information
  • cpufreq: Allow cpufreq to act over individual cpus
  • cpufreq: Create an uncached flag
  • cpufreq: Add support for Intel Speed Shift
  • cpufreq: Clean up multi-device usage
  • cpufreq: Add some debug messages and comments
bwidawsk added a subscriber: cem.
bwidawsk retitled this revision from Summary: RFC: Add support for Intel Speed Shift to Summary: Add support for Intel Speed Shift.Tue, Feb 12, 12:29 AM
bwidawsk edited the summary of this revision. (Show Details)
kib added inline comments.Fri, Feb 15, 1:28 PM
sys/dev/acpica/acpi_perf.c
53

Use bool perhaps ?

149

Comparing with true is too baroque. If you use bool for the type, then if (intel_speed_shift) is good enough.

sys/kern/kern_cpu.c
416

Please do not use implementation-defined namespace without a reason (I do not see one).

421

return (-1);

423

return (set.freq);

428

And there too.

441

Are error values from this function used for anything ? If not, why not return -1 ?

sys/x86/cpufreq/hwpstate_intel.c
139

I think this call can sleep and lock some VM locks as well. It is not good to lock while the thread is bound. How hard is to restructure the code to read all MSRs first, then unbind, then do printfs ?

bwidawsk added inline comments.Sat, Feb 16, 12:18 AM
sys/kern/kern_cpu.c
416

I don't mind changing it, but it's static, so I'm not sure what you mean about adding implementation-defined namespace.

441

Mostly it was for debug to be able to differentiate error cases. I suppose someone debugging can always change it though, and I'll do -1

cem added inline comments.Sat, Feb 16, 12:46 AM
sys/kern/kern_cpu.c
416

The implementation-defined part is the __ prefix in the function name.

cem added inline comments.Sat, Feb 16, 12:51 AM
sys/kern/kern_cpu.c
441

It's mostly uncommon in FreeBSD to return negative error numbers and positive success values — that's more of a Linux (and imported Linux code) thing. Typically we'd return 0 or EFOO and set an output parameter for the level index. (Or sometimes, return the level index, or -1 as an out-of-range sentinel value.)

(Also, the spelling would be return (-ENOENT); the idea being that return() could be defined as a macro.)