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