On 2021/5/19 3:22, Salil Mehta wrote:
From: Andrew Jones [mailto:drjones@redhat.com] Sent: Tuesday, May 18, 2021 8:06 PM To: Salil Mehta <salil.mehta@huawei.com> Cc: wangyanan (Y) <wangyanan55@huawei.com>; Peter Maydell <peter.maydell@linaro.org>; Michael S . Tsirkin <mst@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; qemu-devel@nongnu.org; Shannon Zhao <shannon.zhaosl@gmail.com>; qemu-arm@nongnu.org; Alistair Francis <alistair.francis@wdc.com>; Zengtao (B) <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>; yuzenghui <yuzenghui@huawei.com>; Igor Mammedov <imammedo@redhat.com>; zhukeqian <zhukeqian1@huawei.com>; lijiajie (H) <lijiajie11@huawei.com>; David Gibson <david@gibson.dropbear.id.au>; Linuxarm <linuxarm@huawei.com>; linuxarm@openeuler.org Subject: Re: [RFC PATCH v2 5/6] hw/arm/virt-acpi-build: Add PPTT table
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 Already Agreed :)
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. :) Hi Salil, Drew, 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? Agreed, both don't make any sense if hotplug is not supported and ideally should fail with error. We should block any such topology configuration. In the ARM-specific function virt_smp_parse() (patch 9), there already have been some restrictions for the given -smp configuration. We now only allow: -smp N -smp maxcpus=M -smp N, maxcpus=M
-smp N, sockets=X, cores=Y -smp N, sockets=X, cores=Y, threads=Z -smp maxcpus=M, sockets=X, cores=Y -smp maxcpus=M, sockets=X, cores=Y, threads=Z -smp N, maxcpus=M, sockets=X, cores=Y -smp N, maxcpus=M, sockets=X, cores=Y, threads=Z and disallow the other strange and rare formats that shouldn't be provided. It's reasonable to block the topology configuration which is not useful currently. I will add the requirement for "cpus==maxcpus" in this fuction if the possible conflict with existing command lines is not a big problem. Thanks, Yanan
Thanks Salil .