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

[Zhao Liu]

On Tue, Feb 14, 2023 at 09:12:11AM +0800, Mi, Dapeng1 wrote:
> Date: Tue, 14 Feb 2023 09:12:11 +0800
> From: "Mi, Dapeng1" <dapeng1.mi@intel.com>
> Subject: RE: [RFC 08/52] machine: Add helpers to get cpu topology info from
>  MachineState.topo
> 
> > From: Zhao Liu <zhao1.liu@linux.intel.com>
> > Sent: Monday, February 13, 2023 5:50 PM
> > To: Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum
> > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daud? <philmd@linaro.org>;
> > Yanan Wang <wangyanan55@huawei.com>; Michael S . Tsirkin
> > <mst@redhat.com>; Richard Henderson <richard.henderson@linaro.org>; Paolo
> > Bonzini <pbonzini@redhat.com>; Eric Blake <eblake@redhat.com>; Markus
> > Armbruster <armbru@redhat.com>
> > Cc: qemu-devel@nongnu.org; Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; Mi,
> > Dapeng1 <dapeng1.mi@intel.com>; Ding, Zhuocheng
> > <zhuocheng.ding@intel.com>; Robert Hoo <robert.hu@linux.intel.com>;
> > Christopherson,, Sean <seanjc@google.com>; Like Xu
> > <like.xu.linux@gmail.com>; Liu, Zhao1 <zhao1.liu@intel.com>
> > Subject: [RFC 08/52] machine: Add helpers to get cpu topology info from
> > MachineState.topo
> > 
> > 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;
> > +    }
> > +}
> > +
> > +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;
> > +}
> > +
> 
> It looks most code of this function is same with previous function. Could we 
> extract
> the same part code and define a low level function?

Yes, I can try it.

Thanks,
Zhao

> 
> > +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
> > --
> > 2.34.1
>