Conversation
Betaminos
commented
Mar 9, 2023
Betaminos
commented
- Add read-only mode (run without arguments) for reading from the various registers
- Add additional output in case the MMIO limit reg is not locked
- Minor rephrasing to emphasize state (ENabled and DISabled)
- Remove duplicate variable
Add read-only mode (run without arguments) for reading from the various registers Add additional output in case the MMIO limit reg is not locked Minor rephrasing to emphasize state (ENabled and DISabled) Remove duplicate variable
| INTEL_PL1_ENABLE_BITS_LOW=0x00008000 | ||
| INTEL_PL2_ENABLE_BITS_HIGH=0x00008000 | ||
| INTEL_PL1_PL2_ENABLE_BITS=$((INTEL_PL1_ENABLE_BITS_LOW | (INTEL_PL2_ENABLE_BITS_HIGH<<32))) | ||
| INTEL_PL_ENABLE_BITS=0x00008000 #identical for PL1_ENABLE_BITS_LOW and PL2_ENABLE_BITS_HIGH |
There was a problem hiding this comment.
The fact these two register halves incidental values for the desired field is incidental. They should still be kept separate for clarity and for future maintenance.
| if [ "$#" != "2" ]; then | ||
| if [ "$#" == "0" ]; then | ||
| echo "!! Running SetPL in read-only mode !!" | ||
| readOnly=$TRUE |
There was a problem hiding this comment.
Let's please refer to this as "showOnly" instead of "readOnly", which makes the intention clearer.
| # | ||
| if [ "$#" != "2" ]; then | ||
| if [ "$#" == "0" ]; then | ||
| echo "!! Running SetPL in read-only mode !!" |
There was a problem hiding this comment.
This echo can be removed - it'll be implied from the context of how the user invoked the utility.
| echo "!! Running SetPL in read-only mode !!" | ||
| readOnly=$TRUE | ||
|
|
||
| elif [ "$#" != "2" ]; then |
There was a problem hiding this comment.
Please add the new zero-arg invocation description in this help message.
| echo "**** Setting PL1=$PL1 and PL2=$PL2 in /sys/class/powercap/intel-rapl/intel-rapl:0/constraint_*_power_limit_uw" | ||
| echo "$PL1" > /sys/class/powercap/intel-rapl/intel-rapl:0/constraint_0_power_limit_uw | ||
| echo "$PL2" > /sys/class/powercap/intel-rapl/intel-rapl:0/constraint_1_power_limit_uw | ||
| if [ $readOnly -ne $TRUE ]; then |
There was a problem hiding this comment.
Rather than having the showOnly checks distributed throughout the logic, please add a block here to specifically handle the read-only case by printing the value of the MMIO (lock status + PL1/PL2 values), then exit. Everything beyond this point will then be only for the set case. You can place the logic in the function to make it reusable for both the showOnly and regular cases.
| msr=$((msr | INTEL_PL1_PL2_ENABLE_BITS)) | ||
| writeMsr $INTEL_MSR_PKG_POWER_LIMIT $msr | ||
| else | ||
| echo "**** PL1 and PL2 are DISabled in MSR_PKG_POWER_LIMIT" |
There was a problem hiding this comment.
turbostat's strange capitalization of "DISabled" and "ENabled" doesn't need to be matched in setPL.
- corrected typo - added reference to devmem2
2 participants
