[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,clu
|
From: |
Gavin Shan |
|
Subject: |
Re: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property |
|
Date: |
Tue, 3 Oct 2023 15:05:39 +1000 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 |
Hi Salil,
On 10/2/23 19:53, Salil Mehta wrote:
Many thanks for taking pains to review this patch-set.
No worries.
From: Gavin Shan <gshan@redhat.com>
Sent: Wednesday, September 27, 2023 12:57 AM
To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
qemu-arm@nongnu.org
Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
<jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
peter.maydell@linaro.org; richard.henderson@linaro.org;
imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com;
philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org;
oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org;
linux@armlinux.org.uk; darren@os.amperecomputing.com;
ilkka@os.amperecomputing.com; vishnu@os.amperecomputing.com;
karl.heubaum@oracle.com; miguel.luis@oracle.com; salil.mehta@opnsrc.net;
zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng (C)
<wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>;
jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn
Subject: Re: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU
{socket,cluster,core,thread}-id property
On 9/26/23 20:04, Salil Mehta wrote:
This shall be used to store user specified topology{socket,cluster,core,thread}
and shall be converted to a unique 'vcpu-id' which is used as slot-index during
hot(un)plug of vCPU.
Note that we don't have 'vcpu-id' property. It's actually the index to the array
ms->possible_cpus->cpus[] and cpu->cpu_index. Please improve the commit log if
it makes sense.
I can change but was it mentioned anywhere in the log that vcpu-id is
a property?
I was thinking it's a property when vcpu-id is quoted with ''. Besides,
"vcpu-id" is usually understood as vCPU's ID instead of vCPU index. I ment
to avoid 'vcpu-id' in the commit log because we're talking about vCPU index
instead vCPU ID. Otherwise, readers or reviewers will be confused because
of it.
Co-developed-by: Salil Mehta <salil.mehta@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
hw/arm/virt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
target/arm/cpu.c | 4 +++
target/arm/cpu.h | 4 +++
3 files changed, 71 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7d9dbc2663..57fe97c242 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -221,6 +221,11 @@ static const char *valid_cpus[] = {
ARM_CPU_TYPE_NAME("max"),
};
+static int virt_get_socket_id(const MachineState *ms, int cpu_index);
+static int virt_get_cluster_id(const MachineState *ms, int cpu_index);
+static int virt_get_core_id(const MachineState *ms, int cpu_index);
+static int virt_get_thread_id(const MachineState *ms, int cpu_index);
+
static bool cpu_type_valid(const char *cpu)
{
int i;
@@ -2168,6 +2173,14 @@ static void machvirt_init(MachineState *machine)
&error_fatal);
aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
+ object_property_set_int(cpuobj, "socket-id",
+ virt_get_socket_id(machine, n), NULL);
+ object_property_set_int(cpuobj, "cluster-id",
+ virt_get_cluster_id(machine, n), NULL);
+ object_property_set_int(cpuobj, "core-id",
+ virt_get_core_id(machine, n), NULL);
+ object_property_set_int(cpuobj, "thread-id",
+ virt_get_thread_id(machine, n), NULL);
if (!vms->secure) {
object_property_set_bool(cpuobj, "has_el3", false, NULL);
@@ -2652,10 +2665,59 @@ static int64_t virt_get_default_cpu_node_id(const
MachineState *ms, int idx)
return socket_id % ms->numa_state->num_nodes;
}
It seems it's not unnecessary to keep virt_get_{socket, cluster, core,
thread}_id()
because they're called for once. I would suggest to figure out the socket,
cluster,
core and thread ID through @possible_cpus in machvirt_init(), like below.
It is always good to access properties through accessor functions. Beside the
main purpose here was to keep the code neat. So I would stick with these.
But because these are something which are not specific to VirtMachine I can
move them to some other place or a header file so that other architectures
can also use them.
No, these functions aren't property accessors at all. Actually, they're figuring
out the IDs, passed to object_property_set_int(). I don't see the benefits to
keep those functions who are called for once, and their logic is simple enough
to
be integrated to the callers.
Besides, we can't always expose property "cluster-id" since cluster in the CPU
topology isn't always supported, seeing MachineClass::smp_props. Some users may
want to hide cluster for unknown reasons. 'cluster-id' shouldn't be exposed in
this case. Otherwise, users may be confused by 'cluster-id' property while it
has been disabled. For example, a VM is started with the following command lines
and 'cluster-id' shouldn't be supported in vCPU hot-add.
True. All we are talking about is 4*integer space. This is to avoid complexity
of checks everywhere in the code by having these variables always exists and
with default values as 0. If the architecture does not defines property it will
not use these variable. It is a little tradeoff of memory with respect to
maintainability of code. I would prefer later.
We can definitely put some comments in the places of their declaration.
I'm not sure if a comment will resolve the potential issue. It sounds weird that
'cluster-id' property exists even the level of CPU topology isn't supported at
all.
-cpu host -smp=maxcpus=2,cpus=1,sockets=2,cores=1,threads=1
(qemu) device_add
host,id=cpu1,socket-id=1,cluster-id=0,core-id=0,thread-id=0
object_property_set_int(cpuobj, "socket-id",
possible_cpus->cpus[i].props.socket_id, NULL);
if (mc->smp_props.cluster_supported && mc->smp_props.has_clusters) {
object_property_set_int(cpuobj, "cluster-id",
possible_cpus->cpus[i].props.cluster_id,
NULL);
}
Exactly, these types of checks can be avoided. They make code unnecessarily look
complex and ugly.
As explained above, the property 'cluster-id' shouldn't be existing if cluster
isn't supported in CPU topology.
object_property_set_int(cpuobj, "core-id",
possible_cpus->cpus[i].props.core_id, NULL);
object_property_set_int(cpuobj, "thread-id",
possible_cpus->cpus[i].props.thread_id, NULL);
+static int virt_get_socket_id(const MachineState *ms, int cpu_index)
+{
+ assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
+
+ return ms->possible_cpus->cpus[cpu_index].props.socket_id;
+}
+
+static int virt_get_cluster_id(const MachineState *ms, int cpu_index)
+{
+ assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
+
+ return ms->possible_cpus->cpus[cpu_index].props.cluster_id;
+}
+
+static int virt_get_core_id(const MachineState *ms, int cpu_index)
+{
+ assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
+
+ return ms->possible_cpus->cpus[cpu_index].props.core_id;
+}
+
+static int virt_get_thread_id(const MachineState *ms, int cpu_index)
+{
+ assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len);
+
+ return ms->possible_cpus->cpus[cpu_index].props.thread_id;
+}
+
+static int
+virt_get_cpu_id_from_cpu_topo(const MachineState *ms, DeviceState *dev)
+{
+ int cpu_id, sock_vcpu_num, clus_vcpu_num, core_vcpu_num;
+ ARMCPU *cpu = ARM_CPU(dev);
+
+ /* calculate total logical cpus across socket/cluster/core */
+ sock_vcpu_num = cpu->socket_id * (ms->smp.threads * ms->smp.cores *
+ ms->smp.clusters);
+ clus_vcpu_num = cpu->cluster_id * (ms->smp.threads * ms->smp.cores);
+ core_vcpu_num = cpu->core_id * ms->smp.threads;
+
+ /* get vcpu-id(logical cpu index) for this vcpu from this topology
*/
+ cpu_id = (sock_vcpu_num + clus_vcpu_num + core_vcpu_num) + cpu->thread_id;
+
+ assert(cpu_id >= 0 && cpu_id < ms->possible_cpus->len);
+
+ return cpu_id;
+}
+
This function is called for once in PATCH[04/37]. I think it needs to be moved
around to PATCH[04/37].
Yes, we can do that.
Ok.
[PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time change common
to vCPU {cold|hot}-plug
The function name can be shortened because I don't see the suffix
"_from_cpu_topo"
is too much helpful. I think virt_get_cpu_index() would be good enough since
it's
called for once to return the index in array MachineState::possible_cpus::cpus[]
and the return value is stored to CPUState::cpu_index
This is not an accessor function. This function derives the unique vcpu-id
from topology. Hence, naming is correct. Though, I can shorten the name
to something like below if you wish,
virt_get_cpu_id_from_cpu_topo() -> virt_cpu_id_from_topology()
The name virt_get_cpu_index() suggests as if function is something like below
and which it is not:
virt_get_cpu_index()
{
return cs->cpu_index
}
Well, the point was to indicate 'CPU index' instead of 'CPU ID' returned
from this function, as clarified in the commit log. Please don't mix
'CPU index' and 'CPU ID' even they're same in some situations on aarch64.
So how about renaming it to virt_cpu_index_from_topology()?
static int virt_get_cpu_index(const MachineState *ms, ARMCPU *cpu)
{
int index, cpus_in_socket, cpus_in_cluster, cpus_in_core;
/*
* It's fine to take cluster into account even it's not supported. In this
* case, ms->smp.clusters is always one.
*/
}
static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState
*ms)
{
int n;
unsigned int max_cpus = ms->smp.max_cpus;
+ unsigned int smp_threads = ms->smp.threads;
VirtMachineState *vms = VIRT_MACHINE(ms);
MachineClass *mc = MACHINE_GET_CLASS(vms);
@@ -2669,6 +2731,7 @@ static const CPUArchIdList
*virt_possible_cpu_arch_ids(MachineState *ms)
ms->possible_cpus->len = max_cpus;
for (n = 0; n < ms->possible_cpus->len; n++) {
ms->possible_cpus->cpus[n].type = ms->cpu_type;
+ ms->possible_cpus->cpus[n].vcpus_count = smp_threads;
ms->possible_cpus->cpus[n].arch_id =
virt_cpu_mp_affinity(vms, n);
This initialization seems to accomodate HMP command "info hotpluggable-
cpus".
It would be nice if it can be mentioned in the commit log.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 93c28d50e5..1376350416 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2277,6 +2277,10 @@ static Property arm_cpu_properties[] = {
DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
mp_affinity, ARM64_AFFINITY_INVALID),
DEFINE_PROP_INT32("node-id", ARMCPU, node_id,
CPU_UNSET_NUMA_NODE_ID),
+ DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0),
+ DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0),
+ DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0),
+ DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0),
DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
DEFINE_PROP_END_OF_LIST()
};
All those 4 properties are used for vCPU hot-add, meaning they're not needed
when vCPU hotplug isn't supported on the specific board. Even for hw/virt board,
cluster isn't always supported and 'cluster-id' shouldn't always be exposed,
as explained above. How about to register the properties dynamically only when
they're needed by vCPU hotplug?
Yes, these are part of arch specific files so it is upto the arch whether to
define
them or not to define them at all?
Yes, and as mentioned earlier, there is extra bit of memory(4*integer) which is
being used. I would tradeoff this vis-à-vis maintainability.
Right, making sense to me.
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 88e5accda6..d51d39f621 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1094,6 +1094,10 @@ struct ArchCPU {
QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
int32_t node_id; /* NUMA node this CPU belongs to */
+ int32_t socket_id;
+ int32_t cluster_id;
+ int32_t core_id;
+ int32_t thread_id;
It would be fine to keep those fields even the corresponding properties are
dynamically registered, but a little bit memory overhead incurred :)
You are contradicting yourself here ;)
Correct. I was wandering if we need the properties, even vCPU hotplug
isn't supported. As you explained above, I agree with you that it's fine
to keep these properties even vCPU hotplug is unsupported.
Thanks,
Gavin
- RE: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property, Salil Mehta, 2023/10/02
- Re: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property,
Gavin Shan <=