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)" <wangyana...@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....@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....@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. > > + > > +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