Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201105125524.4409-1-ionela.voinescu@arm.com> <20201117184920.17036-1-ionela.voinescu@arm.com>
In-Reply-To: <20201117184920.17036-1-ionela.voinescu@arm.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 23 Nov 2020 18:32:53 +0100
Message-ID: <CAJZ5v0h9rspagKCS7JfFS_Gx=scoYYA7gycbXEkOc29jsgDXsg@mail.gmail.com>
Subject: Re: [PATCH] cppc_cpufreq: optimise memory allocation for HW and NONE coordination
To:     Ionela Voinescu <ionela.voinescu@arm.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Len Brown <lenb@kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Jeremy Linton <jeremy.linton@arm.com>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 17, 2020 at 7:50 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> While the current domain and cpu lists are appropriate for ALL and ANY
> coordination types where single structures are kept for the domain and
> CPU data, they can be inefficient for NONE and HW coordination types,
> where domain information can end up being allocated either for a single
> CPU or a small number of CPUs.
>
> Therefore remove the psd_data structure and maintain a single CPU list.
> The cppc_cpudata structure will contain information about the PSD domain
> of the CPU: the ACPI coordination type and CPU content. This does result
> in the duplication of domain information in the cppc_cpudata structure
> (type and map), but it is more memory efficient for all coordination
> types, compared to allocating separate structures.
>
> In order to accommodate the struct list_head node in the cppc_cpudata
> structure, the now unused cpu and cur_policy variables are removed.
>
> This change affects all ACPI coordination types, with the greatest
> savings obtained for HW and NONE coordination, when the number of CPUs
> is large.
>
> For example, on a arm64 Juno platform with 6 CPUs:
>  - (0) 0, 1, 2, 3, 4, 5 - NONE coordination
>  - (1) (0, 1, 2, 3) in PSD1, (4, 5) in PSD2 - ANY coordination
>
> memory allocation comparison shows:
>
> Before patch: psd_data and cppc_cpudata structures are allocated for each
>               CPU (0) or domain (1).
>
>  - (0) NONE coordination:
>     total    slack      req alloc/free  caller
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7958
>      768      336      432     6/0     _kernel_size_le_hi32+0x0xffff800008ff7444
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ff7990
>      768       96      672     6/0     _kernel_size_le_hi32+0x0xffff800008ff7604
>
>  - (1) ANY coordination:
>     total    slack      req alloc/free  caller
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7990
>      256      112      144     2/0     _kernel_size_le_hi32+0x0xffff800008fe7444
>      256       32      224     2/0     _kernel_size_le_hi32+0x0xffff800008fe7604
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fe7958
>
> After patch: only cppc_cpudata is allocated for each CPU (0) or domain (1).
>
>  - (0) NONE coordination:
>     total    slack      req alloc/free  caller
>      768        0      768     6/0     _kernel_size_le_hi32+0x0xffff800008ffd410
>        0        0        0     0/6     _kernel_size_le_hi32+0x0xffff800008ffd274
>
>  - (1) ANY coordination:
>     total    slack      req alloc/free  caller
>      256        0      256     2/0     _kernel_size_le_hi32+0x0xffff800008fed410
>        0        0        0     0/2     _kernel_size_le_hi32+0x0xffff800008fed274
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>
> Hi guys,
>
> This is an optimisation/fix for the inefficient allocation that Jeremy
> mentioned for patch 4/8 in the series at [1]. This reverts the use of a
> separate psd_data structure and some of the changes done in cppc_cpudata,
> while adding the list_head needed to maintain a cpu list and removing the
> unused cpu and cur_policy variables.
>
> This patch is based on v5.10-rc4.
>
> Thanks,
> Ionela.
>
> [1] https://lore.kernel.org/linux-pm/20201105125524.4409-1-ionela.voinescu@arm.com/
>
>
>  drivers/acpi/cppc_acpi.c       |  20 ++---
>  drivers/cpufreq/cppc_cpufreq.c | 131 +++++++++++----------------------
>  include/acpi/cppc_acpi.h       |  15 +---
>  3 files changed, 54 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index e1e46cc66eeb..4e478f751ff7 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -416,11 +416,11 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
>  /**
>   * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu
>   * @cpu: Find all CPUs that share a domain with cpu.
> - * @domain: Pointer to given domain data storage
> + * @cpu_data: Pointer to CPU specific CPPC data including PSD info.
>   *
>   *     Return: 0 for success or negative value for err.
>   */
> -int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
> +int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data)
>  {
>         struct cpc_desc *cpc_ptr, *match_cpc_ptr;
>         struct acpi_psd_package *match_pdomain;
> @@ -436,18 +436,18 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>                 return -EFAULT;
>
>         pdomain = &(cpc_ptr->domain_info);
> -       cpumask_set_cpu(cpu, domain->shared_cpu_map);
> +       cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
>         if (pdomain->num_processors <= 1)
>                 return 0;
>
>         /* Validate the Domain info */
>         count_target = pdomain->num_processors;
>         if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_ALL;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ALL;
>         else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_HW;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_HW;
>         else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
> -               domain->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> +               cpu_data->shared_type = CPUFREQ_SHARED_TYPE_ANY;
>
>         for_each_possible_cpu(i) {
>                 if (i == cpu)
> @@ -468,16 +468,16 @@ int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain)
>                 if (pdomain->coord_type != match_pdomain->coord_type)
>                         goto err_fault;
>
> -               cpumask_set_cpu(i, domain->shared_cpu_map);
> +               cpumask_set_cpu(i, cpu_data->shared_cpu_map);
>         }
>
>         return 0;
>
>  err_fault:
>         /* Assume no coordination on any error parsing domain info */
> -       cpumask_clear(domain->shared_cpu_map);
> -       cpumask_set_cpu(cpu, domain->shared_cpu_map);
> -       domain->shared_type = CPUFREQ_SHARED_TYPE_NONE;
> +       cpumask_clear(cpu_data->shared_cpu_map);
> +       cpumask_set_cpu(cpu, cpu_data->shared_cpu_map);
> +       cpu_data->shared_type = CPUFREQ_SHARED_TYPE_NONE;
>
>         return -EFAULT;
>  }
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index b4edeeb57d04..bb4c068601db 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -31,10 +31,11 @@
>
>  /*
>   * This list contains information parsed from per CPU ACPI _CPC and _PSD
> - * structures: this is a list of domain data which in turn contains a list
> - * of cpus with their controls and capabilities, belonging to the domain.
> + * structures: e.g. the highest and lowest supported performance, capabilities,
> + * desired performance, level requested etc. Depending on the share_type, not
> + * all CPUs will have an entry in the list.
>   */
> -static LIST_HEAD(domain_list);
> +static LIST_HEAD(cpu_data_list);
>
>  static bool boost_supported;
>
> @@ -194,6 +195,12 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
>         if (ret)
>                 pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>                          caps->lowest_perf, cpu, ret);
> +
> +       /* Remove CPU node from list and free driver data for policy */
> +       free_cpumask_var(cpu_data->shared_cpu_map);
> +       list_del(&cpu_data->node);
> +       kfree(policy->driver_data);
> +       policy->driver_data = NULL;
>  }
>
>  /*
> @@ -239,105 +246,59 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
>  }
>  #endif
>
> -static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu)
> -{
> -       struct psd_data *domain;
> -       int ret;
> -
> -       list_for_each_entry(domain, &domain_list, node) {
> -               if (cpumask_test_cpu(cpu, domain->shared_cpu_map))
> -                       return domain;
> -       }
> -
> -       domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL);
> -       if (!domain)
> -               return NULL;
> -       if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL))
> -               goto free_domain;
> -       INIT_LIST_HEAD(&domain->cpu_list);
> -
> -       ret = acpi_get_psd_map(cpu, domain);
> -       if (ret) {
> -               pr_err("Error parsing PSD data for CPU%d.\n", cpu);
> -               goto free_mask;
> -       }
> -
> -       list_add(&domain->node, &domain_list);
> -
> -       return domain;
> -
> -free_mask:
> -       free_cpumask_var(domain->shared_cpu_map);
> -free_domain:
> -       kfree(domain);
> -
> -       return NULL;
> -}
>
>  static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
>  {
>         struct cppc_cpudata *cpu_data;
> -       struct psd_data *domain;
>         int ret;
>
> -       domain = cppc_cpufreq_get_domain(cpu);
> -       if (!domain) {
> -               pr_err("Error acquiring domain for CPU.\n");
> -               return NULL;
> -       }
> -
> -       list_for_each_entry(cpu_data, &domain->cpu_list, node) {
> -               if (cpu_data->cpu == cpu)
> -                       return cpu_data;
> -       }
> -
> -       if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) &&
> -           !list_empty(&domain->cpu_list))
> -               return list_first_entry(&domain->cpu_list,
> -                                       struct cppc_cpudata,
> -                                       node);
> -
>         cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
>         if (!cpu_data)
> -               return NULL;
> +               goto out;
> +
> +       if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL))
> +               goto free_cpu;
>
> -       cpu_data->cpu = cpu;
> -       cpu_data->domain = domain;
> +       ret = acpi_get_psd_map(cpu, cpu_data);
> +       if (ret) {
> +               pr_debug("Err parsing CPU%d PSD data: ret:%d\n", cpu, ret);
> +               goto free_mask;
> +       }
>
>         ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps);
>         if (ret) {
> -               pr_err("Err reading CPU%d perf capabilities. ret:%d\n",
> -                       cpu, ret);
> -               goto free;
> +               pr_debug("Err reading CPU%d perf caps: ret:%d\n", cpu, ret);
> +               goto free_mask;
>         }
> +
>         /* Convert the lowest and nominal freq from MHz to KHz */
>         cpu_data->perf_caps.lowest_freq *= 1000;
>         cpu_data->perf_caps.nominal_freq *= 1000;
>
> -       list_add(&cpu_data->node, &domain->cpu_list);
> +       list_add(&cpu_data->node, &cpu_data_list);
>
>         return cpu_data;
> -free:
> -       kfree(cpu_data);
>
> +free_mask:
> +       free_cpumask_var(cpu_data->shared_cpu_map);
> +free_cpu:
> +       kfree(cpu_data);
> +out:
>         return NULL;
>  }
>
>  static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  {
> -       struct cppc_cpudata *cpu_data = NULL;
> -       struct psd_data *domain = NULL;
>         unsigned int cpu = policy->cpu;
> +       struct cppc_cpudata *cpu_data;
>         struct cppc_perf_caps *caps;
> -       int ret = 0;
> +       int ret;
>
>         cpu_data = cppc_cpufreq_get_cpu_data(cpu);
>         if (!cpu_data) {
> -               pr_err("Error in acquiring _CPC/_PSD data for CPU.\n");
> +               pr_err("Error in acquiring _CPC/_PSD data for CPU%d.\n", cpu);
>                 return -ENODEV;
>         }
> -
> -       domain = cpu_data->domain;
>         caps = &cpu_data->perf_caps;
>         policy->driver_data = cpu_data;
>
> @@ -361,7 +322,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                                                             caps->nominal_perf);
>
>         policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
> -       policy->shared_type = domain->shared_type;
> +       policy->shared_type = cpu_data->shared_type;
>
>         switch (policy->shared_type) {
>         case CPUFREQ_SHARED_TYPE_HW:
> @@ -374,7 +335,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                  * operations will use a single cppc_cpudata structure stored
>                  * in policy->driver_data.
>                  */
> -               cpumask_copy(policy->cpus, domain->shared_cpu_map);
> +               cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
>                 break;
>         default:
>                 pr_info("Unsupported cpufreq CPU co-ord type: %d\n",
> @@ -382,8 +343,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>                 return -EFAULT;
>         }
>
> -       cpu_data->cur_policy = policy;
> -
>         /*
>          * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
>          * is supported.
> @@ -487,7 +446,7 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>  {
>         struct cppc_cpudata *cpu_data = policy->driver_data;
>
> -       return cpufreq_show_cpus(cpu_data->domain->shared_cpu_map, buf);
> +       return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
>  }
>  cpufreq_freq_attr_ro(freqdomain_cpus);
>
> @@ -558,38 +517,30 @@ static int __init cppc_cpufreq_init(void)
>         if (acpi_disabled)
>                 return -ENODEV;
>
> +       INIT_LIST_HEAD(&cpu_data_list);
> +
>         cppc_check_hisi_workaround();
>
>         return cpufreq_register_driver(&cppc_cpufreq_driver);
>  }
>
> -static inline void free_cpu_data(struct psd_data *domain)
> +static inline void free_cpu_data(void)
>  {
>         struct cppc_cpudata *iter, *tmp;
>
> -       list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) {
> +       list_for_each_entry_safe(iter, tmp, &cpu_data_list, node) {
> +               free_cpumask_var(iter->shared_cpu_map);
>                 list_del(&iter->node);
>                 kfree(iter);
>         }
> -}
> -
> -static inline void free_domain_data(void)
> -{
> -       struct psd_data *iter, *tmp;
>
> -       list_for_each_entry_safe(iter, tmp, &domain_list, node) {
> -               list_del(&iter->node);
> -               if (!list_empty(&iter->cpu_list))
> -                       free_cpu_data(iter);
> -               free_cpumask_var(iter->shared_cpu_map);
> -               kfree(iter);
> -       }
>  }
> +
>  static void __exit cppc_cpufreq_exit(void)
>  {
>         cpufreq_unregister_driver(&cppc_cpufreq_driver);
>
> -       free_domain_data();
> +       free_cpu_data();
>  }
>
>  module_exit(cppc_cpufreq_exit);
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index c0081fb6032e..dab6b3b4e315 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -122,30 +122,21 @@ struct cppc_perf_fb_ctrs {
>         u64 wraparound_time;
>  };
>
> -/* Container of performance state domain data */
> -struct psd_data {
> -       struct list_head node;
> -       unsigned int shared_type;
> -       struct list_head cpu_list;
> -       cpumask_var_t shared_cpu_map;
> -};
> -
>  /* Per CPU container for runtime CPPC management. */
>  struct cppc_cpudata {
> -       int cpu;
>         struct list_head node;
> -       struct psd_data *domain;
>         struct cppc_perf_caps perf_caps;
>         struct cppc_perf_ctrls perf_ctrls;
>         struct cppc_perf_fb_ctrs perf_fb_ctrs;
> -       struct cpufreq_policy *cur_policy;
> +       unsigned int shared_type;
> +       cpumask_var_t shared_cpu_map;
>  };
>
>  extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>  extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
>  extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
>  extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> -extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain);
> +extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
>  extern unsigned int cppc_get_transition_latency(int cpu);
>  extern bool cpc_ffh_supported(void);
>  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> --

Applied as 5.11 material (on top of the previous cppc_cpufreq patches), thanks!