Re: [RFC 08/52] machine: Add helpers to get cpu topology info from MachineState.topo

[Zhao Liu]

On Fri, Feb 17, 2023 at 03:41:30PM +0800, wangyanan (Y) wrote:
> Date: Fri, 17 Feb 2023 15:41:30 +0800
> From: "wangyanan (Y)" <wangyanan55@huawei.com>
> Subject: Re: [RFC 08/52] machine: Add helpers to get cpu topology info from
>  MachineState.topo
> 
> 在 2023/2/17 11:07, Zhao Liu 写道:
> > On Thu, Feb 16, 2023 at 04:38:38PM +0800, wangyanan (Y) wrote:
> > > Date: Thu, 16 Feb 2023 16:38:38 +0800
> > > From: "wangyanan (Y)" <wangyanan55@huawei.com>
> > > Subject: Re: [RFC 08/52] machine: Add helpers to get cpu topology info 
> > > from
> > >   MachineState.topo
> > > 
> > > Hi Zhao,
> > > 
> > > 在 2023/2/13 17:49, Zhao Liu 写道:
> > > > From: Zhao Liu <zhao1.liu@intel.com>
> > > > 
> > > > When MachineState.topo is introduced, the topology related structures
> > > > become complicated. In the general case (hybrid or smp topology),
> > > > accessing the topology information needs to determine whether it is
> > > > currently smp or hybrid topology, and then access the corresponding
> > > > MachineState.topo.smp or MachineState.topo.hybrid.
> > > > 
> > > > The best way to do this is to wrap the access to the topology to
> > > > avoid having to check each time it is accessed.
> > > > 
> > > > The following helpers are provided here:
> > > > 
> > > > - General interfaces - no need to worry about whether the underlying
> > > >     topology is smp or hybrid:
> > > > 
> > > > * machine_topo_get_cpus()
> > > > * machine_topo_get_max_cpus()
> > > > * machine_topo_is_smp()
> > > > * machine_topo_get_sockets()
> > > > * machine_topo_get_dies()
> > > > * machine_topo_get_clusters()
> > > > * machine_topo_get_threads();
> > > > * machine_topo_get_cores();
> > > > * machine_topo_get_threads_by_idx()
> > > > * machine_topo_get_cores_by_idx()
> > > > * machine_topo_get_cores_per_socket()
> > > > * machine_topo_get_threads_per_socket()
> > > > 
> > > > - SMP-specific interfaces - provided for the cases that are clearly
> > > > known to be smp topology:
> > > > 
> > > > * machine_topo_get_smp_cores()
> > > > * machine_topo_get_smp_threads()
> > > > 
> > > > Since for hybrid topology, each core may has different threads, if
> > > > someone wants "cpus per core", the cpu_index is need to target a
> > > > specific core (machine_topo_get_threads_by_idx()). But for smp, there is
> > > > no need to be so troublesome, so for this case, we provide smp-specific
> > > > interfaces.
> > > > 
> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > ---
> > > >    hw/core/machine-topo.c | 142 
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >    include/hw/boards.h    |  35 ++++++++++
> > > >    2 files changed, 177 insertions(+)
> > > > 
> > > > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c
> > > > index 7223f73f99b0..b20160479629 100644
> > > > --- a/hw/core/machine-topo.c
> > > > +++ b/hw/core/machine-topo.c
> > > > @@ -21,6 +21,148 @@
> > > >    #include "hw/boards.h"
> > > >    #include "qapi/error.h"
> > > > +unsigned int machine_topo_get_sockets(const MachineState *ms)
> > > > +{
> > > > +    return machine_topo_is_smp(ms) ? ms->topo.smp.sockets :
> > > > +                                     ms->topo.hybrid.sockets;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_dies(const MachineState *ms)
> > > > +{
> > > > +    return machine_topo_is_smp(ms) ? ms->topo.smp.dies :
> > > > +                                     ms->topo.hybrid.dies;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_clusters(const MachineState *ms)
> > > > +{
> > > > +    return machine_topo_is_smp(ms) ? ms->topo.smp.clusters :
> > > > +                                     ms->topo.hybrid.clusters;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_smp_cores(const MachineState *ms)
> > > > +{
> > > > +    g_assert(machine_topo_is_smp(ms));
> > > > +    return ms->topo.smp.cores;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_smp_threads(const MachineState *ms)
> > > > +{
> > > > +    g_assert(machine_topo_is_smp(ms));
> > > > +    return ms->topo.smp.threads;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_threads(const MachineState *ms,
> > > > +                                      unsigned int cluster_id,
> > > > +                                      unsigned int core_id)
> > > > +{
> > > > +    if (machine_topo_is_smp(ms)) {
> > > > +        return ms->topo.smp.threads;
> > > > +    } else {
> > > > +        return ms->topo.hybrid.cluster_list[cluster_id]
> > > > +                   .core_list[core_id].threads;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_cores(const MachineState *ms,
> > > > +                                    unsigned int cluster_id)
> > > > +{
> > > > +    if (machine_topo_is_smp(ms)) {
> > > > +        return ms->topo.smp.cores;
> > > > +    } else {
> > > > +        return ms->topo.hybrid.cluster_list[cluster_id].cores;
> > > > +    }
> > > > +}
> > > Is it possible to use variadic function so that those two smp specific
> > > helpers can be avoided? It's a bit wired that we have the generic
> > > machine_topo_get_threads but also need machine_topo_get_smp_threads
> > > at the same time.
> > I am not sure about this, because variadic functions unify function
> > naming, but eliminate the "smp-specific" information from the name.
> > 
> > Trying to get the cres/threads without considering the cpu index can
> > only be used in smp scenarios, and I think the caller needs to
> > understand that he knows it's smp.
> Ok, I get the point.
> When it comes to the naming, would it be more concise to remove the
> *_get_* in the fun name, such as machine_topo_get_cpus to
> machine_topo_cpus, machine_topo_get_clusters to machine_topo_clusters.

Good, thanks!

> 
> And maybe rename machine_topo_get_cores(int cluster_id, int core_id) to
> machine_topo_cores_by_ids?
> 
> Or machine_topo_get_cores() to machine_topo_cores_by_topo_ids()
> and machine_topo_get_cores_by_idx to machine_topo_cores_by_cpu_idx()

I like the latter, nice name.

> > > > +
> > > > +unsigned int machine_topo_get_threads_by_idx(const MachineState *ms,
> > > > +                                             unsigned int cpu_index)
> > > > +{
> > > > +    unsigned cpus_per_die;
> > > > +    unsigned tmp_idx;
> > > > +    HybridCluster *cluster;
> > > > +    HybridCore *core;
> > > > +
> > > > +    if (machine_topo_is_smp(ms)) {
> > > > +        return ms->topo.smp.threads;
> > > > +    }
> > > > +
> > > > +    cpus_per_die = ms->topo.max_cpus / (ms->topo.hybrid.sockets *
> > > > +                            ms->topo.hybrid.dies);
> > > > +    tmp_idx = cpu_index % cpus_per_die;
> > > > +
> > > > +    for (int i = 0; i < ms->topo.hybrid.clusters; i++) {
> > > > +        cluster = &ms->topo.hybrid.cluster_list[i];
> > > > +
> > > > +        for (int j = 0; j < cluster->cores; j++) {
> > > > +            core = &cluster->core_list[j];
> > > > +
> > > > +            if (tmp_idx < core->threads) {
> > > > +                return core->threads;
> > > > +            } else {
> > > > +                tmp_idx -= core->threads;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_cores_by_idx(const MachineState *ms,
> > > > +                                           unsigned int cpu_index)
> > > > +{
> > > > +    unsigned cpus_per_die;
> > > > +    unsigned tmp_idx;
> > > > +    HybridCluster *cluster;
> > > > +    HybridCore *core;
> > > > +
> > > > +    if (machine_topo_is_smp(ms)) {
> > > > +        return ms->topo.smp.cores;
> > > > +    }
> > > > +
> > > > +    cpus_per_die = ms->topo.max_cpus / (ms->topo.hybrid.sockets *
> > > > +                            ms->topo.hybrid.dies);
> > > > +    tmp_idx = cpu_index % cpus_per_die;
> > > > +
> > > > +    for (int i = 0; i < ms->topo.hybrid.clusters; i++) {
> > > > +        cluster = &ms->topo.hybrid.cluster_list[i];
> > > > +
> > > > +        for (int j = 0; j < cluster->cores; j++) {
> > > > +            core = &cluster->core_list[j];
> > > > +
> > > > +            if (tmp_idx < core->threads) {
> > > > +                return cluster->cores;
> > > > +            } else {
> > > > +                tmp_idx -= core->threads;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_cores_per_socket(const MachineState *ms)
> > > > +{
> > > > +    unsigned int cores_per_die = 0;
> > > > +
> > > > +    if (machine_topo_is_smp(ms)) {
> > > > +        return ms->topo.smp.cores * ms->topo.smp.clusters * 
> > > > ms->topo.smp.dies;
> > > > +    }
> > > > +
> > > > +    for (int i = 0; i < ms->topo.hybrid.clusters; i++) {
> > > > +        cores_per_die += ms->topo.hybrid.cluster_list[i].cores;
> > > > +    }
> > > > +
> > > > +    return cores_per_die * ms->topo.hybrid.dies;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_threads_per_socket(const MachineState 
> > > > *ms)
> > > > +{
> > > > +    unsigned int sockets = machine_topo_is_smp(ms) ? 
> > > > ms->topo.smp.sockets :
> > > > +                           ms->topo.hybrid.sockets;
> > > > +    return ms->topo.max_cpus / sockets;
> > > > +}
> > > > +
> > > >    /*
> > > >     * Report information of a machine's supported CPU topology 
> > > > hierarchy.
> > > >     * Topology members will be ordered from the largest to the smallest
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 0a61855499e3..34b64b012022 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -461,4 +461,39 @@ extern const size_t hw_compat_2_2_len;
> > > >    extern GlobalProperty hw_compat_2_1[];
> > > >    extern const size_t hw_compat_2_1_len;
> > > > +static inline
> > > > +unsigned int machine_topo_get_cpus(const MachineState *ms)
> > > > +{
> > > > +    return ms->topo.cpus;
> > > > +}
> > > > +
> > > > +static inline
> > > > +unsigned int machine_topo_get_max_cpus(const MachineState *ms)
> > > > +{
> > > > +    return ms->topo.max_cpus;
> > > > +}
> > > > +
> > > > +static inline
> > > > +bool machine_topo_is_smp(const MachineState *ms)
> > > > +{
> > > > +    return ms->topo.topo_type == CPU_TOPO_TYPE_SMP;
> > > > +}
> > > > +
> > > > +unsigned int machine_topo_get_sockets(const MachineState *ms);
> > > > +unsigned int machine_topo_get_dies(const MachineState *ms);
> > > > +unsigned int machine_topo_get_clusters(const MachineState *ms);
> > > > +unsigned int machine_topo_get_smp_cores(const MachineState *ms);
> > > > +unsigned int machine_topo_get_smp_threads(const MachineState *ms);
> > > > +unsigned int machine_topo_get_threads(const MachineState *ms,
> > > > +                                      unsigned int cluster_id,
> > > > +                                      unsigned int core_id);
> > > > +unsigned int machine_topo_get_cores(const MachineState *ms,
> > > > +                                    unsigned int cluster_id);
> > > > +unsigned int machine_topo_get_threads_by_idx(const MachineState *ms,
> > > > +                                             unsigned int cpu_index);
> > > > +unsigned int machine_topo_get_cores_by_idx(const MachineState *ms,
> > > > +                                           unsigned int cpu_index);
> > > > +unsigned int machine_topo_get_cores_per_socket(const MachineState *ms);
> > > > +unsigned int machine_topo_get_threads_per_socket(const MachineState 
> > > > *ms);
> > > > +
> > > >    #endif
> > > I think it's necessary to document the ablity for each helper.
> > > For example, at a flance, I cant figure out what
> > > machine_topo_get_threads_idx
> > > does. Add some something like:
> > > /*
> > >   * Get number of threads within the CPU core where a processor locates,
> > >   * according to the processor index.
> > >   *
> > >   * @param: ...
> > >   */
> > > will be friendly to future users.
> > Yeah, thanks! I will.
> > 
> > > Thanks,
> > > Yanan
>