On 2021/5/19 16:15, Andrew Jones wrote:
On Wed, May 19, 2021 at 07:54:37AM +0000, Salil Mehta wrote:
From: wangyanan (Y) Sent: Wednesday, May 19, 2021 4:18 AM
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. Hi Yanan, Makes sense. I did see your other patch-set in which cluster support has been added. Are we deferring that too? The merge of that needs to be deferred, but for a different reason. It shouldn't impact hot plug, because if hot plug doesn't like clusters, then one could configure a topology which doesn't have clusters. But, it can't be merged to QEMU until the kernel has merged its support. Agreed!
Thanks, Yanan
Thanks, drew
.