| Message ID | 20220922131143.58003-2-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Only generate cluster node in PPTT when specified | expand |
On Thu, Sep 22, 2022 at 09:11:40PM +0800, 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 it's unncessary to build it if user don't need. So only generate > it when user specify explicitly. > > Also update the test ACPI tables. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> This is an example of a commit log repeating what the patch does. Which is ok but the important thing is to explain the motivation - why is it a bug to generate a cluster node without '-smp clusters'? > --- > hw/acpi/aml-build.c | 2 +- > hw/core/machine-smp.c | 3 +++ > include/hw/boards.h | 2 ++ > 3 files changed, 6 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 7b416c9787..24aafc213d 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; > > /** > -- > 2.24.0
Hi Yicong, On 2022/9/22 21:11, 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 it's unncessary to build it if user don't need. So only generate > it when user specify explicitly. > > Also update the test ACPI tables. It would be much more helpful to explain the problem you have met in practice without this patch. (maybe have some description or a link of the issue in the cover-letter if we need a v2). In qemu which behaves as like a firmware vendor for VM, the ACPI PPTT is built based on the topology info produced by machine_parse_smp_config(). And machine_parse_smp_config will always calculate a complete topology hierarchy using its algorithm, if the user gives an incomplete -smp CLI. I think there are two options for us to chose: 1) approach described in this patch 2) qemu will always generate a full topology hierarchy in PPTT with all the topo members it currently supports. While users need to consider the necessity to use an incomplete -smp or an complete one according to their specific scenario, and should be aware of the kernel behavior resulted from the config. There is some Doc for users to explain how qemu will parse user-specified -smp in [1]. [1] https://www.mankier.com/1/qemu#Options Thanks, Yanan > 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 ++ > 3 files changed, 6 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 7b416c9787..24aafc213d 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; > > /**
On 2022/10/9 14:46, wangyanan (Y) wrote: > Hi Yicong, > > On 2022/9/22 21:11, 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 it's unncessary to build it if user don't need. So only generate >> it when user specify explicitly. >> >> Also update the test ACPI tables. > It would be much more helpful to explain the problem you > have met in practice without this patch. (maybe have some > description or a link of the issue in the cover-letter if we > need a v2). > My problem is related to this but not fully caused by this. I found my schedule domains are not built as expected with command `-smp 8` and 4 NUMA nodes. The final schedule domains built look like below with no NUMA domains built. [ 2.141316] CPU0 attaching sched-domain(s): [ 2.142558] domain-0: span=0-7 level=MC [ 2.145364] groups: 0:{ span=0 cap=964 }, 1:{ span=1 cap=914 }, 2:{ span=2 cap=921 }, 3:{ span=3 cap=964 }, 4:{ span=4 cap=925 }, 5:{ span=5 cap=964 }, 6:{ span=6 cap=967 }, 7:{ span=7 cap=967 } [ 2.158357] CPU1 attaching sched-domain(s): [ 2.158964] domain-0: span=0-7 level=MC should be: [ 2.008885] CPU0 attaching sched-domain(s): [ 2.009764] domain-0: span=0-1 level=MC [ 2.012654] groups: 0:{ span=0 cap=962 }, 1:{ span=1 cap=925 } [ 2.016532] domain-1: span=0-3 level=NUMA [ 2.017444] groups: 0:{ span=0-1 cap=1887 }, 2:{ span=2-3 cap=1871 } [ 2.019354] domain-2: span=0-5 level=NUMA [ 2.019983] groups: 0:{ span=0-3 cap=3758 }, 4:{ span=4-5 cap=1935 } [ 2.021527] domain-3: span=0-7 level=NUMA [ 2.022516] groups: 0:{ span=0-5 mask=0-1 cap=5693 }, 6:{ span=4-7 mask=6-7 cap=3978 } [...] It's because the MC level span extends to Cluster level which spans all the cpus in the system, then the schedule domain building stops at MC level since it already includes all the cpus. It makes people confusing that cluster node is generated without asking for it. A discussion for the problem: https://lore.kernel.org/lkml/2c079860-ee82-7719-d3d2-756192f41704@huawei.com/ > In qemu which behaves as like a firmware vendor for VM, > the ACPI PPTT is built based on the topology info produced > by machine_parse_smp_config(). And machine_parse_smp_config > will always calculate a complete topology hierarchy using its > algorithm, if the user gives an incomplete -smp CLI. > Considering cluster is an optional level and most platforms don't have it, they may even don't realize this is built and a always build policy cannot emulate the topology on these platforms. Also it may influences the build of schedule domains uncousiously in some cases so... > I think there are two options for us to chose: > 1) approach described in this patch > 2) qemu will always generate a full topology hierarchy in PPTT > with all the topo members it currently supports. While users > need to consider the necessity to use an incomplete -smp or > an complete one according to their specific scenario, and > should be aware of the kernel behavior resulted from the > config. > ...I'd prefer 1) then users can generate this *only* when they explicitly know what they want and what they'll get. A full topology hierachy generation lacks flexibility. Any thought? > There is some Doc for users to explain how qemu will > parse user-specified -smp in [1]. > [1] https://www.mankier.com/1/qemu#Options > Thanks! Yicong > Thanks, > Yanan >> 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 ++ >> 3 files changed, 6 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 7b416c9787..24aafc213d 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; >> /** > > .
On 2022/10/7 21:48, Michael S. Tsirkin wrote: > On Thu, Sep 22, 2022 at 09:11:40PM +0800, 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 it's unncessary to build it if user don't need. So only generate >> it when user specify explicitly. >> >> Also update the test ACPI tables. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > This is an example of a commit log repeating what the patch does. > Which is ok but the important thing is to explain the motivation - > why is it a bug to generate a cluster node without '-smp clusters'? > It may not be a bug but may build the unneeded topology unconsciously and doesn't provide a way to inhibit this. So I thought the policy can be improved. Thanks. > >> --- >> hw/acpi/aml-build.c | 2 +- >> hw/core/machine-smp.c | 3 +++ >> include/hw/boards.h | 2 ++ >> 3 files changed, 6 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 7b416c9787..24aafc213d 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; >> >> /** >> -- >> 2.24.0 > > . >
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 7b416c9787..24aafc213d 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; /**