* [PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support @ 2019-06-28 18:14 Jeremy Linton 2019-06-28 18:14 ` [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton 2019-06-28 18:14 ` [PATCH v3 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton 0 siblings, 2 replies; 9+ messages in thread From: Jeremy Linton @ 2019-06-28 18:14 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, catalin.marinas, will.deacon, rjw, lenb, lorenzo.pieralisi, sudeep.holla, Jeremy Linton ACPI 6.3 adds a flag to the CPU node to indicate whether the given CPU is a thread. Add a function to return that information for a given linux logical CPU and then utilize it while building the arm64 topology. v2->v3: Clarify and tweak the return from check_acpi_cpu_flag() Cache the PPTT table revision to avoid repeat acpi_table_get/put calls in the case of missing or old PPTT tables. v1->v2: Return ENOENT instead on ENONET. Jeremy Linton (2): ACPI/PPTT: Add support for ACPI 6.3 thread flag arm64: topology: Use PPTT to determine if PE is a thread arch/arm64/kernel/topology.c | 8 +++-- drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++- include/linux/acpi.h | 5 +++ 3 files changed, 71 insertions(+), 4 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-06-28 18:14 [PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton @ 2019-06-28 18:14 ` Jeremy Linton 2019-07-03 9:24 ` Rafael J. Wysocki 2019-06-28 18:14 ` [PATCH v3 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton 1 sibling, 1 reply; 9+ messages in thread From: Jeremy Linton @ 2019-06-28 18:14 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, catalin.marinas, will.deacon, rjw, lenb, lorenzo.pieralisi, sudeep.holla, Jeremy Linton 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 | 62 +++++++++++++++++++++++++++++++++++++++++++- include/linux/acpi.h | 5 ++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index b72e6afaa8fb..bb6196422fad 100644 --- a/drivers/acpi/pptt.c +++ b/drivers/acpi/pptt.c @@ -517,6 +517,52 @@ 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; + static int saved_pptt_rev = -1; + + /* Cache the PPTT revision to avoid repeat table get/put on failure */ + if (saved_pptt_rev > -1 && saved_pptt_rev < rev) + return -ENOENT; + + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); + if (ACPI_FAILURE(status)) { + saved_pptt_rev = 0; + acpi_pptt_warn_missing(); + return ret; + } + + saved_pptt_rev = table->revision; + + if (saved_pptt_rev >= 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 @@ -581,6 +627,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 @@ -641,7 +702,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 d315d86844e4..3e339375e213 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1301,10 +1301,15 @@ 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_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; -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-06-28 18:14 ` [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton @ 2019-07-03 9:24 ` Rafael J. Wysocki 2019-07-03 15:11 ` Jeremy Linton 2019-07-12 7:21 ` John Garry 0 siblings, 2 replies; 9+ messages in thread From: Rafael J. Wysocki @ 2019-07-03 9:24 UTC (permalink / raw) To: Jeremy Linton Cc: Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- > include/linux/acpi.h | 5 ++++ > 2 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index b72e6afaa8fb..bb6196422fad 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -517,6 +517,52 @@ 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) Why not bool? > +{ > + 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; > + static int saved_pptt_rev = -1; > + > + /* Cache the PPTT revision to avoid repeat table get/put on failure */ This is a rather questionable optimization. Does the extra table get/put really matter? > + if (saved_pptt_rev > -1 && saved_pptt_rev < rev) > + return -ENOENT; It should be fine to return 'false' here, shouldn't it? > + > + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); > + if (ACPI_FAILURE(status)) { > + saved_pptt_rev = 0; > + acpi_pptt_warn_missing(); > + return ret; And here? > + } > + > + saved_pptt_rev = table->revision; > + > + if (saved_pptt_rev >= rev) > + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); > + > + if (cpu_node) > + ret = (cpu_node->flags & flag) != 0; And this can be ret = !!(cpu_node->flags & flag); > + > + 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 > @@ -581,6 +627,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 > @@ -641,7 +702,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 d315d86844e4..3e339375e213 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1301,10 +1301,15 @@ 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_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; > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-07-03 9:24 ` Rafael J. Wysocki @ 2019-07-03 15:11 ` Jeremy Linton 2019-07-03 21:57 ` Rafael J. Wysocki 2019-07-12 7:21 ` John Garry 1 sibling, 1 reply; 9+ messages in thread From: Jeremy Linton @ 2019-07-03 15:11 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla Hi, Thanks for taking a look at this. On 7/3/19 4:24 AM, Rafael J. Wysocki wrote: > On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- >> include/linux/acpi.h | 5 ++++ >> 2 files changed, 66 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >> index b72e6afaa8fb..bb6196422fad 100644 >> --- a/drivers/acpi/pptt.c >> +++ b/drivers/acpi/pptt.c >> @@ -517,6 +517,52 @@ 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) > > Why not bool? At least for the thread flag we need the three states so that we can fall back to the CPU's description of itself on machines without ACPI 6.3 tables. The ThunderX2 is threaded and without a firmware update a change like this will break it. > >> +{ >> + 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; >> + static int saved_pptt_rev = -1; >> + >> + /* Cache the PPTT revision to avoid repeat table get/put on failure */ > > This is a rather questionable optimization. > > Does the extra table get/put really matter? AFAIK, Probably not. > >> + if (saved_pptt_rev > -1 && saved_pptt_rev < rev) >> + return -ENOENT; > > It should be fine to return 'false' here, shouldn't it? See bool, The alternative of course is a dedicated function to export this "error" but I think we want to avoid exporting what is effectively a version check into the reset of the kernel. > >> + >> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); >> + if (ACPI_FAILURE(status)) { >> + saved_pptt_rev = 0; >> + acpi_pptt_warn_missing(); >> + return ret; > > And here? > >> + } >> + >> + saved_pptt_rev = table->revision; >> + >> + if (saved_pptt_rev >= rev) >> + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); >> + >> + if (cpu_node) >> + ret = (cpu_node->flags & flag) != 0; > > And this can be > > ret = !!(cpu_node->flags & flag); Ok, sure. > >> + >> + 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 >> @@ -581,6 +627,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 >> @@ -641,7 +702,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 d315d86844e4..3e339375e213 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1301,10 +1301,15 @@ 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_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; >> -- >> 2.21.0 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-07-03 15:11 ` Jeremy Linton @ 2019-07-03 21:57 ` Rafael J. Wysocki 2019-07-04 2:41 ` Jeremy Linton 0 siblings, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2019-07-03 21:57 UTC (permalink / raw) To: Jeremy Linton Cc: Rafael J. Wysocki, Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla On Wed, Jul 3, 2019 at 5:11 PM Jeremy Linton <jeremy.linton@arm.com> wrote: > > Hi, > > Thanks for taking a look at this. > > On 7/3/19 4:24 AM, Rafael J. Wysocki wrote: > > On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. > >> > >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > >> --- > >> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- > >> include/linux/acpi.h | 5 ++++ > >> 2 files changed, 66 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > >> index b72e6afaa8fb..bb6196422fad 100644 > >> --- a/drivers/acpi/pptt.c > >> +++ b/drivers/acpi/pptt.c > >> @@ -517,6 +517,52 @@ 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) > > > > Why not bool? > > At least for the thread flag we need the three states so that we can > fall back to the CPU's description of itself on machines without ACPI > 6.3 tables. > > The ThunderX2 is threaded and without a firmware update a change like > this will break it. Fair enough. > > > >> +{ > >> + 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; > >> + static int saved_pptt_rev = -1; > >> + > >> + /* Cache the PPTT revision to avoid repeat table get/put on failure */ > > > > This is a rather questionable optimization. > > > > Does the extra table get/put really matter? > > AFAIK, Probably not. Then why to optimize it? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-07-03 21:57 ` Rafael J. Wysocki @ 2019-07-04 2:41 ` Jeremy Linton 2019-07-09 7:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 9+ messages in thread From: Jeremy Linton @ 2019-07-04 2:41 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla Hi, On 7/3/19 4:57 PM, Rafael J. Wysocki wrote: > On Wed, Jul 3, 2019 at 5:11 PM Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> Hi, >> >> Thanks for taking a look at this. >> >> On 7/3/19 4:24 AM, Rafael J. Wysocki wrote: >>> On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. >>>> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> --- >>>> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- >>>> include/linux/acpi.h | 5 ++++ >>>> 2 files changed, 66 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >>>> index b72e6afaa8fb..bb6196422fad 100644 >>>> --- a/drivers/acpi/pptt.c >>>> +++ b/drivers/acpi/pptt.c >>>> @@ -517,6 +517,52 @@ 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) >>> >>> Why not bool? >> >> At least for the thread flag we need the three states so that we can >> fall back to the CPU's description of itself on machines without ACPI >> 6.3 tables. >> >> The ThunderX2 is threaded and without a firmware update a change like >> this will break it. > > Fair enough. > >>> >>>> +{ >>>> + 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; >>>> + static int saved_pptt_rev = -1; >>>> + >>>> + /* Cache the PPTT revision to avoid repeat table get/put on failure */ >>> >>> This is a rather questionable optimization. >>> >>> Does the extra table get/put really matter? >> >> AFAIK, Probably not. > > Then why to optimize it? There was some discussion in the v2 review thread about all the get/put operations which only existed to return failure for each core in the machine. https://www.spinics.net/lists/arm-kernel/msg735948.html I guess I should drop it, until we have some proof that there is a problem. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-07-04 2:41 ` Jeremy Linton @ 2019-07-09 7:19 ` Rafael J. Wysocki 0 siblings, 0 replies; 9+ messages in thread From: Rafael J. Wysocki @ 2019-07-09 7:19 UTC (permalink / raw) To: Jeremy Linton Cc: Rafael J. Wysocki, Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla On Thu, Jul 4, 2019 at 4:41 AM Jeremy Linton <jeremy.linton@arm.com> wrote: > > Hi, > > On 7/3/19 4:57 PM, Rafael J. Wysocki wrote: > > On Wed, Jul 3, 2019 at 5:11 PM Jeremy Linton <jeremy.linton@arm.com> wrote: > >> > >> Hi, > >> > >> Thanks for taking a look at this. > >> > >> On 7/3/19 4:24 AM, Rafael J. Wysocki wrote: > >>> On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. > >>>> > >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > >>>> --- > >>>> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- > >>>> include/linux/acpi.h | 5 ++++ > >>>> 2 files changed, 66 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > >>>> index b72e6afaa8fb..bb6196422fad 100644 > >>>> --- a/drivers/acpi/pptt.c > >>>> +++ b/drivers/acpi/pptt.c > >>>> @@ -517,6 +517,52 @@ 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) > >>> > >>> Why not bool? > >> > >> At least for the thread flag we need the three states so that we can > >> fall back to the CPU's description of itself on machines without ACPI > >> 6.3 tables. > >> > >> The ThunderX2 is threaded and without a firmware update a change like > >> this will break it. > > > > Fair enough. > > > >>> > >>>> +{ > >>>> + 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; > >>>> + static int saved_pptt_rev = -1; > >>>> + > >>>> + /* Cache the PPTT revision to avoid repeat table get/put on failure */ > >>> > >>> This is a rather questionable optimization. > >>> > >>> Does the extra table get/put really matter? > >> > >> AFAIK, Probably not. > > > > Then why to optimize it? > > There was some discussion in the v2 review thread about all the get/put > operations which only existed to return failure for each core in the > machine. > > https://www.spinics.net/lists/arm-kernel/msg735948.html > > I guess I should drop it, until we have some proof that there is a problem. Yes, please. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag 2019-07-03 9:24 ` Rafael J. Wysocki 2019-07-03 15:11 ` Jeremy Linton @ 2019-07-12 7:21 ` John Garry 1 sibling, 0 replies; 9+ messages in thread From: John Garry @ 2019-07-12 7:21 UTC (permalink / raw) To: Rafael J. Wysocki, Jeremy Linton Cc: Linux ARM, ACPI Devel Maling List, Catalin Marinas, Will Deacon, Rafael J. Wysocki, Len Brown, Lorenzo Pieralisi, Sudeep Holla 在 03/07/2019 17:24, Rafael J. Wysocki 写道: > On Fri, Jun 28, 2019 at 8:15 PM Jeremy Linton <jeremy.linton@arm.com> 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. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++++++++++- >> include/linux/acpi.h | 5 ++++ >> 2 files changed, 66 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >> index b72e6afaa8fb..bb6196422fad 100644 >> --- a/drivers/acpi/pptt.c >> +++ b/drivers/acpi/pptt.c >> @@ -517,6 +517,52 @@ 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) > > Why not bool? > >> +{ >> + 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; >> + static int saved_pptt_rev = -1; >> + >> + /* Cache the PPTT revision to avoid repeat table get/put on failure */ > > This is a rather questionable optimization. > > Does the extra table get/put really matter? I would have made saved_pptt_rev a global, but, indeed, caching the value is not so important. > >> + if (saved_pptt_rev > -1 && saved_pptt_rev < rev) >> + return -ENOENT; > > It should be fine to return 'false' here, shouldn't it? > >> + >> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table); >> + if (ACPI_FAILURE(status)) { >> + saved_pptt_rev = 0; >> + acpi_pptt_warn_missing(); >> + return ret; > > And here? > >> + } >> + >> + saved_pptt_rev = table->revision; >> + >> + if (saved_pptt_rev >= rev) >> + cpu_node = acpi_find_processor_node(table, acpi_cpu_id); >> + >> + if (cpu_node) >> + ret = (cpu_node->flags & flag) != 0; > > And this can be > > ret = !!(cpu_node->flags & flag); > >> + >> + 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 >> @@ -581,6 +627,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); >> +} You could also consider the following to avoid the tri-state return value: bool acpi_pptt_cpu_find_threaded(unsigned int cpu, bool *is_threaded) >> + >> /** >> * find_acpi_cpu_topology() - Determine a unique topology value for a given CPU >> * @cpu: Kernel logical CPU number >> @@ -641,7 +702,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 d315d86844e4..3e339375e213 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1301,10 +1301,15 @@ 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_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; >> -- >> 2.21.0 >> > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] arm64: topology: Use PPTT to determine if PE is a thread 2019-06-28 18:14 [PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton 2019-06-28 18:14 ` [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton @ 2019-06-28 18:14 ` Jeremy Linton 1 sibling, 0 replies; 9+ messages in thread From: Jeremy Linton @ 2019-06-28 18:14 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, catalin.marinas, will.deacon, rjw, lenb, lorenzo.pieralisi, sudeep.holla, Jeremy Linton ACPI 6.3 adds a thread flag to represent if a CPU/PE is actually a thread. Given that the MPIDR_MT bit may not represent this information consistently on homogeneous machines we should prefer the PPTT flag if its available. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/kernel/topology.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 0825c4a856e3..cbbedb53cf06 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -346,11 +346,9 @@ void remove_cpu_topology(unsigned int cpu) */ static int __init parse_acpi_topology(void) { - bool is_threaded; + int is_threaded; int cpu, topology_id; - is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK; - for_each_possible_cpu(cpu) { int i, cache_id; @@ -358,6 +356,10 @@ static int __init parse_acpi_topology(void) if (topology_id < 0) return topology_id; + is_threaded = acpi_pptt_cpu_is_thread(cpu); + if (is_threaded < 0) + is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK; + if (is_threaded) { cpu_topology[cpu].thread_id = topology_id; topology_id = find_acpi_cpu_topology(cpu, 1); -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-12 7:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-28 18:14 [PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support Jeremy Linton 2019-06-28 18:14 ` [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag Jeremy Linton 2019-07-03 9:24 ` Rafael J. Wysocki 2019-07-03 15:11 ` Jeremy Linton 2019-07-03 21:57 ` Rafael J. Wysocki 2019-07-04 2:41 ` Jeremy Linton 2019-07-09 7:19 ` Rafael J. Wysocki 2019-07-12 7:21 ` John Garry 2019-06-28 18:14 ` [PATCH v3 2/2] arm64: topology: Use PPTT to determine if PE is a thread Jeremy Linton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).