RE: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property

[Salil Mehta]

Hi Gavin,
Many thanks for taking pains to review this patch-set.

> 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
> 
> Hi Salil,
> 
> 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?


> > 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.


> 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.


>      -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.


>      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.


> [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
}



> 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.



> > 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 ;)


Thanks
Salil.