John Ferlan wrote:
On 06/07/2018 09:45 AM, Roman Bogorodskiy wrote:
John Ferlan wrote:
On 05/29/2018 12:57 PM, Roman Bogorodskiy wrote:
Recently, bhyve started supporting specifying guest CPU topology. It looks this way:
bhyve -c cpus=C,sockets=S,cores=C,threads=T ...
The old behaviour with bhyve -c C, where C is a number of vCPUs, is still supported.
So if we have CPU topology in the domain XML, use the new syntax, otherwise keeps the old behaviour.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_capabilities.c | 7 +++-- src/bhyve/bhyve_capabilities.h | 1 + src/bhyve/bhyve_command.c | 26 ++++++++++++++++++- ...yvexml2argv-cputopology-nvcpu-mismatch.xml | 26 +++++++++++++++++++ .../bhyvexml2argv-cputopology.args | 9 +++++++ .../bhyvexml2argv-cputopology.ldargs | 3 +++ .../bhyvexml2argv-cputopology.xml | 26 +++++++++++++++++++ tests/bhyvexml2argvtest.c | 8 +++++- 8 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml
diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index e13085b1d5..a3229cea75 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -227,7 +227,7 @@ bhyveProbeCapsDeviceHelper(unsigned int *caps, }
static int -bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary) +bhyveProbeCapsFromHelp(unsigned int *caps, char *binary)
Could have made this a separate just a name change patch
{ char *help; virCommandPtr cmd = NULL; @@ -244,6 +244,9 @@ bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary) if (strstr(help, "-u:") != NULL) *caps |= BHYVE_CAP_RTC_UTC;
+ if (strstr(help, "sockets=n][,cores=n][,threads=n") != NULL) + *caps |= BHYVE_CAP_CPUTOPOLOGY; +
I assume no concerns w/ i18n? That is the help is always in English?
Yeah, there's no i18n for bhyve.
It's really a shame there's no other way to determine other than help scraping. Other options would be the lack of "-c vcpus" in the output or finding topology... Avoids using that long exact string.
What is the issue with the long exact string? I guess it's worse performance-wise, but probably not that critical compared to running the bhyve binary... OTOH, checking it this way seems to be more straightforward.
Fear of someone changing "something" in the string and that causing problems. No one ever changes those strings though, right? ;-) The -c: string text changed for that particular patch from " -c: # cpus (default 1)\n" to "-c: number of cpus and/or topology specification", so relying on the current string never to change is a risk. In the long run IDC and it's not the time for length of compare - just the risk of change. Your call.
Yes, I guess your suggestion to check absence "-c vcpus" is less error prone for small formatting changes / extensions of the '-c' switch. I've changed this check accordingly, and re-rolled commits to look like 1. function rename 2. topology support + driver page update 3. news update. I plan to push it today without sending out v3. Thanks
John
Things seem find otherwise,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
Roman Bogorodskiy
Roman Bogorodskiy