| Message ID | 20190801034634.26913-2-jeremy.linton@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64/PPTT ACPI 6.3 thread flag support | expand |
Hi Jeremy, On Wed, Jul 31, 2019 at 10:46:33PM -0500, Jeremy Linton wrote: > ACPI 6.3 adds a flag to the CPU node to indicate whether > the given PE is a thread. Add a function to return that > information for a given linux logical CPU. > Apart from few minor nits, Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/acpi/pptt.c | 54 +++++++++++++++++++++++++++++++++++++++++++- > include/linux/acpi.h | 5 ++++ > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index 1e7ac0bd0d3a..84718f6cb741 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -540,6 +540,44 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag) > return retval; > } > > +/** > + * check_acpi_cpu_flag() - Determine if CPU node has a flag set > + * @cpu: Kernel logical CPU number > + * @rev: The PPTT revision defining the flag [nit] I would rather put it as minimum PPTT revision that supports the flag. It aligns with the code too as we are not looking for exact match. > + * @flag: The flag itself > + * > + * Check the node representing a CPU for a given flag. > + * > + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or > + * the table revision isn't new enough. > + * 1, any passed flag set > + * 0, flag unset > + */ > +static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag) > +{ > + struct acpi_table_header *table; > + acpi_status status; > + u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu); > + struct acpi_pptt_processor *cpu_node = NULL; > + int ret = -ENOENT; > + > + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); > + if (ACPI_FAILURE(status)) { > + acpi_pptt_warn_missing(); > + return ret; > + } > + > + if (table->revision >= rev) > + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); > + > + if (cpu_node) > + ret = (cpu_node->flags & flag) != 0; > + > + acpi_put_table(table); > + > + return ret; > +} > + > /** > * acpi_find_last_cache_level() - Determines the number of cache levels for a PE > * @cpu: Kernel logical CPU number > @@ -604,6 +642,21 @@ int cache_setup_acpi(unsigned int cpu) > return status; > } > > +/** > + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread > + * @cpu: Kernel logical CPU number > + * [nit] If you spin the patch again, you can drop extra line space here. -- Regards, Sudeep
Hi, Thanks for looking at this. On 8/1/19 10:57 AM, Sudeep Holla wrote: > > Hi Jeremy, > > On Wed, Jul 31, 2019 at 10:46:33PM -0500, Jeremy Linton wrote: >> ACPI 6.3 adds a flag to the CPU node to indicate whether >> the given PE is a thread. Add a function to return that >> information for a given linux logical CPU. >> > > Apart from few minor nits, > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/acpi/pptt.c | 54 +++++++++++++++++++++++++++++++++++++++++++- >> include/linux/acpi.h | 5 ++++ >> 2 files changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >> index 1e7ac0bd0d3a..84718f6cb741 100644 >> --- a/drivers/acpi/pptt.c >> +++ b/drivers/acpi/pptt.c >> @@ -540,6 +540,44 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag) >> return retval; >> } >> >> +/** >> + * check_acpi_cpu_flag() - Determine if CPU node has a flag set >> + * @cpu: Kernel logical CPU number >> + * @rev: The PPTT revision defining the flag > > [nit] I would rather put it as minimum PPTT revision that supports the > flag. It aligns with the code too as we are not looking for exact match. Ok, sure. > >> + * @flag: The flag itself >> + * >> + * Check the node representing a CPU for a given flag. >> + * >> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or >> + * the table revision isn't new enough. >> + * 1, any passed flag set >> + * 0, flag unset >> + */ >> +static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag) >> +{ >> + struct acpi_table_header *table; >> + acpi_status status; >> + u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu); >> + struct acpi_pptt_processor *cpu_node = NULL; >> + int ret = -ENOENT; >> + >> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); >> + if (ACPI_FAILURE(status)) { >> + acpi_pptt_warn_missing(); >> + return ret; >> + } >> + >> + if (table->revision >= rev) >> + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); >> + >> + if (cpu_node) >> + ret = (cpu_node->flags & flag) != 0; >> + >> + acpi_put_table(table); >> + >> + return ret; >> +} >> + >> /** >> * acpi_find_last_cache_level() - Determines the number of cache levels for a PE >> * @cpu: Kernel logical CPU number >> @@ -604,6 +642,21 @@ int cache_setup_acpi(unsigned int cpu) >> return status; >> } >> >> +/** >> + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread >> + * @cpu: Kernel logical CPU number >> + * > > [nit] If you spin the patch again, you can drop extra line space here. Sure.. > > -- > Regards, > Sudeep > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 31.07.19 22:46:33, Jeremy Linton wrote: > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 9426b9aaed86..9d0e20a2ac83 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1302,11 +1302,16 @@ static inline int lpit_read_residency_count_address(u64 *address) > #endif > > #ifdef CONFIG_ACPI_PPTT > +int acpi_pptt_cpu_is_thread(unsigned int cpu); > int find_acpi_cpu_topology(unsigned int cpu, int level); > int find_acpi_cpu_topology_package(unsigned int cpu); > int find_acpi_cpu_topology_hetero_id(unsigned int cpu); > int find_acpi_cpu_cache_topology(unsigned int cpu, int level); All those functions (exept hetero_id) are used only in parse_acpi_topology(). So how about creating a struct with thread_id, core_id, and cache_id (and hetero_id (?)) and have a single pptt table parsing function that fills in all of this into that struct? This simplifies the api and also the code. This also shows that hetid (see arm_pmu_acpi.c) better should be stored in cpu_topology[] too and thus being parsed with the other parameters as well and made accessible from there by a helper. -Robert
Hi, Thanks for taking a look at this. On 8/2/19 8:05 AM, Robert Richter wrote: > On 31.07.19 22:46:33, Jeremy Linton wrote: > >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 9426b9aaed86..9d0e20a2ac83 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1302,11 +1302,16 @@ static inline int lpit_read_residency_count_address(u64 *address) >> #endif >> >> #ifdef CONFIG_ACPI_PPTT >> +int acpi_pptt_cpu_is_thread(unsigned int cpu); >> int find_acpi_cpu_topology(unsigned int cpu, int level); >> int find_acpi_cpu_topology_package(unsigned int cpu); >> int find_acpi_cpu_topology_hetero_id(unsigned int cpu); >> int find_acpi_cpu_cache_topology(unsigned int cpu, int level); > > All those functions (exept hetero_id) are used only in > parse_acpi_topology(). So how about creating a struct with thread_id, > core_id, and cache_id (and hetero_id (?)) and have a single pptt table > parsing function that fills in all of this into that struct? This > simplifies the api and also the code. > > This also shows that hetid (see arm_pmu_acpi.c) better should be > stored in cpu_topology[] too and thus being parsed with the other > parameters as well and made accessible from there by a helper. I think the idea here was to avoid an additional set of intermediate data structures between the PPTT table/structure and the final arch specific data structures (which themselves are used to feed other things, like the scheduler, note the llc parsing). Rather the attempt is to provide a set of tools to retrieve information and let the policy for how that information is used be dictated by the consumer. In the future, if we can further unify the arch specific cpu_topology structures it would make sense to parse directly into them, but until that happens I don't think we should try. The existing code does parse directly into the cache structures, but the cpu topology is subtly arm64 at the moment. If another arch decided to use this, i'm not sure they would want or need it parsed in exactly the same way.
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index 1e7ac0bd0d3a..84718f6cb741 100644 --- a/drivers/acpi/pptt.c +++ b/drivers/acpi/pptt.c @@ -540,6 +540,44 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag) return retval; } +/** + * check_acpi_cpu_flag() - Determine if CPU node has a flag set + * @cpu: Kernel logical CPU number + * @rev: The PPTT revision defining the flag + * @flag: The flag itself + * + * Check the node representing a CPU for a given flag. + * + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or + * the table revision isn't new enough. + * 1, any passed flag set + * 0, flag unset + */ +static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag) +{ + struct acpi_table_header *table; + acpi_status status; + u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu); + struct acpi_pptt_processor *cpu_node = NULL; + int ret = -ENOENT; + + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); + if (ACPI_FAILURE(status)) { + acpi_pptt_warn_missing(); + return ret; + } + + if (table->revision >= rev) + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); + + if (cpu_node) + ret = (cpu_node->flags & flag) != 0; + + acpi_put_table(table); + + return ret; +} + /** * acpi_find_last_cache_level() - Determines the number of cache levels for a PE * @cpu: Kernel logical CPU number @@ -604,6 +642,21 @@ int cache_setup_acpi(unsigned int cpu) return status; } +/** + * acpi_pptt_cpu_is_thread() - Determine if CPU is a thread + * @cpu: Kernel logical CPU number + * + * + * Return: 1, a thread + * 0, not a thread + * -ENOENT ,if the PPTT doesn't exist, the CPU cannot be found or + * the table revision isn't new enough. + */ +int acpi_pptt_cpu_is_thread(unsigned int cpu) +{ + return check_acpi_cpu_flag(cpu, 2, ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD); +} + /** * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU * @cpu: Kernel logical CPU number @@ -664,7 +717,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level) return ret; } - /** * find_acpi_cpu_topology_package() - Determine a unique CPU package value * @cpu: Kernel logical CPU number diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 9426b9aaed86..9d0e20a2ac83 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1302,11 +1302,16 @@ static inline int lpit_read_residency_count_address(u64 *address) #endif #ifdef CONFIG_ACPI_PPTT +int acpi_pptt_cpu_is_thread(unsigned int cpu); int find_acpi_cpu_topology(unsigned int cpu, int level); int find_acpi_cpu_topology_package(unsigned int cpu); int find_acpi_cpu_topology_hetero_id(unsigned int cpu); int find_acpi_cpu_cache_topology(unsigned int cpu, int level); #else +static inline int acpi_pptt_cpu_is_thread(unsigned int cpu) +{ + return -EINVAL; +} static inline int find_acpi_cpu_topology(unsigned int cpu, int level) { return -EINVAL;
ACPI 6.3 adds a flag to the CPU node to indicate whether the given PE is a thread. Add a function to return that information for a given linux logical CPU. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/acpi/pptt.c | 54 +++++++++++++++++++++++++++++++++++++++++++- include/linux/acpi.h | 5 ++++ 2 files changed, 58 insertions(+), 1 deletion(-) -- 2.21.0