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.