| Message ID | 20221027032613.18377-2-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Only generate cluster node in PPTT when specified | expand |
Hi Yicong, On 2022/10/27 11:26, Yicong Yang wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > Currently we'll always generate a cluster node no matter user has > specified '-smp clusters=X' or not. Cluster is an optional level > and will participant the building of Linux scheduling domains and > only appears on a few platforms. It's unncessary to always build > it which cannot reflect the real topology on platforms have no > cluster and to avoid affecting the linux scheduling domains in the > VM. So only generate it when user specified explicitly. > > Tested qemu-system-aarch64 with `-smp 8` and linux 6.1-rc1, without > this patch: > estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_* > ff # cluster_cpus > 0-7 # cluster_cpus_list > 56 # cluster_id > > with this patch: > estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_* > ff # cluster_cpus > 0-7 # cluster_cpus_list > 36 # cluster_id, with no cluster node kernel will make it to > physical package id > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > hw/acpi/aml-build.c | 2 +- > hw/core/machine-smp.c | 3 +++ > include/hw/boards.h | 2 ++ > qemu-options.hx | 2 ++ > 4 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index e6bfac95c7..aab73af66d 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, > 0, socket_id, NULL, 0); > } > > - if (mc->smp_props.clusters_supported) { > + if (mc->smp_props.clusters_supported && ms->smp.build_cluster) { > if (cpus->cpus[n].props.cluster_id != cluster_id) { > assert(cpus->cpus[n].props.cluster_id > cluster_id); > cluster_id = cpus->cpus[n].props.cluster_id; > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c > index b39ed21e65..5d37e8d07a 100644 > --- a/hw/core/machine-smp.c > +++ b/hw/core/machine-smp.c > @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms, > ms->smp.threads = threads; > ms->smp.max_cpus = maxcpus; > > + if (config->has_clusters) > + ms->smp.build_cluster = true; > + > /* sanity-check of the computed topology */ > if (sockets * dies * clusters * cores * threads != maxcpus) { > g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 311ed17e18..c53f047b90 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState { > * @cores: the number of cores in one cluster > * @threads: the number of threads in one core > * @max_cpus: the maximum number of logical processors on the machine > + * @build_cluster: build cluster topology or not > */ > typedef struct CpuTopology { > unsigned int cpus; > @@ -314,6 +315,7 @@ typedef struct CpuTopology { > unsigned int cores; > unsigned int threads; > unsigned int max_cpus; > + bool build_cluster; build_cluster seems a variable defined specifically for ACPI PPTT generation. It may not be proper to place it in the generic struct CpuTopology which only holds topo members. What about a more generic variable in struct SMPCompatProps together with @clusters_supported. Something like below: diff --git a/include/hw/boards.h b/include/hw/boards.h index 1f57ee8ca2..8db0706d5d 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -130,11 +130,14 @@ typedef struct { * @prefer_sockets - whether sockets are preferred over cores in smp parsing * @dies_supported - whether dies are supported by the machine * @clusters_supported - whether clusters are supported by the machine + * @has_clusters - whether clusters is explicitly specified in the user + * provided SMP configuration. */ typedef struct { bool prefer_sockets; bool dies_supported; bool clusters_supported; + bool has_clusters; } SMPCompatProps; > } CpuTopology; > > /** > diff --git a/qemu-options.hx b/qemu-options.hx > index eb38e5dc40..0a710e7be3 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -342,6 +342,8 @@ SRST > were preferred over threads), however, this behaviour is considered > liable to change. Prior to 6.2 the preference was sockets over cores > over threads. Since 6.2 the preference is cores over sockets over threads. > + The cluster topology will only be generated if explicitly specified > + by the "-cluster" option. no "-cluster" option, only "-smp" :) > For example, the following option defines a machine board with 2 sockets > of 1 core before 6.2 and 1 socket of 2 cores after 6.2: Maybe better to add a note at *end* of the doc about -smp like: Note: The cluster topology will only be generated in ACPI and exposed to guest if it's explicitly specified in -smp. Thanks, Yanan
Hi Yanan, On 2022/10/31 14:56, wangyanan (Y) wrote: > Hi Yicong, > > On 2022/10/27 11:26, Yicong Yang wrote: >> From: Yicong Yang <yangyicong@hisilicon.com> >> >> Currently we'll always generate a cluster node no matter user has >> specified '-smp clusters=X' or not. Cluster is an optional level >> and will participant the building of Linux scheduling domains and >> only appears on a few platforms. It's unncessary to always build >> it which cannot reflect the real topology on platforms have no >> cluster and to avoid affecting the linux scheduling domains in the >> VM. So only generate it when user specified explicitly. >> >> Tested qemu-system-aarch64 with `-smp 8` and linux 6.1-rc1, without >> this patch: >> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_* >> ff # cluster_cpus >> 0-7 # cluster_cpus_list >> 56 # cluster_id >> >> with this patch: >> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_* >> ff # cluster_cpus >> 0-7 # cluster_cpus_list >> 36 # cluster_id, with no cluster node kernel will make it to >> physical package id >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> hw/acpi/aml-build.c | 2 +- >> hw/core/machine-smp.c | 3 +++ >> include/hw/boards.h | 2 ++ >> qemu-options.hx | 2 ++ >> 4 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index e6bfac95c7..aab73af66d 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, >> 0, socket_id, NULL, 0); >> } >> - if (mc->smp_props.clusters_supported) { >> + if (mc->smp_props.clusters_supported && ms->smp.build_cluster) { >> if (cpus->cpus[n].props.cluster_id != cluster_id) { >> assert(cpus->cpus[n].props.cluster_id > cluster_id); >> cluster_id = cpus->cpus[n].props.cluster_id; >> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c >> index b39ed21e65..5d37e8d07a 100644 >> --- a/hw/core/machine-smp.c >> +++ b/hw/core/machine-smp.c >> @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms, >> ms->smp.threads = threads; >> ms->smp.max_cpus = maxcpus; >> + if (config->has_clusters) >> + ms->smp.build_cluster = true; >> + >> /* sanity-check of the computed topology */ >> if (sockets * dies * clusters * cores * threads != maxcpus) { >> g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 311ed17e18..c53f047b90 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState { >> * @cores: the number of cores in one cluster >> * @threads: the number of threads in one core >> * @max_cpus: the maximum number of logical processors on the machine >> + * @build_cluster: build cluster topology or not >> */ >> typedef struct CpuTopology { >> unsigned int cpus; >> @@ -314,6 +315,7 @@ typedef struct CpuTopology { >> unsigned int cores; >> unsigned int threads; >> unsigned int max_cpus; >> + bool build_cluster; > build_cluster seems a variable defined specifically for ACPI PPTT > generation. It may not be proper to place it in the generic struct > CpuTopology which only holds topo members. > > What about a more generic variable in struct SMPCompatProps > together with @clusters_supported. Something like below: > ok. will follow the suggestion. Thanks for the snippet :) > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 1f57ee8ca2..8db0706d5d 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -130,11 +130,14 @@ typedef struct { > * @prefer_sockets - whether sockets are preferred over cores in smp parsing > * @dies_supported - whether dies are supported by the machine > * @clusters_supported - whether clusters are supported by the machine > + * @has_clusters - whether clusters is explicitly specified in the user > + * provided SMP configuration. > */ > typedef struct { > bool prefer_sockets; > bool dies_supported; > bool clusters_supported; > + bool has_clusters; > } SMPCompatProps; > >> } CpuTopology; >> /** >> diff --git a/qemu-options.hx b/qemu-options.hx >> index eb38e5dc40..0a710e7be3 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -342,6 +342,8 @@ SRST >> were preferred over threads), however, this behaviour is considered >> liable to change. Prior to 6.2 the preference was sockets over cores >> over threads. Since 6.2 the preference is cores over sockets over threads. >> + The cluster topology will only be generated if explicitly specified >> + by the "-cluster" option. > no "-cluster" option, only "-smp" :) ok. >> For example, the following option defines a machine board with 2 sockets >> of 1 core before 6.2 and 1 socket of 2 cores after 6.2: > Maybe better to add a note at *end* of the doc about -smp like: > > Note: The cluster topology will only be generated in ACPI and exposed > to guest if it's explicitly specified in -smp. > will do. Thanks.
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index e6bfac95c7..aab73af66d 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, 0, socket_id, NULL, 0); } - if (mc->smp_props.clusters_supported) { + if (mc->smp_props.clusters_supported && ms->smp.build_cluster) { if (cpus->cpus[n].props.cluster_id != cluster_id) { assert(cpus->cpus[n].props.cluster_id > cluster_id); cluster_id = cpus->cpus[n].props.cluster_id; diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index b39ed21e65..5d37e8d07a 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms, ms->smp.threads = threads; ms->smp.max_cpus = maxcpus; + if (config->has_clusters) + ms->smp.build_cluster = true; + /* sanity-check of the computed topology */ if (sockets * dies * clusters * cores * threads != maxcpus) { g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); diff --git a/include/hw/boards.h b/include/hw/boards.h index 311ed17e18..c53f047b90 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState { * @cores: the number of cores in one cluster * @threads: the number of threads in one core * @max_cpus: the maximum number of logical processors on the machine + * @build_cluster: build cluster topology or not */ typedef struct CpuTopology { unsigned int cpus; @@ -314,6 +315,7 @@ typedef struct CpuTopology { unsigned int cores; unsigned int threads; unsigned int max_cpus; + bool build_cluster; } CpuTopology; /** diff --git a/qemu-options.hx b/qemu-options.hx index eb38e5dc40..0a710e7be3 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -342,6 +342,8 @@ SRST were preferred over threads), however, this behaviour is considered liable to change. Prior to 6.2 the preference was sockets over cores over threads. Since 6.2 the preference is cores over sockets over threads. + The cluster topology will only be generated if explicitly specified + by the "-cluster" option. For example, the following option defines a machine board with 2 sockets of 1 core before 6.2 and 1 socket of 2 cores after 6.2: