On Tue, May 18, 2021 at 06:34:08PM +0000, Salil Mehta wrote:
Those benefits, when vcpu pinning is used, are the same benefits
as for the host, which already use PPTT tables to describe topology, even though hot plug isn't supported.
yes sure, you mean pinning vcpus according to the cpu topology for performance?
Yup
Now, if you're saying we should only generate tables for smp.cpus, not
Correct. This is what I thought we must be doing even now
smp.maxcpus, because hot plug isn't supported anyway, then I see your point. But, it'd be better to require smp.cpus == smp.maxcpus in our smp_parse function to do that, which we've never done before, so we may have trouble supporting existing command lines.
I am trying to recall, if the vcpu Hotplug is not supported then can they ever be different?
cpus = (threads * cores * sockets)
static void smp_parse(MachineState *ms, QemuOpts *opts) { [...]
if (sockets * cores * threads != ms->smp.max_cpus) { warn_report("Invalid CPU topology deprecated: " "sockets (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, cores, threads, ms->smp.max_cpus); } [...] }
Although, above check does not exit(1) and just warns on detecting invalid CPU topology. Not sure why?
Hmm, not sure what code you have there. I see this in hw/core/machine.c:smp_parse if (ms->smp.max_cpus < cpus) { error_report("maxcpus must be equal to or greater than smp"); exit(1); } if (sockets * cores * threads != ms->smp.max_cpus) { error_report("Invalid CPU topology: " "sockets (%u) * cores (%u) * threads (%u) " "!= maxcpus (%u)", sockets, cores, threads, ms->smp.max_cpus); exit(1); }
Well if you think there are subtleties to support above implementation and we cannot do it now then sure it is your call. :)
The problem is that -smp 4,maxcpus=8 doesn't error out today, even though it doesn't do anything. OTOH, -smp 4,cores=2 doesn't error out either, but we're proposing that it should. Maybe we can start erroring out when cpus != maxcpus until hot plug is supported? Thanks, drew