| Message ID | 20190614223158.49575-2-jeremy.linton@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64/PPTT ACPI 6.3 thread flag support | expand |
Hi Jeremy, Few nits below. Also, I had a look at the other PPTT processor flags that were introduced in 6.3, and the only other one being used is ACPI_LEAF_NODE in acpi_pptt_leaf_node(). However that one already has a handle on the table header, so the check_acpi_cpu_flag() isn't of much help there. I don't believe the other existing flags will benefit from the helper since they are more about describing the PPTT tree, but I think it doesn't hurt to keep it around for potential future flags. On 14/06/2019 23:31, Jeremy Linton wrote: [...] > @@ -517,6 +517,43 @@ 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. > + * Otherwise returns flag value > + */ Nit: strictly speaking we're not returning the flag value but its mask applied to the flags field. I don't think anyone will care about getting the actual flag value, but it should be made obvious in the doc: -ENOENT if ... 0 if the flag isn't set > 0 if it is set. [...] > @@ -581,6 +618,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: extra newline > + * 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 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level) [...]
On 6/17/19 7:34 AM, Valentin Schneider wrote: > Hi Jeremy, > > Few nits below. > > Also, I had a look at the other PPTT processor flags that were introduced > in 6.3, and the only other one being used is ACPI_LEAF_NODE in > acpi_pptt_leaf_node(). However that one already has a handle on the table > header, so the check_acpi_cpu_flag() isn't of much help there. > > I don't believe the other existing flags will benefit from the helper since > they are more about describing the PPTT tree, but I think it doesn't hurt > to keep it around for potential future flags. That was the thought process. > > On 14/06/2019 23:31, Jeremy Linton wrote: > [...] >> @@ -517,6 +517,43 @@ 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. >> + * Otherwise returns flag value >> + */ > > Nit: strictly speaking we're not returning the flag value but its mask > applied to the flags field. I don't think anyone will care about getting > the actual flag value, but it should be made obvious in the doc: Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too? > > -ENOENT if ... > 0 if the flag isn't set >> 0 if it is set. > > [...] >> @@ -581,6 +618,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: extra newline > >> + * 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 +693,6 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level) > [...] >
On 18/06/2019 15:40, Valentin Schneider wrote: > On 18/06/2019 15:21, Jeremy Linton wrote: > [...] >>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or >>>> + * the table revision isn't new enough. >>>> + * Otherwise returns flag value >>>> + */ >>> >>> Nit: strictly speaking we're not returning the flag value but its mask >>> applied to the flags field. I don't think anyone will care about getting >>> the actual flag value, but it should be made obvious in the doc: >> >> Or I clarify the code to actually do what the comments says. Maybe that is what John G was also pointing out too? >> No, I was just saying that the kernel topology can be broken without this series. > > Mmm I didn't find any reply from John regarding this in v1, but I wouldn't > mind either way, as long as the doc & code are aligned. > BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too much, i.e. check if the PPTT is new enough to support the thread flag and also check if it is set for a specific cpu. I'd consider separate functions here. thanks, John > [...] > > . >
Hi, On 6/18/19 12:23 PM, John Garry wrote: > On 18/06/2019 15:40, Valentin Schneider wrote: >> On 18/06/2019 15:21, Jeremy Linton wrote: >> [...] >>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be >>>>> found or >>>>> + * the table revision isn't new enough. >>>>> + * Otherwise returns flag value >>>>> + */ >>>> >>>> Nit: strictly speaking we're not returning the flag value but its mask >>>> applied to the flags field. I don't think anyone will care about >>>> getting >>>> the actual flag value, but it should be made obvious in the doc: >>> >>> Or I clarify the code to actually do what the comments says. Maybe >>> that is what John G was also pointing out too? >>> > > No, I was just saying that the kernel topology can be broken without > this series. > >> >> Mmm I didn't find any reply from John regarding this in v1, but I >> wouldn't >> mind either way, as long as the doc & code are aligned. >> > > BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too > much, i.e. check if the PPTT is new enough to support the thread flag > and also check if it is set for a specific cpu. I'd consider separate > functions here. ? Your suggesting replacing the if (table->revision >= rev) cpu_node = acpi_find_processor_node(table, acpi_cpu_id); check with if (revision_check(table, rev)) cpu_node = acpi_find_processor_node(table, acpi_cpu_id); and a function like static int revision_check(acpixxxx *table, int rev) { return (table->revision >= rev); } Although, frankly if one were to do this, it should probably be a macro with the table type, and used in the dozen or so other places I found doing similar checks (spcr, iort, etc). Or something else? > > thanks, > John > >> [...] >> >> . >> > >
On 18/06/2019 22:28, Jeremy Linton wrote: > Hi, > > On 6/18/19 12:23 PM, John Garry wrote: >> On 18/06/2019 15:40, Valentin Schneider wrote: >>> On 18/06/2019 15:21, Jeremy Linton wrote: >>> [...] >>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be >>>>>> found or >>>>>> + * the table revision isn't new enough. >>>>>> + * Otherwise returns flag value >>>>>> + */ >>>>> >>>>> Nit: strictly speaking we're not returning the flag value but its mask >>>>> applied to the flags field. I don't think anyone will care about >>>>> getting >>>>> the actual flag value, but it should be made obvious in the doc: >>>> >>>> Or I clarify the code to actually do what the comments says. Maybe >>>> that is what John G was also pointing out too? >>>> >> >> No, I was just saying that the kernel topology can be broken without >> this series. >> >>> >>> Mmm I didn't find any reply from John regarding this in v1, but I >>> wouldn't >>> mind either way, as long as the doc & code are aligned. >>> >> >> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too >> much, i.e. check if the PPTT is new enough to support the thread flag >> and also check if it is set for a specific cpu. I'd consider separate >> functions here. > Hi, > ? Your suggesting replacing the > I am not saying definitely that this should be changed, it's just that acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a typical API format. How about acpi_pptt_support_thread_info(cpu) and acpi_pptt_cpu_is_threaded(cpu), both returning false/true only? None of this is ideal. BTW, Have you audited which arm64 systems have MT bit set legitimately? > > if (table->revision >= rev) I know that checking the table revision is not on the fast path, but it seems unnecessarily inefficient to always read it this way, I mean calling acpi_table_get(). Can you have a static value for the table revision? Or is this just how other table info is accessed in ACPI code? > cpu_node = acpi_find_processor_node(table, acpi_cpu_id); > > check with > > if (revision_check(table, rev)) > cpu_node = acpi_find_processor_node(table, acpi_cpu_id); > > > and a function like > > static int revision_check(acpixxxx *table, int rev) > { > return (table->revision >= rev); > } > > Although, frankly if one were to do this, it should probably be a macro > with the table type, and used in the dozen or so other places I found > doing similar checks (spcr, iort, etc). > > Or something else? > > > > thanks, John >> >>> [...] >>> >>> . >>> >> >> > > > . >
On Mon, Jun 17, 2019 at 01:34:51PM +0100, Valentin Schneider wrote: > Hi Jeremy, > > Few nits below. > > Also, I had a look at the other PPTT processor flags that were introduced > in 6.3, and the only other one being used is ACPI_LEAF_NODE in > acpi_pptt_leaf_node(). However that one already has a handle on the table > header, so the check_acpi_cpu_flag() isn't of much help there. > > I don't believe the other existing flags will benefit from the helper since > they are more about describing the PPTT tree, but I think it doesn't hurt > to keep it around for potential future flags. > > On 14/06/2019 23:31, Jeremy Linton wrote: > [...] > > @@ -517,6 +517,43 @@ 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 How about the "the processor structure flag being examined" ? > > + * > > + * 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. > > + * Otherwise returns flag value > > + */ > > Nit: strictly speaking we're not returning the flag value but its mask > applied to the flags field. I don't think anyone will care about getting > the actual flag value, but it should be made obvious in the doc: > I agree with that. I am also fine if you want to change the code to return 0 or 1 based on the flag value. It then aligns well with comment under acpi_pptt_cpu_is_thread. Either way, we just need consistency. -- Regards, Sudeep
Hi, On 6/19/19 4:15 AM, John Garry wrote: > On 18/06/2019 22:28, Jeremy Linton wrote: >> Hi, >> >> On 6/18/19 12:23 PM, John Garry wrote: >>> On 18/06/2019 15:40, Valentin Schneider wrote: >>>> On 18/06/2019 15:21, Jeremy Linton wrote: >>>> [...] >>>>>>> + * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be >>>>>>> found or >>>>>>> + * the table revision isn't new enough. >>>>>>> + * Otherwise returns flag value >>>>>>> + */ >>>>>> >>>>>> Nit: strictly speaking we're not returning the flag value but its >>>>>> mask >>>>>> applied to the flags field. I don't think anyone will care about >>>>>> getting >>>>>> the actual flag value, but it should be made obvious in the doc: >>>>> >>>>> Or I clarify the code to actually do what the comments says. Maybe >>>>> that is what John G was also pointing out too? >>>>> >>> >>> No, I was just saying that the kernel topology can be broken without >>> this series. >>> >>>> >>>> Mmm I didn't find any reply from John regarding this in v1, but I >>>> wouldn't >>>> mind either way, as long as the doc & code are aligned. >>>> >>> >>> BTW, to me, function acpi_pptt_cpu_is_thread() seems to try to do too >>> much, i.e. check if the PPTT is new enough to support the thread flag >>> and also check if it is set for a specific cpu. I'd consider separate >>> functions here. >> > > Hi, > >> ? Your suggesting replacing the >> > > I am not saying definitely that this should be changed, it's just that > acpi_pptt_cpu_is_thread() returning false, true, or "no entry" is not a > typical API format. > > How about acpi_pptt_support_thread_info(cpu) and > acpi_pptt_cpu_is_threaded(cpu), both returning false/true only? I'm not sure we want to be exporting what is effectively a version check into the rest of the code. Plus, AFAIK it doesn't really simplify anything except the case of ACPI machines with revision 1 PPTTs, because those would only be doing a single check and assuming the state of the MT bit. That MT check is suspect anyway, although AFAIK it gets the right answer on all machines that predate ACPI 6.3.. > > None of this is ideal. > > BTW, Have you audited which arm64 systems have MT bit set legitimately? Not formally, given I don't have access to everything available. > >> >> if (table->revision >= rev) > > I know that checking the table revision is not on the fast path, but it > seems unnecessarily inefficient to always read it this way, I mean > calling acpi_table_get(). > > Can you have a static value for the table revision? Or is this just how > other table info is accessed in ACPI code? Yes caching the revision internally would save a get/put per core, for older machines. I don't think its a big deal in normal operation but its a couple extra lines so... I will post it with an internally cached saved_pptt_rev. That will save CPU count get/puts in the case where the revision isn't new enough. > >> cpu_node = acpi_find_processor_node(table, acpi_cpu_id); >> >> check with >> >> if (revision_check(table, rev)) >> cpu_node = acpi_find_processor_node(table, acpi_cpu_id); >> >> >> and a function like >> >> static int revision_check(acpixxxx *table, int rev) >> { >> return (table->revision >= rev); >> } >> >> Although, frankly if one were to do this, it should probably be a macro >> with the table type, and used in the dozen or so other places I found >> doing similar checks (spcr, iort, etc). >> >> Or something else? >> >> >> >> > > thanks, > John
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index b72e6afaa8fb..6f45f8c07b46 100644 --- a/drivers/acpi/pptt.c +++ b/drivers/acpi/pptt.c @@ -517,6 +517,43 @@ 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. + * Otherwise returns flag value + */ +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; + + 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 +618,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 +693,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;
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 | 53 +++++++++++++++++++++++++++++++++++++++++++- include/linux/acpi.h | 5 +++++ 2 files changed, 57 insertions(+), 1 deletion(-) -- 2.21.0