Re: [PATCH v3 2/3] ACPI: PPTT: Leave the table mapped for the runtime usage

[Rafael J. Wysocki]

On Wed, Jul 20, 2022 at 2:56 PM Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> Currently, everytime an information needs to be fetched from the PPTT,
> the table is mapped via acpi_get_table() and unmapped after the use via
> acpi_put_table() which is fine. However we do this at runtime especially
> when the CPU is hotplugged out and plugged in back since we re-populate
> the cache topology and other information.
>
> However, with the support to fetch LLC information from the PPTT in the
> cpuhotplug path which is executed in the atomic context, it is preferred
> to avoid mapping and unmapping of the PPTT for every single use as the
> acpi_get_table() might sleep waiting for a mutex.
>
> In order to avoid the same, the table is needs to just mapped once on
> the boot CPU and is never unmapped allowing it to be used at runtime
> with out the hassle of mapping and unmapping the table.
>
> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>
> --
>
> Hi Rafael,
>
> Sorry to bother you again on this PPTT changes. Guenter reported an issue
> with lockdep enabled in -next that include my cacheinfo/arch_topology changes
> to utilise LLC from PPTT in the CPU hotplug path.
>
> Please ack the change once you are happy so that I can get it merged with
> other fixes via Greg's tree.

OK

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

>
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index dd3222a15c9c..c91342dcbcd6 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -533,21 +533,37 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
>         return -ENOENT;
>  }
>
> +
> +static struct acpi_table_header *acpi_get_pptt(void)
> +{
> +       static struct acpi_table_header *pptt;
> +       acpi_status status;
> +
> +       /*
> +        * PPTT will be used at runtime on every CPU hotplug in path, so we
> +        * don't need to call acpi_put_table() to release the table mapping.
> +        */
> +       if (!pptt) {
> +               status = acpi_get_table(ACPI_SIG_PPTT, 0, &pptt);
> +               if (ACPI_FAILURE(status))
> +                       acpi_pptt_warn_missing();
> +       }
> +
> +       return pptt;
> +}
> +
>  static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>  {
>         struct acpi_table_header *table;
> -       acpi_status status;
>         int retval;
>
> -       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> -       if (ACPI_FAILURE(status)) {
> -               acpi_pptt_warn_missing();
> +       table = acpi_get_pptt();
> +       if (!table)
>                 return -ENOENT;
> -       }
> +
>         retval = topology_get_acpi_cpu_tag(table, cpu, level, flag);
>         pr_debug("Topology Setup ACPI CPU %d, level %d ret = %d\n",
>                  cpu, level, retval);
> -       acpi_put_table(table);
>
>         return retval;
>  }
> @@ -568,16 +584,13 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>  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;
> -       }
> +       table = acpi_get_pptt();
> +       if (!table)
> +               return -ENOENT;
>
>         if (table->revision >= rev)
>                 cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
> @@ -585,8 +598,6 @@ static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
>         if (cpu_node)
>                 ret = (cpu_node->flags & flag) != 0;
>
> -       acpi_put_table(table);
> -
>         return ret;
>  }
>
> @@ -605,18 +616,15 @@ int acpi_find_last_cache_level(unsigned int cpu)
>         u32 acpi_cpu_id;
>         struct acpi_table_header *table;
>         int number_of_levels = 0;
> -       acpi_status status;
> +
> +       table = acpi_get_pptt();
> +       if (!table)
> +               return -ENOENT;
>
>         pr_debug("Cache Setup find last level CPU=%d\n", cpu);
>
>         acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> -       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> -       if (ACPI_FAILURE(status)) {
> -               acpi_pptt_warn_missing();
> -       } else {
> -               number_of_levels = acpi_find_cache_levels(table, acpi_cpu_id);
> -               acpi_put_table(table);
> -       }
> +       number_of_levels = acpi_find_cache_levels(table, acpi_cpu_id);
>         pr_debug("Cache Setup find last level level=%d\n", number_of_levels);
>
>         return number_of_levels;
> @@ -638,20 +646,16 @@ int acpi_find_last_cache_level(unsigned int cpu)
>  int cache_setup_acpi(unsigned int cpu)
>  {
>         struct acpi_table_header *table;
> -       acpi_status status;
> -
> -       pr_debug("Cache Setup ACPI CPU %d\n", cpu);
>
> -       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> -       if (ACPI_FAILURE(status)) {
> -               acpi_pptt_warn_missing();
> +       table = acpi_get_pptt();
> +       if (!table)
>                 return -ENOENT;
> -       }
> +
> +       pr_debug("Cache Setup ACPI CPU %d\n", cpu);
>
>         cache_setup_acpi_cpu(table, cpu);
> -       acpi_put_table(table);
>
> -       return status;
> +       return 0;
>  }
>
>  /**
> @@ -730,50 +734,38 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
>  int find_acpi_cpu_topology_cluster(unsigned int cpu)
>  {
>         struct acpi_table_header *table;
> -       acpi_status status;
>         struct acpi_pptt_processor *cpu_node, *cluster_node;
>         u32 acpi_cpu_id;
>         int retval;
>         int is_thread;
>
> -       status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
> -       if (ACPI_FAILURE(status)) {
> -               acpi_pptt_warn_missing();
> +       table = acpi_get_pptt();
> +       if (!table)
>                 return -ENOENT;
> -       }
>
>         acpi_cpu_id = get_acpi_id_for_cpu(cpu);
>         cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
> -       if (cpu_node == NULL || !cpu_node->parent) {
> -               retval = -ENOENT;
> -               goto put_table;
> -       }
> +       if (!cpu_node || !cpu_node->parent)
> +               return -ENOENT;
>
>         is_thread = cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD;
>         cluster_node = fetch_pptt_node(table, cpu_node->parent);
> -       if (cluster_node == NULL) {
> -               retval = -ENOENT;
> -               goto put_table;
> -       }
> +       if (!cluster_node)
> +               return -ENOENT;
> +
>         if (is_thread) {
> -               if (!cluster_node->parent) {
> -                       retval = -ENOENT;
> -                       goto put_table;
> -               }
> +               if (!cluster_node->parent)
> +                       return -ENOENT;
> +
>                 cluster_node = fetch_pptt_node(table, cluster_node->parent);
> -               if (cluster_node == NULL) {
> -                       retval = -ENOENT;
> -                       goto put_table;
> -               }
> +               if (!cluster_node)
> +                       return -ENOENT;
>         }
>         if (cluster_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID)
>                 retval = cluster_node->acpi_processor_id;
>         else
>                 retval = ACPI_PTR_DIFF(cluster_node, table);
>
> -put_table:
> -       acpi_put_table(table);
> -
>         return retval;
>  }
>
>
> --
> b4 0.10.0-dev-54fef