| Message ID | 20220317133419.3901736-3-Pierre.Gondois@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable EAS for CPPC/ACPI based systems | expand |
On 2022-03-17 13:34, Pierre Gondois wrote: > In ACPI, describing power efficiency of CPUs can be done through the > following arm specific field: > ACPI 6.4, s5.2.12.14 'GIC CPU Interface (GICC) Structure', > 'Processor Power Efficiency Class field': > Describes the relative power efficiency of the associated pro- > cessor. Lower efficiency class numbers are more efficient than > higher ones (e.g. efficiency class 0 should be treated as more > efficient than efficiency class 1). However, absolute values > of this number have no meaning: 2 isn’t necessarily half as > efficient as 1. > > The efficiency_class field is stored in the GicC structure of the > ACPI MADT table and it's currently supported in Linux for arm64 only. > Thus, this new functionality is introduced for arm64 only. > > To allow the cppc_cpufreq driver to know and preprocess the > efficiency_class values of all the CPUs, add a per_cpu efficiency_class > variable to store them. Also add a static efficiency_class_populated > to let the driver know efficiency_class values are usable and register > an artificial Energy Model (EM) based on normalized class values. > > At least 2 different efficiency classes must be present, > otherwise there is no use in creating an Energy Model. > > The efficiency_class values are squeezed in [0:#efficiency_class-1] > while conserving the order. For instance, efficiency classes of: > [111, 212, 250] > will be mapped to: > [0 (was 111), 1 (was 212), 2 (was 250)]. > > Each policy being independently registered in the driver, populating > the per_cpu efficiency_class is done only once at the driver > initialization. This prevents from having each policy re-searching the > efficiency_class values of other CPUs. > > The patch also exports acpi_cpu_get_madt_gicc() to fetch the GicC > structure of the ACPI MADT table for each CPU. > > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com> > --- > arch/arm64/kernel/smp.c | 1 + > drivers/cpufreq/cppc_cpufreq.c | 55 ++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index 27df5c1e6baa..56637cbea5d6 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -512,6 +512,7 @@ struct acpi_madt_generic_interrupt > *acpi_cpu_get_madt_gicc(int cpu) > { > return &cpu_madt_gicc[cpu]; > } > +EXPORT_SYMBOL(acpi_cpu_get_madt_gicc); Why not EXPORT_SYMBOL_GPL()? > > /* > * acpi_map_gic_cpu_interface - parse processor MADT entry > diff --git a/drivers/cpufreq/cppc_cpufreq.c > b/drivers/cpufreq/cppc_cpufreq.c > index 8f950fe72765..a6cd95c3b474 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -422,12 +422,66 @@ static unsigned int > cppc_cpufreq_get_transition_delay_us(unsigned int cpu) > return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; > } > > +static bool efficiency_class_populated; > +static DEFINE_PER_CPU(unsigned int, efficiency_class); > + > +static int populate_efficiency_class(void) > +{ > + unsigned int min = UINT_MAX, max = 0, class; > + struct acpi_madt_generic_interrupt *gicc; > + int cpu; > + > + for_each_possible_cpu(cpu) { > + gicc = acpi_cpu_get_madt_gicc(cpu); > + if (!gicc) > + return -ENODEV; How can that happen if you made it here using ACPI? > + > + per_cpu(efficiency_class, cpu) = gicc->efficiency_class; > + min = min_t(unsigned int, min, gicc->efficiency_class); > + max = max_t(unsigned int, max, gicc->efficiency_class); > + } Why don't you use a temporary bitmap of 256 bits, tracking the classes that are actually being used? > + > + if (min == max) { This would become (bitmap_weight(used_classes) <= 1). Then from the same construct you know how many different classes you have. You also have the min, max, and all the values in between. > + pr_debug("Efficiency classes are all equal (=%d). " > + "No EM registered", max); > + return -EINVAL; > + } > + > + /* > + * Squeeze efficiency class values on [0:#efficiency_class-1]. > + * Values are per spec in [0:255]. > + */ > + for (class = 0; class < 256; class++) { > + unsigned int new_min, curr; > + > + new_min = UINT_MAX; > + for_each_possible_cpu(cpu) { > + curr = per_cpu(efficiency_class, cpu); > + if (curr == min) > + per_cpu(efficiency_class, cpu) = class; > + else if (curr > min) > + new_min = min(new_min, curr); > + } > + > + if (new_min == UINT_MAX) > + break; > + min = new_min; > + } I find it really hard to reason about this because you are dynamically rewriting the values you keep reevaluating. How about something like this, which I find more readable: DECLARE_BITMAP(used_classes, 256) = {}; int class, index, cpu; for_each_possible_cpu(cpu) { unsigned int ec; ec = acpi_cpu_get_madt_gicc(cpu)->efficiency_class & 0xff; bitmap_set(ec, &used_classes); } if (bitmap_weight(&used_classes, 256) <= 1) return; index = 0; for_each_set_bit(class, &used_classes, 256) { for_each_possible_cpu(cpu) { if (acpi_cpu_get_madt_gicc(cpu)->efficiency_class == class) per_cpu(efficiency_class, cpu) = index; } index++; } Thanks, M.
On 3/17/22 16:13, Marc Zyngier wrote: > On 2022-03-17 13:34, Pierre Gondois wrote: >> In ACPI, describing power efficiency of CPUs can be done through the >> following arm specific field: >> ACPI 6.4, s5.2.12.14 'GIC CPU Interface (GICC) Structure', >> 'Processor Power Efficiency Class field': >> Describes the relative power efficiency of the associated pro- >> cessor. Lower efficiency class numbers are more efficient than >> higher ones (e.g. efficiency class 0 should be treated as more >> efficient than efficiency class 1). However, absolute values >> of this number have no meaning: 2 isn’t necessarily half as >> efficient as 1. >> >> The efficiency_class field is stored in the GicC structure of the >> ACPI MADT table and it's currently supported in Linux for arm64 only. >> Thus, this new functionality is introduced for arm64 only. >> >> To allow the cppc_cpufreq driver to know and preprocess the >> efficiency_class values of all the CPUs, add a per_cpu efficiency_class >> variable to store them. Also add a static efficiency_class_populated >> to let the driver know efficiency_class values are usable and register >> an artificial Energy Model (EM) based on normalized class values. >> >> At least 2 different efficiency classes must be present, >> otherwise there is no use in creating an Energy Model. >> >> The efficiency_class values are squeezed in [0:#efficiency_class-1] >> while conserving the order. For instance, efficiency classes of: >> [111, 212, 250] >> will be mapped to: >> [0 (was 111), 1 (was 212), 2 (was 250)]. >> >> Each policy being independently registered in the driver, populating >> the per_cpu efficiency_class is done only once at the driver >> initialization. This prevents from having each policy re-searching the >> efficiency_class values of other CPUs. >> >> The patch also exports acpi_cpu_get_madt_gicc() to fetch the GicC >> structure of the ACPI MADT table for each CPU. >> >> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com> >> --- >> arch/arm64/kernel/smp.c | 1 + >> drivers/cpufreq/cppc_cpufreq.c | 55 ++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c >> index 27df5c1e6baa..56637cbea5d6 100644 >> --- a/arch/arm64/kernel/smp.c >> +++ b/arch/arm64/kernel/smp.c >> @@ -512,6 +512,7 @@ struct acpi_madt_generic_interrupt >> *acpi_cpu_get_madt_gicc(int cpu) >> { >> return &cpu_madt_gicc[cpu]; >> } >> +EXPORT_SYMBOL(acpi_cpu_get_madt_gicc); > > Why not EXPORT_SYMBOL_GPL()? From what I understand, this could be made EXPORT_SYMBOL_GPL(). The only reason was that the other symbol exportation in the file wasn't restricted to GPL. > >> >> /* >> * acpi_map_gic_cpu_interface - parse processor MADT entry >> diff --git a/drivers/cpufreq/cppc_cpufreq.c >> b/drivers/cpufreq/cppc_cpufreq.c >> index 8f950fe72765..a6cd95c3b474 100644 >> --- a/drivers/cpufreq/cppc_cpufreq.c >> +++ b/drivers/cpufreq/cppc_cpufreq.c >> @@ -422,12 +422,66 @@ static unsigned int >> cppc_cpufreq_get_transition_delay_us(unsigned int cpu) >> return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; >> } >> >> +static bool efficiency_class_populated; >> +static DEFINE_PER_CPU(unsigned int, efficiency_class); >> + >> +static int populate_efficiency_class(void) >> +{ >> + unsigned int min = UINT_MAX, max = 0, class; >> + struct acpi_madt_generic_interrupt *gicc; >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + gicc = acpi_cpu_get_madt_gicc(cpu); >> + if (!gicc) >> + return -ENODEV; > > How can that happen if you made it here using ACPI? This is effectively an extra check. This could be removed. > >> + >> + per_cpu(efficiency_class, cpu) = gicc->efficiency_class; >> + min = min_t(unsigned int, min, gicc->efficiency_class); >> + max = max_t(unsigned int, max, gicc->efficiency_class); >> + } > > Why don't you use a temporary bitmap of 256 bits, tracking > the classes that are actually being used? > >> + >> + if (min == max) { > > This would become (bitmap_weight(used_classes) <= 1). Then from > the same construct you know how many different classes you have. > You also have the min, max, and all the values in between. > >> + pr_debug("Efficiency classes are all equal (=%d). " >> + "No EM registered", max); >> + return -EINVAL; >> + } >> + >> + /* >> + * Squeeze efficiency class values on [0:#efficiency_class-1]. >> + * Values are per spec in [0:255]. >> + */ >> + for (class = 0; class < 256; class++) { >> + unsigned int new_min, curr; >> + >> + new_min = UINT_MAX; >> + for_each_possible_cpu(cpu) { >> + curr = per_cpu(efficiency_class, cpu); >> + if (curr == min) >> + per_cpu(efficiency_class, cpu) = class; >> + else if (curr > min) >> + new_min = min(new_min, curr); >> + } >> + >> + if (new_min == UINT_MAX) >> + break; >> + min = new_min; >> + } > > I find it really hard to reason about this because you are > dynamically rewriting the values you keep reevaluating. > > How about something like this, which I find more readable: > > DECLARE_BITMAP(used_classes, 256) = {}; > int class, index, cpu; > > for_each_possible_cpu(cpu) { > unsigned int ec; > > ec = acpi_cpu_get_madt_gicc(cpu)->efficiency_class & 0xff; > bitmap_set(ec, &used_classes); > } > > if (bitmap_weight(&used_classes, 256) <= 1) > return; > > index = 0; > > for_each_set_bit(class, &used_classes, 256) { > for_each_possible_cpu(cpu) { > if (acpi_cpu_get_madt_gicc(cpu)->efficiency_class == class) > per_cpu(efficiency_class, cpu) = index; > } > > index++; > } This is effectively much more readable. Thanks for the code snippet. Regards, Pierre > > > Thanks, > > M.
On Thu, 17 Mar 2022 16:07:01 +0000, Pierre Gondois <pierre.gondois@arm.com> wrote: > > >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > >> index 27df5c1e6baa..56637cbea5d6 100644 > >> --- a/arch/arm64/kernel/smp.c > >> +++ b/arch/arm64/kernel/smp.c > >> @@ -512,6 +512,7 @@ struct acpi_madt_generic_interrupt > >> *acpi_cpu_get_madt_gicc(int cpu) > >> { > >> return &cpu_madt_gicc[cpu]; > >> } > >> +EXPORT_SYMBOL(acpi_cpu_get_madt_gicc); > > > > Why not EXPORT_SYMBOL_GPL()? > > From what I understand, this could be made EXPORT_SYMBOL_GPL(). > The only reason was that the other symbol exportation in the > file wasn't restricted to GPL. I'm personally keen on keeping this for GPL code only, just like the current code is. If there is a further need to relax this, we can discuss it separately. > > > > >> > >> /* > >> * acpi_map_gic_cpu_interface - parse processor MADT entry > >> diff --git a/drivers/cpufreq/cppc_cpufreq.c > >> b/drivers/cpufreq/cppc_cpufreq.c > >> index 8f950fe72765..a6cd95c3b474 100644 > >> --- a/drivers/cpufreq/cppc_cpufreq.c > >> +++ b/drivers/cpufreq/cppc_cpufreq.c > >> @@ -422,12 +422,66 @@ static unsigned int > >> cppc_cpufreq_get_transition_delay_us(unsigned int cpu) > >> return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; > >> } > >> > >> +static bool efficiency_class_populated; > >> +static DEFINE_PER_CPU(unsigned int, efficiency_class); > >> + > >> +static int populate_efficiency_class(void) > >> +{ > >> + unsigned int min = UINT_MAX, max = 0, class; > >> + struct acpi_madt_generic_interrupt *gicc; > >> + int cpu; > >> + > >> + for_each_possible_cpu(cpu) { > >> + gicc = acpi_cpu_get_madt_gicc(cpu); > >> + if (!gicc) > >> + return -ENODEV; > > > > How can that happen if you made it here using ACPI? > > This is effectively an extra check. This could be removed. Please do. Thanks, M.
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 27df5c1e6baa..56637cbea5d6 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -512,6 +512,7 @@ struct acpi_madt_generic_interrupt *acpi_cpu_get_madt_gicc(int cpu) { return &cpu_madt_gicc[cpu]; } +EXPORT_SYMBOL(acpi_cpu_get_madt_gicc); /* * acpi_map_gic_cpu_interface - parse processor MADT entry diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index 8f950fe72765..a6cd95c3b474 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -422,12 +422,66 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu) return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; } +static bool efficiency_class_populated; +static DEFINE_PER_CPU(unsigned int, efficiency_class); + +static int populate_efficiency_class(void) +{ + unsigned int min = UINT_MAX, max = 0, class; + struct acpi_madt_generic_interrupt *gicc; + int cpu; + + for_each_possible_cpu(cpu) { + gicc = acpi_cpu_get_madt_gicc(cpu); + if (!gicc) + return -ENODEV; + + per_cpu(efficiency_class, cpu) = gicc->efficiency_class; + min = min_t(unsigned int, min, gicc->efficiency_class); + max = max_t(unsigned int, max, gicc->efficiency_class); + } + + if (min == max) { + pr_debug("Efficiency classes are all equal (=%d). " + "No EM registered", max); + return -EINVAL; + } + + /* + * Squeeze efficiency class values on [0:#efficiency_class-1]. + * Values are per spec in [0:255]. + */ + for (class = 0; class < 256; class++) { + unsigned int new_min, curr; + + new_min = UINT_MAX; + for_each_possible_cpu(cpu) { + curr = per_cpu(efficiency_class, cpu); + if (curr == min) + per_cpu(efficiency_class, cpu) = class; + else if (curr > min) + new_min = min(new_min, curr); + } + + if (new_min == UINT_MAX) + break; + min = new_min; + } + + efficiency_class_populated = true; + return 0; +} + #else static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu) { return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; } +static int populate_efficiency_class(void) +{ + return 0; +} #endif @@ -757,6 +811,7 @@ static int __init cppc_cpufreq_init(void) cppc_check_hisi_workaround(); cppc_freq_invariance_init(); + populate_efficiency_class(); ret = cpufreq_register_driver(&cppc_cpufreq_driver); if (ret)
In ACPI, describing power efficiency of CPUs can be done through the following arm specific field: ACPI 6.4, s5.2.12.14 'GIC CPU Interface (GICC) Structure', 'Processor Power Efficiency Class field': Describes the relative power efficiency of the associated pro- cessor. Lower efficiency class numbers are more efficient than higher ones (e.g. efficiency class 0 should be treated as more efficient than efficiency class 1). However, absolute values of this number have no meaning: 2 isn’t necessarily half as efficient as 1. The efficiency_class field is stored in the GicC structure of the ACPI MADT table and it's currently supported in Linux for arm64 only. Thus, this new functionality is introduced for arm64 only. To allow the cppc_cpufreq driver to know and preprocess the efficiency_class values of all the CPUs, add a per_cpu efficiency_class variable to store them. Also add a static efficiency_class_populated to let the driver know efficiency_class values are usable and register an artificial Energy Model (EM) based on normalized class values. At least 2 different efficiency classes must be present, otherwise there is no use in creating an Energy Model. The efficiency_class values are squeezed in [0:#efficiency_class-1] while conserving the order. For instance, efficiency classes of: [111, 212, 250] will be mapped to: [0 (was 111), 1 (was 212), 2 (was 250)]. Each policy being independently registered in the driver, populating the per_cpu efficiency_class is done only once at the driver initialization. This prevents from having each policy re-searching the efficiency_class values of other CPUs. The patch also exports acpi_cpu_get_madt_gicc() to fetch the GicC structure of the ACPI MADT table for each CPU. Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com> --- arch/arm64/kernel/smp.c | 1 + drivers/cpufreq/cppc_cpufreq.c | 55 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)