From: Mark Rutland <mark.rutland@arm.com>
To: Gavin Shan <gshan@redhat.com>
Cc: lorenzo.pieralisi@arm.com, catalin.marinas@arm.com,
robin.murphy@arm.com, shan.gavin@gmail.com, sudeep.holla@arm.com,
will@kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 4/4] arm64: Remove CPU operations dereferencing array
Date: Thu, 19 Mar 2020 19:38:01 +0000 [thread overview]
Message-ID: <20200319193801.GE4876@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20200318230145.72097-5-gshan@redhat.com>
On Thu, Mar 19, 2020 at 10:01:45AM +1100, Gavin Shan wrote:
> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
> much memory has been used for this. Also, all secondary CPUs must use
> same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
> Pieralisi and Mark Rutland pointed out.
>
> This introduces two variables (@{boot,secondary}_cpu_ops) to store the
> CPU operations for boot CPU and secondary CPUs separately, which are
> figured out from device tree or ACPI table. The secondary CPUs which
> have inconsistent operations won't be brought up. With this, the CPU
> operations dereferencing array is removed and 2KB memory is saved. Note
> the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
> is simple enough and no need to have a separate function for it.
To be honest, I'm not too keen on this. We've generally tried to bucket
things as either global or per-cpu, and it's odd to go against that.
Is 2K a problem because it forms part of the static Image size? If so,
could we make this a percpu pointer instead, or is there a problem with
that?
Thanks,
Mark.
>
> Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
> 1 file changed, 39 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index e133011f64b5..a0f647d22e36 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -20,41 +20,20 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
> #endif
> extern const struct cpu_operations cpu_psci_ops;
>
> -static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> -
> -static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
> +static const struct cpu_operations *const available_cpu_ops[] __initconst = {
> &smp_spin_table_ops,
> - &cpu_psci_ops,
> - NULL,
> -};
> -
> -static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
> #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> &acpi_parking_protocol_ops,
> #endif
> &cpu_psci_ops,
> - NULL,
> };
> +static const struct cpu_operations *boot_cpu_ops __ro_after_init;
> +static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
>
> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
> -{
> - const struct cpu_operations *const *ops;
> -
> - ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
> -
> - while (*ops) {
> - if (!strcmp(name, (*ops)->name))
> - return *ops;
> -
> - ops++;
> - }
> -
> - return NULL;
> -}
> -
> -static const char *__init cpu_read_enable_method(int cpu)
> +static const struct cpu_operations * __init get_cpu_method(int cpu)
> {
> const char *enable_method;
> + int i;
>
> if (acpi_disabled) {
> struct device_node *dn = of_get_cpu_node(cpu, NULL);
> @@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
> }
> }
>
> - return enable_method;
> + if (!enable_method) {
> + pr_warn("No enable-method found on CPU %d\n", cpu);
> + return NULL;
> + }
> +
> + /* Search in the array with method */
> + for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
> + if (!strcmp(available_cpu_ops[i]->name, enable_method))
> + return available_cpu_ops[i];
> + }
> +
> + return NULL;
> }
> -/*
> - * Read a cpu's enable method and record it in cpu_ops.
> - */
> +
> int __init init_cpu_ops(int cpu)
> {
> - const char *enable_method = cpu_read_enable_method(cpu);
> + const struct cpu_operations *ops = get_cpu_method(cpu);
>
> - if (!enable_method)
> - return -ENODEV;
> -
> - cpu_ops[cpu] = cpu_get_ops(enable_method);
> - if (!cpu_ops[cpu]) {
> - pr_warn("Unsupported enable-method: %s\n", enable_method);
> + if (!ops)
> return -EOPNOTSUPP;
> +
> + /* Update boot CPU operations */
> + if (!cpu) {
> + boot_cpu_ops = ops;
> + return 0;
> + }
> +
> + /* Update secondary CPU operations if it's not initialized yet */
> + if (!secondary_cpu_ops) {
> + secondary_cpu_ops = ops;
> + return 0;
> + }
> +
> + /* We should have unified secondary CPU operations */
> + if (ops != secondary_cpu_ops) {
> + pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
> + ops->name, secondary_cpu_ops->name, cpu);
> + return -EINVAL;
> }
>
> return 0;
> @@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
>
> const struct cpu_operations *get_cpu_ops(int cpu)
> {
> - return cpu_ops[cpu];
> + return cpu ? secondary_cpu_ops : boot_cpu_ops;
> }
> --
> 2.23.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-03-19 19:38 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-18 23:01 [PATCH v5 0/4] arm64: Dereference CPU operations indirectly Gavin Shan
2020-03-18 23:01 ` [PATCH v5 1/4] arm64: Declare ACPI parking protocol CPU operation if needed Gavin Shan
2020-03-18 23:01 ` [PATCH v5 2/4] arm64: Rename cpu_read_ops() to init_cpu_ops() Gavin Shan
2020-03-18 23:01 ` [PATCH v5 3/4] arm64: Introduce get_cpu_ops() helper function Gavin Shan
2020-03-19 19:31 ` Mark Rutland
2020-03-18 23:01 ` [PATCH v5 4/4] arm64: Remove CPU operations dereferencing array Gavin Shan
2020-03-19 19:38 ` Mark Rutland [this message]
2020-03-19 22:54 ` Gavin Shan
2020-03-24 17:29 ` [PATCH v5 0/4] arm64: Dereference CPU operations indirectly Catalin Marinas
2020-03-25 11:49 ` Gavin Shan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200319193801.GE4876@lakrids.cambridge.arm.com \
--to=mark.rutland@arm.com \
--cc=catalin.marinas@arm.com \
--cc=gshan@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=robin.murphy@arm.com \
--cc=shan.gavin@gmail.com \
--cc=sudeep.holla@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).