Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

virtcontainers: fix invalid CPU topology#1606

Merged
chavafg merged 1 commit intokata-containers:masterfrom
devimc:topic/virtcontainers/fixCpuTopology
May 7, 2019
Merged

virtcontainers: fix invalid CPU topology#1606
chavafg merged 1 commit intokata-containers:masterfrom
devimc:topic/virtcontainers/fixCpuTopology

Conversation

@devimc
Copy link

@devimc devimc commented Apr 29, 2019

sockets * cores * threads should be equal to maxcpus otherwise a
warning is thrown: 'warning: Invalid CPU topology deprecated:
sockets * cores * threads != maxcpus'

This warning in the future will be an error and won't be possible to run
kata containers.

fixes #1605

Signed-off-by: Julio Montes julio.montes@intel.com

@devimc
Copy link
Author

devimc commented Apr 29, 2019

/test

sockets * cores * threads should be equal to maxcpus otherwise a
warning is thrown: 'warning: Invalid CPU topology deprecated:
    sockets * cores * threads != maxcpus'

This warning in the future will be an error and won't be possible to run
kata containers.

fixes kata-containers#1605

Signed-off-by: Julio Montes <julio.montes@intel.com>
@devimc devimc force-pushed the topic/virtcontainers/fixCpuTopology branch from c06fabd to fa5de87 Compare April 29, 2019 20:14
@devimc
Copy link
Author

devimc commented Apr 29, 2019

/test

smp := govmmQemu.SMP{
CPUs: vcpus,
Sockets: vcpus,
Sockets: maxvcpus,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(thought I'd left this question already - maybe it was on a parallel PR or Issue???)
@devimc - does this have any performance impacts? iirc, way back when we first played with topologies there was discussion that different topologies could affect performance. Did you measure with the metrics report by chance?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grahamwhaley metrics-CI is happy, do you want me to run a specific test?

@grahamwhaley
Copy link
Contributor

@devimc - nope, thats fine. Fair point. Maybe it made more of a difference when we used to set the default ncpu to >=4 etc. Now we set to 1 and use hotplug, maybe we won't notice (at least not in the default container case). Hmm, maybe we need some metrics using ncpu > 1 :-)

Just for ref, metrics looks good with:

21:34:47 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+
21:34:47 | P/F |         NAME         |  FLR  |  MEAN  |  CEIL  |  GAP  |  MIN   |  MAX   |  RNG  | COV  | ITS |
21:34:47 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+
21:34:47 | P   | boot-times           | 95.0% | 102.8% | 105.0% | 10.0% | 93.4%  | 122.1% | 30.7% | 6.0% |  20 |
21:34:47 | P   | memory-footprint     | 95.0% | 100.3% | 105.0% | 10.0% | 100.3% | 100.3% | 0.0%  | 0.0% |   1 |
21:34:47 | P   | memory-footprint-ksm | 95.0% | 101.8% | 105.0% | 10.0% | 101.8% | 101.8% | 0.0%  | 0.0% |   1 |
21:34:47 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@grahamwhaley
Copy link
Contributor

Nudged fedora CI rebuild - looked like a net timeout.
Of course, when this lands, we'll also need to tweak the metrics CI bounds checks. Now, it would be great if somebody other than me did this - so we can spread the love - and not have me as the bottleneck...
In my head I'm even thinking 'he who broke it, fixes it'... heh.

@chavafg chavafg merged commit 4c5527f into kata-containers:master May 7, 2019
@devimc devimc deleted the topic/virtcontainers/fixCpuTopology branch October 9, 2019 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

qemu: warning: Invalid CPU topology deprecated

5 participants