-
Notifications
You must be signed in to change notification settings - Fork 292
CP-54473 Configure max-cstate by XAPI #6681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/config-ntp-timezone-maxcstate
Are you sure you want to change the base?
CP-54473 Configure max-cstate by XAPI #6681
Conversation
Test:
host.set_max_cstate
dbsync
error message:
python script example:
|
ocaml/xapi/xapi_host.ml
Outdated
Xapi_host_max_cstate.xenpm_set value 0L ; | ||
Xapi_host_max_cstate.xen_cmdline_set value 0L ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that currently the only value allowed for the substate is 0 - so the C1E enhanced halt state will not be legal (given that max cstate is currently 1). is this going to change in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The requirement limits the value to C0, C1 or default. And C1E itself should not be configured by the integer substate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to make the API itself more generic and allow configuring the sub-states too with the same API call, i.e. make it take 2 parameters (it is fine to make it stricter in the UI).
The requirement are for the high-level user experience (maybe the person who wrote it thought that keeping the list smaller is easier to implement, but that is usually not the case), and shouldn't be taken too literally.
The requirements aren't for the low-level/internal API details (where we shouldn't block things unless we have a good reason to), although of course the API needs to allow at least what the requirements asked for (it is OK to allow more).
One of the documented design principles of XAPI is:
The API is for developers and power users. If you ask it to blow your foot off, it should blow your foot off, not ask if you're sure. The GUI and CLI are where you query whether your user is sane.
If you want to keep the GUI and CLI simple by exposing just C0, C1 and unlimited that is probably OK (although in this case restricting the CLI would be more work for no good reason, it might be easier to just make it allow whatever the API allows)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add the support to set max-sub-cstate now. See the updated test result in the first comment. The field now is string, just the same rule as https://xenbits.xen.org/docs/unstable/misc/xen-command-line.html#max_cstate-x86
I'm also not completely sure I understand how this fits into the "config-ntp-timezone-maxcstate" feature, could the bigger picture be clarified? |
@last-genius As it shows, in this branch, some apis will be added to configure ntp, timezone, max_cstate things. Currently, users have to ssh to dom0 to configure them. Now they can use xenapi to replace the ssh based workflows. Especially we provided the feature that can disable ssh on dom0 recently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally solid. Some minor comments.
ocaml/xapi/xapi_host.ml
Outdated
in | ||
raise | ||
Api_errors.( | ||
Server_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the list should not be used for arbitrary strings. Our API errors are not ergonomic :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. the message is like
You attempted to set a value that is not supported by this implementation. The fully-qualified field name and the value that you tried to set are returned. Also returned is a developer-only diagnostic reason.
field: max_cstate
value: 3
reason: Only C0, C1 and unlimited are supported for max-cstate
The values in list are filled to field, value, reason orderly.
Printf.sprintf "Failed to update max_cstate: %s" (Printexc.to_string e) | ||
in | ||
error "%s" err_msg ; | ||
Helpers.internal_error "%s" err_msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal_error
accepts a printf-style format. Why not use it directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the internal error has a parameter log_err and default set tp false. I want to add the log. Then I didn't want to write the format string two times.
ocaml/xapi/xapi_host_max_cstate.ml
Outdated
| v, sub_v when v >= 0L && sub_v >= 0L -> | ||
["--set-xen"; Printf.sprintf "max_cstate=%Ld,%Ld" v sub_v] | ||
| v, -1L when v >= 0L -> | ||
["--set-xen"; Printf.sprintf "max_cstate=%Ld" v] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have something like this already for setting sched-gran.
It'd be good to refactor that API call (in a separate commit prior to this one), and introduce some set-xen and delete-xen helpers, that both of these API calls could reuse. That'll also help if in the future we want to add more tweakable parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactored functions should probably take a list of key-value pairs, where the value is optional, so that we only need to declare the key once, and we wouldn't risk typoing delete-xen vs set-xen.
(the value needs to be optional to allow for arguments that are not key-value pairs, like some kernel cmdline options are)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick try, the functions are like Xen_cmdline_helper.set_xen kvs
, Xen_cmdline_helper.delete_xen keys
. I find it's every tiny different to add one more abstract layer for the xen_cmdline script. And I am not very clear about the xen_cmdline_helper behavior in different cases (like set a list of key-values). So, I am not going to refactor the functions now.
Done. The test result is updated in the first comment. |
Two commands are used to set max_cstate: xenpm to set at runtime and xen-cmdline to set it in grub conf file to take effect after reboot. Signed-off-by: Changlei Li <changlei.li@cloud.com>
String is used to represent the max_cstate and max_sub_cstate. "" -> unlimited "N" -> max cstate CN "N,M" -> max cstate CN with max sub state M Just follow the xen-cmdline cstate, see https://xenbits.xen.org/docs/unstable/misc/xen-command-line.html#max_cstate-x86 Signed-off-by: Changlei Li <changlei.li@cloud.com>
Signed-off-by: Changlei Li <changlei.li@cloud.com>
74e5aa9
to
182a553
Compare
C-states are power management states for CPUs where higher numbered states represent deeper sleep modes with lower power consumption but higher wake-up latency. The max_cstate parameter controls the deepest C-state that CPUs are allowed to enter.
Common C-state values:
To set max_cstate on dom0 host, two commands are used:
xenpm
to set at runtime andxen-cmdline
to set it in grub conf file to take effect after reboot.xenpm examples:
xen-command-line examples:
xen-command-line.max_cstate.
This PR adds a new field
host.max_cstate
to manage host's max_cstate.host.set_max_cstate
use the two commands mentioned above to configure. While dbsync on xapi start, the filed will be synced byxen-cmdline --get-xen max_cstate