[PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support

[PATCH v3 0/2] arm64/PPTT ACPI 6.3 thread flag support

[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

[PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[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

[PATCH v3 2/2] arm64: topology: Use PPTT to determine if PE is a thread

[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

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[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?

> +       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
>

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[Jeremy Linton]

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
>>

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[Rafael J. Wysocki]

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?

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[Jeremy Linton]

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.

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[Rafael J. Wysocki]

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.

Re: [PATCH v3 1/2] ACPI/PPTT: Add support for ACPI 6.3 thread flag

[John Garry]

在 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
>>
> 
> .
>